Skip to content
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

Closed
wants to merge 1 commit into from

Conversation

tappleby
Copy link
Contributor

@tappleby tappleby commented Apr 4, 2016

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.

@gaearon
Copy link
Contributor

gaearon commented Apr 4, 2016

Wow, that’s a really neat approach. Is there any way we can forbid applying “old-style” enhancers now?

@tappleby
Copy link
Contributor Author

tappleby commented Apr 4, 2016

Ive been trying to think of a way we could throw/warn, but not sure if its possible to detect.

@tomkis
Copy link
Contributor

tomkis commented Apr 4, 2016

That's exactly what I did locally, this approach has one significiant drawback, it will not ensure that @@INIT action is dispatched as first action (see my comment in original issue)

eg. redux-devtools will dispatch its own init function before redux/@@INIT and populates appstate beforehand... this will unfortunatelly not solve our issue in redux-elm where we would need the first action (be that either redux-devtools init or redux init) dispatched with all the middlewares applied.

}
}

var finalCreateStore = enhancer ? enhancer(createStore) : createStore
Copy link
Contributor

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.

@gaearon
Copy link
Contributor

gaearon commented Apr 4, 2016

eg. redux-devtools will dispatch its own init function before redux/@@init and populates appstate beforehand

Can you point to where this happens? I don’t see it dispatching any initial actions in the source.

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.

If we go ahead with this, we’ll forbid dispatching while enhancers/middleware are being created, kinda like #1485 but more generic.

@gaearon gaearon added this to the 4.0 milestone Apr 4, 2016
@tomkis
Copy link
Contributor

tomkis commented Apr 4, 2016

Can you point to where this happens? I don’t see it dispatching any initial actions in the source.

https://github.com/gaearon/redux-devtools/blob/master/src/instrument.js#L151 recomputeStates will ensure calling that mutation beforehand, strictly speaking it does not dispatch the action but pre-populates app state which as i've explained for example won't work for us in redux-elm

If we go ahead with this, we’ll forbid dispatching while enhancers/middleware are being created, kinda like #1485 but more generic.

If that's acceptable then this may be a way to go.

@gaearon
Copy link
Contributor

gaearon commented Apr 4, 2016

https://github.com/gaearon/redux-devtools/blob/master/src/instrument.js#L151 recomputeStates will ensure calling that mutation beforehand, strictly speaking it does not dispatch the action but pre-populates app state which as i've explained for example won't work for us in redux-elm

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?

@tomkis
Copy link
Contributor

tomkis commented Apr 4, 2016

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 @@INIT, for user it should be transparent they should never be aware of the action, user should only know that there is some action which ensures proper initialization of app state.

In my opinion this is the only reason why @@INIT function should be executed with middlewares otherwise people should never rely on its presence. You said that we could forbid dispatching while store is being created, but this will solve only part of the problem, because the middleware can potentially provide non-undefined app state beforehand.

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.

@tomkis
Copy link
Contributor

tomkis commented Apr 4, 2016

Anyway, re-visited original problem and it looks like #465 (comment) justifies it. But still as @acdlite mentioned, it's questionable...

@gaearon
Copy link
Contributor

gaearon commented Apr 4, 2016

All about tradeoffs. Being slightly hacky in two or three projects is better in my view than forcing everyone to add a custom dispatch() call and update all tutorials and docs.

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 :-)

@tappleby
Copy link
Contributor Author

tappleby commented Apr 5, 2016

@gaearon what was the original reason for the INIT action?

@gaearon
Copy link
Contributor

gaearon commented Apr 5, 2016

@tappleby Not sure what you mean. What was its purpose in DevTools, or generally?

@tappleby
Copy link
Contributor Author

tappleby commented Apr 6, 2016

Meant generally. Was asking because I thought we might be able to do something like initialState = reducer(initialState, { type: ActionTypes.INIT }) but then realized thats pretty much the same thing as dispatching.

@gaearon
Copy link
Contributor

gaearon commented May 9, 2016

Closing in favor of #1702 but big thanks for providing the inspiration.

@gaearon gaearon closed this May 9, 2016
@timdorr timdorr removed this from the 4.0 milestone Nov 3, 2017
@markerikson markerikson mentioned this pull request Aug 4, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants