-
Notifications
You must be signed in to change notification settings - Fork 2
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: enable HMR of persistentReducedStreams (opt in) #555
base: master
Are you sure you want to change the base?
Conversation
@@ -48,9 +49,31 @@ export const persistentReducedStream = <State>( | |||
tag(name) | |||
); | |||
|
|||
const stream = new ObservableState(name, source$, initialState); | |||
try { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are you using try-catch instead of module.hot?
That error detection seems the wrong trigger for hot reloading?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good question!
This file is not the file that is being hot reloaded, it is the file that uses this function that will be reloaded. So that file will run multiple times and register a persistentReducedStream each time, which typically is a duplicate reducer error that needs to be handled in order to allow HMR to happen.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see. Using that error type is an indirect pattern, that is basically not ideal. I think it would be better to add some info in dev mode to the subscriber that this is a subscription triggered by a hot reloading to make the reason for the re-subscription explicit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For this part of it. All looks good to me, except for the comment from chriskr.
But also have you tested that this is working?
It's working, but a better solution is required. I haven't had time to revisit it yet, but I'll circle back to it. I'm getting exposed to a lot more work with streams and rxbeach so I'll be better suited for the next iteration 👌 |
Adds:
What it does NOT handle:
reduceState()
andaction$.pipe()
sI'm hoping we can use this to start a discussion about how we can enable full HMR support over time, the challenge is mainly accessing the internal state of observables in order to set the initial state based on the previous state like this PR does with persistentReducedStreams.
Without being well versed in RXJS, I think the pattern of directly subscribing to the $action stream is problematic because it exposes implementation details to the end user, and prevents us from wrapping the registration to add things like HMR. To get where we want we need to be able to:
.pipe
seems to remove this flexibility from us/requires that the developer register HMR manuallyaction$.pipe
subscription in order to combine theaction$
stream with a development-modetakeUntil(onDestroy$)
I think it should be possible, my biggest problem at the moment is that I do not know RXJS well enough.
Not sure who else should review, but please add anyone who could have insight into this as I'm actively looking for feedback.