redux-injectors: React 16.13 throws a warning when "nesting" useInjectReducer calls

Ok after much investigation I’ve found a few scenarios where this can happen:

1. Selectors that return a new object or array are not memoized
   • These include selectors that use .map and .filter
2. Redux devtools are installed.
   • Devtools has kind of a surprising behaviour. It intercepts the @@redux/REPLACE action, and re-dispatches previous actions. This is actually kind of concerning because it’s causing extra renders and could even cause different behaviour between people who have the extension and people who don’t
   • See: https://github.com/zalmoxisus/redux-devtools-extension/blob/master/docs/Troubleshooting.md#init-or-replace-action-resets-the-state-of-the-app-or-last-actions-re-applied
   • See: https://github.com/zalmoxisus/redux-devtools-extension/issues/205
   • See: https://github.com/zalmoxisus/redux-devtools-extension/issues/326

So we have two options:

1. Make no code changes and update the documentation to explain these reasons and how to fix them. 1 can be fixed by properly memoizing. 2 can be fixed by setting shouldHotReload to false in the dev tools options.
   • Setting shouldHotReload, however, causes another issue https://github.com/reduxjs/redux-devtools/issues/378
2. Use useLayoutEffect instead of injecting reducers synchronously on the first render.
   • This will mean sagas and reducers will inject before any other useEffects However, it will run after children’s useLayoutEffect. So if anyone is using dispatch inside a useLayoutEffect, it might not get picked up

Opinion I think we should do a combination of the above:

1. Mention in the docs that parts of the app may re-render unexpectedly when using useInjectReducer, and that pages should be able to handle that. For example, an order shouldn’t be placed twice because a component re-rendered. This is how you’re supposed to write react components anyway.
2. Change our injection mechanism to use useLayoutEffect instead of injecting during the render. This is because in truth replacing reducers is a side-effect, and may cause renders in other parts of the app for any number of reasons. As one example, you don’t have to use memoized selectors, and forcing people to do so is probably not good.
3. Make useInjectReducer return a boolean indicating if the reducer is injected. Consumers can use this value to determine whether to render children or not, which can help avoid race conditions when consumers are dispatching actions inside useLayoutEffect. This should be rare enough that it’s not used often.
4. Mention the useLayoutEffect race condition in our docs
5. Possibly introduce a new api called “createManager” that would avoid these race conditions by only rendering it’s children after the reducer/saga is injected
6. The one thing I’m not sure about is whether we should recommend setting shouldHotReload to false. If set to false users will suffer https://github.com/reduxjs/redux-devtools/issues/378 but if set to true you might get different behaviour between dev and production. I’ll try to come up with an example of this.

More resources: https://blog.logrocket.com/post-hooks-guide-react-call-order https://codepen.io/benlorantfy/pen/bGEJveX

Any update on this issue?

Small update: I’ve released a pre-release (2.0.0-rc) that fixes this issue. It’s not production ready, but if any volunteers want to test it with their apps, that would be helpful.

If we don’t find any unexpected issues we should be able to release a full release soon.

@BenLorantfy Wow, just confirmed redux-devtools was causing the error messages in my current situation. Nice catch… I’ve spent so long trying to figure out the root issue. Going to keep looking into this as well.

This is what the function ended up looking like:

const useInjectReducer = ({ key, reducer }) => {
  const store = useStore();

  const isInjected = React.useRef(false);

  if (!isInjected.current) {
    isInjected.current = true;
    setTimeout(() => {
      getInjectors(store).injectReducer(key, reducer);
    }, 0);
  }
};

@BenLorantfy - should have one up in ~15min

@joejordan this pattern is unfortunately not possible anymore without getting the warning here.

The reason this used to work was because the reducer was being injected during the render. Injecting a reducer is usually a side-effect, which is what react is warning about here.

You would have to re-structure the code in this case so useSelector is called within a child of FeatureAppLoader. You could also try the new createManager api which would not have this problem.

@danielrob your comment aligns with my understanding of this issue as well…

The main problem with implementing useInjectReducer with the useLayoutEffect implementation is that children can’t use dispatch inside a useLayoutEffect, or else the reducer might not be injected in time to catch it.

Children can use dispatch inside a useEffect though. All useLayoutEffects will run before all useEffects in a given tree as seen here

