microsoft / redux-dynamic-modules

Modularize Redux by dynamically loading reducers and middlewares.
https://redux-dynamic-modules.js.org
MIT License
1.07k stars 116 forks source link

Support React-Redux V6 #27

Closed winklerp closed 5 years ago

winklerp commented 5 years ago

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 commented 5 years ago

@winklerp Thanks I'll take a look

abettadapur commented 5 years ago

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

winklerp commented 5 years ago

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 commented 5 years ago

It's released now.

abdurahmanus commented 5 years ago

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 commented 5 years ago

@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 commented 5 years ago

@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 commented 5 years ago

@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 commented 5 years ago

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 commented 5 years ago

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

navneet-g commented 5 years ago

I did further debugging and opened following issue in react-redux repo https://github.com/reduxjs/react-redux/issues/1194

navneet-g commented 5 years ago

This is due to a known change in react-redux with their support to new context API. https://github.com/reduxjs/react-redux/issues/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.