-
-
Notifications
You must be signed in to change notification settings - Fork 15.3k
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
Proof of concept: fully create store before dispatching init action. #1576
Conversation
Wow, that’s a really neat approach. Is there any way we can forbid applying “old-style” enhancers now? |
Ive been trying to think of a way we could throw/warn, but not sure if its possible to detect. |
That's exactly what I did locally, this approach has one significiant drawback, it will not ensure that eg. |
} | ||
} | ||
|
||
var finalCreateStore = enhancer ? enhancer(createStore) : createStore |
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.
Enhancer can potentially dispatch an action which will be called before ActionTypes.INIT
therefore this does not ensure that first action has all the middlewares applied.
Can you point to where this happens? I don’t see it dispatching any initial actions in the source.
If we go ahead with this, we’ll forbid dispatching while enhancers/middleware are being created, kinda like #1485 but more generic. |
https://github.com/gaearon/redux-devtools/blob/master/src/instrument.js#L151
If that's acceptable then this may be a way to go. |
Can we change DevTools to fix this? In other words, if the behavior in PR was accepted in Redux, can we change DevTools code to be compatible with Redux Elm? |
It's not only about Redux Elm, there are workarounds but I am thinking more in general. Think about this: It's not good idea to check whether type of Action is In my opinion this is the only reason why Conclusion: Would this PR even make sense then? Because if people really want to bootstrap their app, they should probably provide their own Bootstrapping action because it's the only reliable way given the fact that some Enhancer could break it. |
Anyway, re-visited original problem and it looks like #465 (comment) justifies it. But still as @acdlite mentioned, it's questionable... |
All about tradeoffs. Being slightly hacky in two or three projects is better in my view than forcing everyone to add a custom As far as I remember the whole reason why we put initial action there in the DevTools is because of the problem this PR fixes. Unless I miss something I think it won’t be necessary if we go ahead with this PR. If this is correct it would solve both your and my problem :-) |
@gaearon what was the original reason for the INIT action? |
@tappleby Not sure what you mean. What was its purpose in DevTools, or generally? |
Meant generally. Was asking because I thought we might be able to do something like |
Closing in favor of #1702 but big thanks for providing the inspiration. |
Not ready to be merged in this state, looking for feedback. Proof of concept for my comment in #465.
The idea is to split up creating + initializing the store into separate phases, ensuring the store enhancer is applied before the first
@@INIT
action is dispatched.