So my approach in https://github.com/react-boilerplate/redux-injectors/pull/22 is to change the implementation to useLayoutEffect but also have useInjectReducer return a boolean that indicates whether or not the reducer is injected. Parents can use this boolean to hide their children until the reducer is injected.

I’m not sure what else we can do 🤷‍♂️

The underlying warning is for scenarios when setState is called for a component during the rendering of a different component e.g. like so:

const Component = props => {
  props.setOtherComponentState();
}

Now, under the hood redux-injectors uses redux’s replaceReducer method.

When replaceReducer is called it fires an internal REPLACE action in order to populate the store state according to the new reducers (i.e. so that you immediately have access to the initial state of new reducers passed in)

Thus if you call replaceReducer in a component as it is mounting i.e.

const Component = () => {
   const store = useStore();
   store.replaceReducer(newReducer);
}

It causes the store state to change.

Finally the way that useSelector is implemented is with a listener on store state, such that whenever the store state changes it will call setState inside the component it is in if the associated selector returns a different result.

Thus, for all intents and purposes in any app with non-trivial state subscriptions using replaceReducer is equivalent to calling setState in other components (unless you are fiendishly vigilant about memoizing selectors).

One of the major constraints in all of this is that to access the store instance redux-injectors goes through the React context.

The end result is this key question:

• When is the first available appropriate time to call replaceReducer after I have access to the context in a component.

It seems that the HOC approach solves this problem by doing this in the constructor, but I’m not actually convinced this is appropriate? The React docs say to avoid side effects in the constructor.

The hooks approach doesn’t give us access to that constructor to be able to call replaceReducer before the render phase, thus the only available phase is the commit phase. The remaining issue is the ordering of the commit phase means that child components may run their effects which could rely on the injected reducers before the parent component that is supposed to inject the reducer.

I imagine this is what is leading @BenLorantfy to go down the path of “we either do this at the first possible moment in the commit phase, or simply render no children until such moment as the reducer has been injected”. It is also what @d-pollard is going for in their comment here .

I’m equally unclear on whether either of these options is any good. useEffectLayout somehow fundamentally scares me because at the end of the day, child components have already started mounting, yet I’m not sure not rendering children on the first render is sufficiently eloquent.

I don’t understand the react internals enough to know if this is a good idea. My suspicion is that this is just getting around the error and will probably be an issue in a future react version.

Note that this will likely start hard failing in future versions of React. Intentional or otherwise (we’ve had a lot of bugs with this pattern). So regardless, you might end up getting stuck on an old version. If it’s not possible to fix it, I’d recommend pinning to an older version of React.

https://github.com/facebook/react/issues/18178#issuecomment-592662192

I think we’ll probably end up changing this to useLayoutEffect, and increasing the major version just to be safe. But I’m trying to understand the issue further before making that change.

interesting, I can’t replicate in a fresh repo, so it’s gotta be a pattern in my repo causing it, will update when I have more info;

I do know that I can in fact fix the issue by editing the source of redux-injectors useInjectReducer to utilize useLayoutEffect with a state boolean and the error goes away

I think that solution can cause the reducer to not being injected before an action is dispatched, similar to this issue: https://github.com/react-boilerplate/react-boilerplate/pull/2757

@BenLorantfy I figure out that I was using @hot-loader/react-dom webpack alias that is not yet updated for the last react@16.13.1, and the warning is not helpful, so I disabled for a while.

resolve: {
    modules: ['node_modules', 'app'],
    extensions: ['.js', '.jsx', '.react.js'],
    alias: {
      moment$: path.resolve(process.cwd(), 'node_modules/moment/moment.js'),
      'styled-components': path.resolve(process.cwd(), 'node_modules/styled-components'),
      react: path.resolve(process.cwd(), 'node_modules/react'),
      'react-dom': path.resolve(process.cwd(), 'node_modules/@hot-loader/react-dom'),
      ...options.resolve.alias,
    },
  },

Now I got a better view of the problem in the stack trace.

I don’t know. Would that be an issue?

TBH, the more I think about it, the more I dislike this solution.

For now, in my team at least, I think we’re gonna use the HOC on ChildContainer instead of the hook (if that fixes the issue, haven’t tried it yet).