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

[BenLorantfy]

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: zalmoxisus/redux-devtools-extension#205
   • See: zalmoxisus/redux-devtools-extension#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 reduxjs/redux-devtools#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 reduxjs/redux-devtools#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

[mdaffan]

Any update on this issue?

[d-pollard]

@BenLorantfy - should have one up in ~15min

[d-pollard]

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);
  }
};

[chasekaylee]

@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.

[BenLorantfy]

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.

[julienben]

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

[vrbarros]

@BenLorantfy I figure out that I was using @hot-loader/react-dom webpack alias that is not yet updated for the last [email protected], 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.

[BenLorantfy]

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

[d-pollard]

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

[BenLorantfy]

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.

facebook/react#18178 (comment)

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.

[danielrob]

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.

[BenLorantfy]

@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 #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 🤷‍♂️

[BenLorantfy]

@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.

[julienben]

It's a hack and probably not a good idea to use it long term but might it be possible to fix this with useLayoutEffect? It should guarantee that the injection happens before any other effects.

[BenLorantfy]

Might be our best option... will it run before other useLayoutEffects though?

[vrbarros]

Hi, anyone found a solution for this warning? It become very annoying this errors in the console.

[BenLorantfy]

@julienben @vrbarros Do either of you have a short example you could share with me? I'm having trouble re-producing.

I tried this code and it doesn't log a warning:

const ChildComponent = () => {
  useInjectReducer({ key: "books", reducer: booksReducer })
  return null;
}

const ParentComponent = () => {
  useInjectReducer({ key: "users", reducer: usersReducer })
  return (
    <React.Fragment>
      <ChildComponent myProp={{}} />
    </React.Fragment>
  );
}

render(
  <Provider store={store}>
    <ParentComponent />
  </Provider>
);

[julienben]

That sample looks right to me... Are you on React v16.13.0 or 16.13.1? They reverted the warning in certain cases in the last version. More here.

[vrbarros]

Here I'm using React v16.13.1 and still getting a lot of errors.

Here I have one example from a container that is throwing this error when it is inside of other container.

/**
 *
 * ChangeContextMenu
 *
 */
import React, { useEffect, useState } from 'react';
import { FormattedMessage } from 'react-intl';
import { useSelector, useDispatch } from 'react-redux';
import { useInjectReducer, useInjectSaga } from 'redux-injectors';
import { createStructuredSelector } from 'reselect';

import { Mui } from '../../../components';
import { MenuItem } from '../../../components/widgets/MenuSidebar';
import * as actions from './actions';
import messages from './messages';
import reducer from './reducer';
import saga from './saga';
import makeSelectChangeContextMenu from './selectors';
import { useUser } from '../../common/UserProvider';

const stateSelector = createStructuredSelector({
  changeContextMenu: makeSelectChangeContextMenu(),
});

export function ChangeContextMenu() {
  useInjectReducer({ key: 'changeContextMenu', reducer });
  useInjectSaga({ key: 'changeContextMenu', saga });

  const dispatch = useDispatch();
  const { isAuthenticated, push } = useUser();
  const { changeContextMenu } = useSelector(stateSelector);

  const { isStaff = false, isSeller = false, isInstitution = false } = changeContextMenu;

  useEffect(() => {
    if (isAuthenticated) dispatch(actions.retrieveRequest());
  }, [isAuthenticated, dispatch]);

  const [anchorEl, setAnchorEl] = useState(null);

  function handleClick(event) {
    setAnchorEl(event.currentTarget);
  }

  function handleClose(to) {
    setAnchorEl(null);
    if (to) push(to);
  }

  if (!isAuthenticated) return null;

  const items = [
    {
      key: 'institution',
      icon: <Mui.Icon>school_outlined</Mui.Icon>,
      label: <FormattedMessage {...messages.institutionLabel} />,
      visible: isInstitution || isStaff,
      onClick: () => handleClose('/institutions'),
    },
    {
      key: 'seller',
      icon: <Mui.Icon>store_outlined</Mui.Icon>,
      label: <FormattedMessage {...messages.sellerLabel} />,
      visible: isSeller || isStaff,
      onClick: () => handleClose('/sellers'),
    },
    {
      key: 'family',
      icon: <Mui.Icon>house_outlined</Mui.Icon>,
      label: <FormattedMessage {...messages.familyLabel} />,
      visible: true,
      onClick: () => handleClose('/families'),
    },
  ];

  if (isStaff || isSeller || isInstitution) {
    return (
      <div>
        <Mui.Grow in timeout={3600}>
          <Mui.IconButton
            role="presentation"
            aria-controls="sellers-menu"
            aria-haspopup="true"
            onClick={handleClick}>
            <Mui.Icon>apps</Mui.Icon>
          </Mui.IconButton>
        </Mui.Grow>
        <Mui.Menu
          id="sellers-menu"
          anchorEl={anchorEl}
          keepMounted
          open={Boolean(anchorEl)}
          onClose={() => handleClose()}>
          {items
            .filter(item => item.visible === true)
            .map((item, index) => (
              <MenuItem {...item} key={item.key}>
                {index !== items.length - 1 && <Mui.Divider />}
              </MenuItem>
            ))}
        </Mui.Menu>
      </div>
    );
  }

  return null;
}

