react-redux 6.0.0
Opened this issue · 12 comments
Just ran into an issue with the new way react-redux behaves.
You can see more details about it in this thread: reduxjs/react-redux#935 (comment)
As we can see from the last release of react-redux 6.0.0: https://github.com/reduxjs/react-redux/releases
Behavior Changes
Any library that attempts to access the store instance out of legacy context will break, because we now put the store state into a <Context.Provider> instead. Examples of this include connected-react-router and react-redux-subspace. (The current implementation does also put the store itself into that same context. While accessing the store in context is not part of our public API, we will still try to make it possible for other libraries to access it, with the understanding that this could break at any time.)
This makes the onLoad function not working properly. The fourth parameter context is no more populated with the store.
Although it is not a real issue for me, as I use redux-reducers-injector (I'm the maintainer), I have a reference of the store that I can use, and can use replaceReducer.
The real issue is that even with injecting my reducer correctly, the mapStateToProps will not reflect this change in its first parameter state and I won't be able to display my component correctly.
You can read more about this issue in the link I post above on the react-redux project: reduxjs/react-redux#935 (comment)
Feel free to ask me more information about this.
Any ideas on a quick fix? Seeing as you maintain a lib that solves these things. I know there’s an open or to Link for redux 6
I’m in the middle of supporting react static which is close to release, so I’m tied up for this week monitoring finalizations and ptching a big issue.
I’ll comms with you more soon as my RS stuff clears up by week end
Looking back to this issue, we can see the first problem has been fixed in reduxjs/react-redux#1126
Using an HOC that will override the ReactReduxContext provider including the last injected reducer make it work.
But the original problem is not fixed, the onLoad function is not working properly. The fourth parameter context is no more populated with the store.
We should update react-universal-component for not breaking with the new version of react-redux@6.0.0.
This store is finally mandatory in a SSR environment.
Not knowing the store in which we want to dynamically inject a reducer in a lazy loaded component will lead to store overrides when doing multiple calls.
Example : a service worker for cache purposes wants to precache several routes. It will trigger multiple calls in parallel to the server. All these calls made at the same time will overlap the current store being used. Using a reference to the one and only store works with a Single Page Application (SPA) not with Server Side Rendering (SSR) context where multiple store can be instantiated.
The only way to deal with it when injecting reducer in the store is to know the context store as before.
@ScriptedAlchemy Do you think, there is a way to reinclude the context store in the fourth parameter?
Feel free to read the reduxjs/react-redux#1126 issue about how to provide the ReactReduxContext to react-universal-component.
Sounds like a toughie, can you make a code example of how the end user would use it?
'
Álso thins might help
#156
Let me know, but i need to move RUC branch to react 16 and i know its stable. How’s the time to add any last things to it if you wish @GuillaumeCisco
Hit me up on slack if neeed be
Hey @ScriptedAlchemy, I spent yesterday afternoon on this issue and what it led me to think react-universal-component had a hidden feature that should not exist in the first place.
Looking at: https://github.com/faceyspacey/react-universal-component#api-and-options
onLoad is a callback function that receives the entire module. It allows you to export and put to use things other than your default component export, like reducers, sagas, etc. E.g:
onLoad: (module, info, props, context) => {
context.store.replaceReducer({ ...otherReducers, foo: module.fooReducer })
// if a route triggered component change, new reducers needs to reflect it
context.store.dispatch({ type: 'INIT_ACTION_FOR_ROUTE', payload: { param: props.param } })
}
As you can see we have thought of everything you might need to really do code-splitting right (we have real apps that use this stuff). onLoad is fired directly before the component is rendered so you can setup any reducers/etc it depends on. Unlike the onAfter prop, this option to the universal HOC is only fired the first time the module is received. Also note: it will fire on the server, so do if (!isServer) if you have to. But also keep in mind you will need to do things like replace reducers on both the server + client for the imported component that uses new reducers to render identically in both places.
Here the context is available. But what is awesome is that @faceyspacey allowed the store from a potential redux implementation available in the context. I think he did that for making things easier with redux-first-router. So in the first place, onLoad should not even have a store property in its context.
As of react-redux 6.0.0, we cannot get the store from the context by using contextTypes as done here:
https://github.com/faceyspacey/react-universal-component/blob/master/src/index.js#L111
As you can see in the example of the onLoad method, the context parameter is especially used for dealing with the context.store i.e replacing reducers, dispatch actions etc...
So I thought the easiest way was to simply add the ReactReduxContext component for providing the store directly.
Turning the onLoad example to:
import universal from 'react-universal-component';
import {ReactReduxContext} from 'react-redux';
const UniversalComponent = universal(props => import(`./${props.page}`), {
onLoad: (module, info, {context, ...props}) => { // modification for using context from a prop
context.store.replaceReducer({ ...otherReducers, foo: module.fooReducer })
// if a route triggered component change, new reducers needs to reflect it
context.store.dispatch({ type: 'INIT_ACTION_FOR_ROUTE', payload: { param: props.param } })
}
})
export default () =>
<div>
<ReactReduxContext.Consumer>
{(reduxContext) => <UniversalComponent page='Foo' context={reduxContext}/>}
</ReactReduxContext.Consumer>);
</div>Interestingly, it worked like a charm with one of my connected component which is lazy loaded (universal) and dynamically load a reducer (store.replaceReducer) with the new withRedux pattern defined as:
import React, {Component} from 'react';
import {ReactReduxContext} from 'react-redux';
export default function withRedux(WrappedComponent) {
class WithRedux extends Component {
constructor(...args) {
super(...args);
this.firstRender = true;
}
render() {
if (this.firstRender) {
this.firstRender = false;
return (
<ReactReduxContext.Consumer>
{(reduxContext) =>
<ReactReduxContext.Provider
value={{
...reduxContext,
storeState: reduxContext.store.getState(),
}}
>
<WrappedComponent {...this.props} />
</ReactReduxContext.Provider>
}
</ReactReduxContext.Consumer>
);
}
return <WrappedComponent {...this.props} />;
}
}
return WithRedux;
}Used as:
export default withRedux(connect(mapStateToProps)(MyComponent));But absolutely not for a second Component which seem to be build on the same architecture.
I'm investigating that today, and hope coming back with a solution ;)
Hello there, I succeeded to fix this issue, but another one appeared with react-hot-loader.
You can see why it was not working on this codesandbox:
https://codesandbox.io/s/5mk3564w0k
And here it is working:
https://codesandbox.io/s/vmwm085633
The difference leaves in the route.js file. One use the context props and the other the reduxcontext props.
Do not hesitate to open the console on the bottom of the codesandbox page for understanding what is going on.
We can see in the not working example this error:
Warning: React.createElement: type is invalid -- expected a string (for built-in components) or a class/function (for composite components) but got: undefined. You likely forgot to export your component from the file it's defined in, or you might have mixed up default and named imports.
Check the render method of `Connect(MyComponent)`.
in Connect(MyComponent) (created by Context.Consumer)
in WithRedux (created by UniversalComponent)
in UniversalComponent (created by Context.Consumer)
in _default (created by App)
in div (created by App)
in App
in Provider
This issue is now directly related to reduxjs/react-redux#1126.
I do not really understand why modifying the name of the props context to reduxcontext make it works. If someone can help :)
One more new issue appeared though :/
I'm using react-hot-loader, and now when I modify the component file, it leads to an infinite loop trying to rerender the component :/
Looks like having multiple consumer and a provider override make it fails :/
Very not something I'm enjoying :x
By the way, you can test modifying the working example in the codesandbox and you'll see the code is broken too after a codesandbox hot reload. Try clicking on the fill results button, then modify something in the render method of the component MyComponent, you'll see a codesandbox hot reload, then try to click on fill results button again, it won't work.
So I'm glad now I can use the context.store in the onLoad method of react-universal-component without modifying its codebase.
What I think we should try to think about now is this part of the code: https://github.com/faceyspacey/react-universal-component/blob/master/src/index.js#L110
static contextTypes = {
store: PropTypes.object,
report: PropTypes.func
}It does not work with react-redux@6.0.0 and in the first place, it should never has been available.
I'd love having @faceyspacey's point of view on this topic.
For now, the only recommendation I have for making react-universal-component and reducer injection working with multiple concurrent calls on the server is to change the onLoad documentation to:
import universal from 'react-universal-component';
import {ReactReduxContext} from 'react-redux';
const UniversalComponent = universal(props => import(`./${props.page}`), {
onLoad: (module, info, {reduxcontext, ...props}) => { // modification for using context from a prop
reduxcontext.store.replaceReducer({ ...otherReducers, foo: module.fooReducer })
// if a route triggered component change, new reducers needs to reflect it
reduxcontext.store.dispatch({ type: 'INIT_ACTION_FOR_ROUTE', payload: { param: props.param } })
}
})
export default () =>
<div>
<ReactReduxContext.Consumer>
{(reduxContext) => <UniversalComponent page='Foo' reduxcontext={reduxContext}/>}
</ReactReduxContext.Consumer>);
</div>And talking about the withRedux HOC.
Thoughts?
Dude, this is exactly what onload is for, you unraveled one of James secrets (i think)
there are a few hidden gems in RUC that we have not disclosed, however, with this one, it might be a good idea to document your implementations as you have done here.
This is a really long issue, could we have a call around this, my dyslexia makes it very hard to follow your discoveries on this whole thing.
If what you are saying is you found the gem in RUC and hot reloading is the only blocker, i know the maintainer of it personally and if you provide me a codesandbox - ill have him address it directly.
Sorry its been a busy day and so many words are moving around that i cannot fully get your needs here.
We can do a slack call tomorrow or later today if you wish?
Hello @ScriptedAlchemy I succeeded to make it works with react-hot-loader.
I described the whole thing in the react-redux github project here
I do not think we should update the code of react-universal-component for now.
But if people wants to use it with react-redux@6.0.0, we definitively need to write a documentation about how to use it with the onLoad method for getting the redux context.
What do you think?
The new implementation I come with is:
import React, {Component} from "react";
import universal from "react-universal-component";
import { injectReducer } from "redux-reducers-injector";
import { ReactReduxContext } from "react-redux";
class Universal extends Component {
constructor(props) {
super(props);
this.firstRender = true;
}
render() {
const UniversalComponent = universal(import("./component"), {
loading: <span>loading</span>,
onLoad: (module, info, { reduxcontext }) => {
if (reduxcontext && reduxcontext.store) {
injectReducer("api", module.reducer, false, reduxcontext.store);
}
}
if (this.firstRender) {
this.firstRender = false;
return (<ReactReduxContext.Consumer>
{(reduxContext) => <UniversalComponent reduxcontext={reduxContext}/>}
</ReactReduxContext.Consumer>);
}
return <UniversalComponent/>;
});
export default Universal;Can you open a pull request so I can better understand the changes proposed? Ideally on the React 16 branch. If it’s hot loading. I know the guy who built it so easy fix.
Can this be abstracted into the HOC to some level. Seems like a lot of boilerplate. Can’t this just be in RUC?
With facebooks suspense being built from merging in RUC, we will also be putting up documentation on that.
Make a PR ideally to RUC and I’ll look it over for merge
Hey @ScriptedAlchemy
I've just updated the README of redux-reducers-injector and redux-sagas-injector on how to use react-universal-component with react-redux@6.0.0.
I think we can transpose some main guidelines into the README of react-universal-component.
Most important thing is to warn user, the context argument is no more available from react-redux@6.0.0 and best thing is to use what I described using ReactReduxContext.
Maybe we should write a tutorial on how to use react-universal-component with dynamic injection when lazy loading.
I'd like to wait the point of view of other people interested with react-universal-component before.
Right now, there is absolutely nothing to change in the code of react-universal-component, we should only update documentation and create tutorials :)
If you would update the documentation on our end to point to those PRs or readmes that would be great information for us to have on hand!!
I use RR with RUC in this way:
class App extends Component {
render() {
return (
<ReactReduxContext.Consumer>
{props => {
return (
<Switcher store={props.store} /> /* or even {...props} */
)
}}
</ReactReduxContext.Consumer>
)
}
}
export default hot(module)(App)and somewhere in Switcher.js:
let innerStore
const UniversalComponent = universal(determineHowToLoad, {
//...
onLoad: (module, { isSync, isServer }, props, context) => {
const { store } = props
innerStore = store
}
}
const Switcher = ({ store, page, type }) => (
<UniversalComponent
component={`${page}`}
onAfter={onAfterChanged}
type={type}
store={store}
/>
)