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

Support React-Redux V6 #27

Closed
winklerp opened this issue Nov 10, 2018 · 12 comments
Closed

Support React-Redux V6 #27

winklerp opened this issue Nov 10, 2018 · 12 comments
Assignees

Comments

@winklerp
Copy link

React-Redux v6 will use React's ( new ) context api which is not compatible with the legacy context API. By accessing store from context ( reference ) it causes the app to break when it tries to dynamic load modules.

"Could not load store from React context" - Error is thrown in DynamicModuleLoader.

As @timdoor has described in its breaking changes:

Passing store as a prop to a connected component is no longer supported. Instead, you may pass a custom context={MyContext} prop to both and . You may also pass {context : MyContext} as an option to connect.

@abettadapur abettadapur self-assigned this Nov 10, 2018
@abettadapur
Copy link
Collaborator

@winklerp Thanks I'll take a look

@abettadapur
Copy link
Collaborator

@winklerp Here, I think we can use the ReactReduxContext.Consumer that is exposed, and get the same behavior

@winklerp
Copy link
Author

Hi, ReactReduxContext.Consumer works. But since React-Redux 6 supports custom context I think we should pass the context into DynamicModuleLoader and not only rely hard coded on ReactReduxContext.

See release notes of react redux 6.0-beta:

• Passing store as a prop to a connected component is no longer supported. Instead, you may pass a custom context={MyContext} prop to both and . You may also pass {context : MyContext} as an option to connect.

With this it is possible to have several stores in different contexts. So this is why I think passing the context to module loader is necessary. By the way: if we pass a property named store, react redux throws an exception :). Only passing the context works.

@cschleiden
Copy link
Contributor

It's released now.

@abdurahmanus
Copy link

Is it ok that I get undefined when I try to access dynamic modules's state ("myModule") when I wrap Lazy component with connect function and use it inside DynamicModuleLoader component? Is't it supposed to have "myModule" to be loaded?

https://codesandbox.io/s/7200w810n6

@abettadapur
Copy link
Collaborator

abettadapur commented Jan 19, 2019

@navneet-g Could you chime in here? I am able to reproduce the problem reported above, but I am not sure why.

I see that the ReactRedux provider context passes store (which has the correct state) and storeState (which does not)

@navneet-g
Copy link
Contributor

@abdurahmanus This is really interesting ... I spent sometime in debugging but did not reach a conclusion, so far it is pointing in direction that it is outside scope of redux dynamic modules and something related to react and react-redux. For example see below screenshot, the _this2.setState actually calls mapStateToProps twice first with old state that does not have myModule state set and immediately after correct state.

image

@abdurahmanus
Copy link

@navneet-g it seems like this behavior was changed somewhere in between v5.1.1 and 6.0.0 of react-redux.
the same example with react-redux v5.1.1 works different way (no undefined value)
https://codesandbox.io/s/xj6l40kjqw
The question is what should be considered as correct behavior? If (while loading process) some module could be undefined it should have nullable type in *AwareState interface (if ts using) and should be checked for undefined value in mapStateToProps or anywere else.

@navneet-g
Copy link
Contributor

If this is a behavior change in react-redux, please open an issue there, we can take it forward then depending upon if they make changes.

@abdurahmanus
Copy link

I've also noticed that todo-example uses react-redux@6 and it's broken now
image

@navneet-g
Copy link
Contributor

I did further debugging and opened following issue in react-redux repo
reduxjs/react-redux#1194

@navneet-g
Copy link
Contributor

navneet-g commented Feb 18, 2019

This is due to a known change in react-redux with their support to new context API.
reduxjs/react-redux#1126

We need to update DynamicModuleLoader as follows https://codesandbox.io/s/pwmm8r3110
I probably won't have time, if someone wants to make change and send the PR, it would be awesome.

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

No branches or pull requests

5 participants