ChangeContextMenu.propTypes = {};

export default ChangeContextMenu;

When I remove the useInjectReducer({ key: 'changeContextMenu', reducer }); the error disappear.

[BenLorantfy]

So I've done some digging and realized this warning can happen when selectors don't return the same value (when compared referentially) when the top-level state changes.

For example, this selector will always produce a new referential value when called:

const selectBooks = (state) => state.books.map((book) => ({ ...book, rating: 5 })

However, this selector would be fine:

const selectBookState = createSelector(
  (s) => s.books,
  (books) => books.map((book) => ({ ...book, rating: 5 })
);

This selector would also cause the warning (if using toJS from immutable.js):

const selectBookState = createSelector(
  (s) => s.books.toJS(),
  (books) => books.map((book) => ({ ...book, rating: 5 })
);

The reason is because:

1. useInjectReducer calls store.replaceReducer
2. store.replaceReducer dispatches the init action: { type: @@INIT } and adds a new entry to the top-level state
3. The top-level state is now referentially different, which causes all selectors to be called with the new state
4. If any of these selectors return a referentially different value, components will re-render (because of a referentially equal check inside useSelector), and cause this warning

@julienben @vrbarros Any chance one of your selectors is doing this?

[vrbarros]

@BenLorantfy my selectors follow the same standard:

import { createSelector } from 'reselect';
import { initialState } from './reducer';

/**
 * Direct selector to the changeContextMenu state domain
 */

const selectChangeContextMenuDomain = state => state.changeContextMenu || initialState;

/**
 * Other specific selectors
 */

/**
 * Default selector used by ChangeContextMenu
 */

const makeSelectChangeContextMenu = () =>
  createSelector(selectChangeContextMenuDomain, substate => substate);

export default makeSelectChangeContextMenu;
export { selectChangeContextMenuDomain };

[BenLorantfy]

@vrbarros That example looks like it should be fine. The only way I can re-produce this warning is with a selector that returns a new value when the top-level state changes. I wonder if there's any selectors in your app that do this?

It would be helpful if possible if you could link to a repository that I could debug further. It's hard to debug without all the files (reducer, selectors, etc.)

[vrbarros]

@BenLorantfy unfortunately not, all the selectors follows the same pattern here, and I'm getting this warning for all the containers that uses useInjectReducer in the app. I will try to replicate and share with you.

[vrbarros]

@BenLorantfy I found this PR, but I didn't check yet if this solution works.

[d-pollard]

bump as this is still an issue; will try and debug myself and see if I can come up with a solution

[BenLorantfy]

@d-pollard If you could create a github repo that has the issue and share it with me that would be very helpful. It's hard to reproduce this otherwise.

[d-pollard]

@BenLorantfy - it looks like it's related to async actions; If I don't attach the sagas, it mounts perfectly fine, but once I mount the sagas, it throws the aforementioned error.

I wonder if parent-state updating is causing an issue; seems like it could possibly be an attempt to inject a new reducer while an existing reducer is actively updating its state

[d-pollard]

@BenLorantfy - here's your working error

repo: https://github.com/d-pollard/inject-reducer-error-poc

to see the issue, run the app then visit: http://localhost:3000/the-issue

[mdaffan]

I have this issue too on all the components that use the hooks of redux-injectors

[d-pollard]

@BenLorantfy - if it helps any; using setTimeout to mount the reducers fixes the hook implementation

[mdaffan]

@d-pollard Won't that affect actions, For example, actions dispatched even before the reducer mounts

[d-pollard]

It didn't mess with my actions that fired; would need to do more extensive testing though

[mdaffan]

Okay, Did you keep any timeout though like a second or 500ms or just a 0 did the job?

[d-pollard]

0 did the job

[d-pollard]

essentially acts like a "nextTick"

[mdaffan]

Yeah, right. This could actually do the job until the package itself gets updated with the solution!

[mdaffan]

@julienben @BenLorantfy If the above solution works just fine, Can you please update the package with this solution?

[vrbarros]

@BenLorantfy I agree with you, I'm in the same situation and I need to refactor parts of my code related to the hooks pattern that are still not 100% clear to me.

[chasekaylee]

Yeah I've been getting the same error

Warning: Cannot update a component (ConnectFunction) while rendering a different component (CreateOrderPage). To locate the bad setState() call inside CreateOrderPage, follow the stack trace as described in https://fb.me/setstate-in-render

Running react version 16.13.1

[mdaffan]

Any update on the fix? @BenLorantfy @julienben

[martinezjose]

@mdaffan, we have created a StateManager component that injects all the application's reducers and sagas in the meantime; we render the component as part of the App render. Not sure if this might be an okay solution for now.

[mdaffan]

Can you show an example implementation of how you are doing it @martinezjose ?

[BenLorantfy]

I was thinking we could introduce a new api like this:

const BooksManager = createManager({
  reducers: [booksReducer],
  sagas: [booksSaga]
});

And then used like this:

<BooksManager>
  ...
</BooksManager>

Internally, BooksManager would use useInjectReducer and useInjectSaga but only render its children after useInjectReducer and useInjectSaga have returned true, indicating the reducer/saga is injected.

This causes a double-render (one render with BooksManager rendering null and one render with BooksManager rendering its children) , but I don't think would have a big performance impact.

[danielrob]

Yes, tackling this in my use case, I just ended up opting for an HOC that null's one render. It just seems the safest option with the minuscule downside of a null render as you say:

const createInjectDomain = ({ key, reducer, saga }) => (
  Component
) => (props) => {
  const store = useStore();
  const [injected, setInjected] = useState(Boolean(store.asyncReducers[key]));

  useEffect(() => {
    if (!injected) {
      store.injectReducer(key, reducer);
      store.injectSaga(key, saga);
      setInjected(true);
    }
  }, []);

  return injected ? <Component {...props} /> : null;
};

export { createInjectDomain };

Which is along the same lines of what you are suggesting with createManager. Providing booleans from the injector hooks would allow people to do this similarly in a once-off DIY wrapper component fashion.

[lid3rs]

Any updates?

[BenLorantfy]

@mdaffan @lid3rs I'm waiting for review of this PR: #22

[joejordan]

@BenLorantfy at this point I would consider forking the project and creating a new npm package. Redux-injectors is very useful for code splitting reducers, and it is not at all dependent on React Boilerplate, which is no longer being maintained.

[Scott-HangarA]

Honestly is code splitting reducers worth trying to dig through all this? When it comes down to it, reducers and sagas are already easy to split using simple combined reducers and a root saga. What does this actually get besides dynamically injecting the sagas as needed? Is there a real performance improvement or any incentive beyond "it looks nice"?

[BenLorantfy]

@Scott-HangarA The code-splitting will definitely decrease bundle size, especially in a large application.

To be fair I haven't measured the performance impacts. So we should probably measure that before declaring there is any significant improvement.

However, this library is not just about performance or code-splitting. It's also about enabling a more modular architecture where you don't need to list all your reducers or sagas in one spot. This allows you to create libraries of encapsulated containers which leads to a loosely coupled architecture.

I hope you don't mind but I'm also going to mark this thread as off topic.

[Scott-HangarA]

@BenLorantfy While I agree there are definitely benefits, the main question I have is .. will this end up working without throwing errors, and with that in mind is it worth pursuing implementing this into a market product. I've seen some people say to fork the project to implement a "rough" solution, some saying there isn't a good way forward in using hooks, and that's all fine and I love that people are working towards a solution but I also can't have errors being thrown in a production application. This has been a problem for over a year now and I don't really see a solution put forward in this thread so I guess what I'm asking is, is there a plan to maintain this?

Edit: Also I do see there is an open PR to resolve this issue, but within that PR there are discussions about whether that is the right approach.

[BenLorantfy]

@Scott-HangarA Most open source development is slow and based on volunteered time. There's a pull request here that I recently pushed some more commits to: #22

With this PR, hooks can be used without errors if you understand the caveats but I'm proposing a new API: createManager that avoids these caveats.

Hopefully this gets merged soon.

[joejordan]

@BenLorantfy I attempted migrating our (rather large) frontend project to the 2.0.0-rc release. Unfortunately it looks like some of the patterns we were using are no longer working. For example, every feature page is set up with a "loader" type of component that makes the network calls and checks for any errors on them before rendering the feature page with the retrieved data. At the top of these loader components, we typically inject our reducers and sagas here, and then attempt to extract from them via a useSelector after.

function FeatureAppLoader(props) {
  useInjectReducer({ key, reducer });
  useInjectSaga({ key, saga });
  const isLoaded = useSelector(({ featureApp }) => featureApp?.isLoaded);
...

Currently I get an undefined error from useSelector on the first pass-through. Is there any way to retain the original functionality where the defaults on the reducer are immediately available upon injection?

[BenLorantfy]

This was fixed in #28