Downshift: useSelect `cannot update during an existing state transition` warning using onSelectItemChange with react-router push

[TLadd]

• downshift version: 4.0.1
• node version: 10.16.3
• npm (or yarn) version: yarn 1.21.1

Relevant code or config

import React from "react";
import * as ReactDOM from "react-dom";
import {
  BrowserRouter as Router,
  Switch,
  Route,
  useHistory
} from "react-router-dom";
import { useSelect } from "downshift";

function App() {
  return (
    <Router>
      <div>
        <NavSelect />
        <Switch>
          <Route path="/about">
            <About />
          </Route>
          <Route path="/users">
            <Users />
          </Route>
          <Route path="/">
            <Home />
          </Route>
        </Switch>
      </div>
    </Router>
  );
}

function NavSelect() {
  const { push } = useHistory();
  const items = ["/about", "/users", "/"];
  const {
    getToggleButtonProps,
    isOpen,
    getMenuProps,
    getItemProps
  } = useSelect({
    onSelectedItemChange: changes => {
      push(changes.selectedItem);
    },
    items
  });
  return (
    <>
      <button type="button" {...getToggleButtonProps()}>
        Toggle
      </button>
      <ul {...getMenuProps()}>
        {isOpen &&
          items.map((item, index) => {
            return (
              <li key={item} {...getItemProps({ index })}>
                {item}
              </li>
            );
          })}
      </ul>
    </>
  );
}

function Home() {
  return <h2>Home</h2>;
}

function About() {
  return <h2>About</h2>;
}

function Users() {
  return <h2>Users</h2>;
}

const rootElement = document.getElementById("root");
ReactDOM.render(<App />, rootElement);

What you did:

I'm using useSelect with an onSelectItemChange callback that calls react-router's push function to change the route.

What happened:

The push works, but I end up with this warning in the console:

Warning: Cannot update during an existing state transition (such as within `render`). Render methods should be a pure function of props and state. 
    in NavSelect (at src/​index.js:15)
    in App (at src/​index.js:78)

Reproduction repository:

https://codesandbox.io/s/hopeful-currying-hvx2p

Problem description:

There seems to be some specific incompatibility between where onSelectITemChange is called and how push works. I don't quite understand it yet. push calls history's setState (not React's), which calls notifyListeners which calls react-router's history listener.

Suggested solution:

Unsure.

    onSelectedItemChange: changes => {
      setTimeout(() => {
        push(changes.selectedItem);
      });
    },

works as a workaround.

[TLadd]

Created another CodeSandbox that results in this warning and has the same basic scenario that is taking place between react-router-dom and downshift.

import React from "react";
import ReactDOM from "react-dom";

import "./styles.css";

// stand-in for history.listen
let listener = () => {};
// stand-in for history location
let count = 0;
// stand-in for `push`
const incCount = () => {
  count++;
  listener();
};

function App() {
  // stand-in for useEnhancedReducer
  const [state, dispatch] = React.useReducer(
    (state, action) => {
      switch (action.type) {
        default:
          incCount();
          return { flag: !state.flag };
      }
    },
    { flag: true }
  );

  return (
    <div className="App">
      <ComponentWithListenerSetInConstructor>
        {({ countInState }) => {
          return (
            <div>
              <h1>Hello CodeSandbox</h1>
              <h2>Start editing to see some magic happen!</h2>
              <button
                onClick={() => {
                  dispatch({ type: "GREAT" });
                }}
                type="button"
              >
                Click
              </button>
              {countInState}
              {String(state.flag)}
            </div>
          );
        }}
      </ComponentWithListenerSetInConstructor>
    </div>
  );
}

class ComponentWithListenerSetInConstructor extends React.Component {
  constructor(props) {
    super(props);

    this.state = {
      count: count
    };

    listener = () => {
      this.setState({
        count: count
      });
    };
  }

  render() {
    return <>{this.props.children({ countInState: this.state.count })}</>;
  }
}

const rootElement = document.getElementById("root");
ReactDOM.render(<App />, rootElement);

In this example, the React.useReducer is acting similarly to useEnhancedReducer; it calls incCount, which is equivalent to useEnhancedReducer calling push through onSelectedItemChange. ComponentWithListenerSetInConstructor is acting like the Router component of react-router-dom, setting up the listener and syncing up state whenever the listener is called.

[silviuaavram]

Should we follow up with issues on react / react-router-dom? I don't know what's the issue so we may need some help.

[TLadd]

Added an issue in react-router. Will see what they say and proceed accordingly, perhaps filing an issue in React.

[TLadd]

Issue in react-router was closed. From timdorr's explanation, it seems like window.history can be changed during a render cycle, but that's just creating an opportunity for something else to be calling it at the wrong time.

So the onSelectedItemChange, which is calling push in this case, gets called as a result of callOnChangeProps in useEnhancedReducer.

When first looking at useEnhancedReducer, I found it to be a bit non-standard, since it's calling callbacks inside a reducer. Just a hunch, but it sticks out to me as something that could be unsafe. I would typically expect useReducer to just handle the state updates and then a separate useEffect to detect changes in state and call the appropriate callbacks. I'm guessing there was a reason to go with the useEnhancedReducer approach as opposed to a separate useEffect?

I think useReducer does technically run during the render phase, so the warning message would make sense. If that is true, what I don't understand is why the warning doesn't appear if you do something like this:

export default function App() {
  const [otherState, setOtherState] = React.useState(0);
  const reducer = React.useCallback(
    (state, action) => {
      if (action.type === "CALL_SET_STATE") {
        setOtherState(otherState + 1);
      }
      return state;
    },
    [otherState]
  );
  const [, dispatch] = React.useReducer(reducer, { great: 4 });
  return (
    <button onClick={() => dispatch({ type: "CALL_SET_STATE" })} type="button">
      Click
    </button>
  );
}

I'm guessing React is able to handle this case somehow whereas the window indirection takes it out of React's hands in the push case.

[silviuaavram]

useEnhancedReducer is like that as we had to properly handle the controlled prop scenarios inside the useReducer logic. PR is https://github.com/downshift-js/downshift/pull/774 There is an issue attached as well.

I don't know React that well to fix this at the moment, but if you have an idea then we can start looking into it. Thanks!

[federico-qadra]

I have the same problem but using useCombobox and Formik.

I have a Combobox component in my app that does this.

const combobox = useCombobox({
  onSelectedItemChange: changes => {
    if (changes.itemSelected) onChange(itemSelected)
  }
)

Then in the render prop of Formik

<Combobox
  onChange={item => {
    if (item) setFieldValue('city', item.city)
  }}
/> 

Using the same component outside of Formik works fine.

[silviuaavram]

Can someone look into this please?

[cannatrade]

Same problem when using useCombobox and react-instantsearch-dom

[silviuaavram]

We should take a look at the useEnhancedReducer and see how it can be
fixed, maybe move the call of callbacks somewhere else. If someone wants to
take a look at it great, I’m on a small vacay atm. 🙂

On Thu, 23 Jan 2020 at 23:14, Jeremiah Smith [e-mail]
wrote:

Same problem when using useCombobox and react-instantsearch-dom

—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
https://github.com/downshift-js/downshift/issues/874?email_source=notifications&email_token=ACWAZABNDA6FPOAOWTDIODTQ7IJE3A5CNFSM4KCSQDC2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEJZBRVQ#issuecomment-577902806,
or unsubscribe
https://github.com/notifications/unsubscribe-auth/ACWAZAD2FKDJHPYVZAH5XUTQ7IJE3ANCNFSM4KCSQDCQ
.

[mufasa71]

@silviuavram I prepared pull request and tested it with react-router-dom and react-instantsearch-dom, works good as for me.

[silviuaavram]

Awesome. I will release an alpha to check the sandbox with the issue. A sandbox with react-instantsearch-dom would be useful as well.

[mufasa71]

https://codesandbox.io/s/busy-wilson-exk8t

I prepared sandbox, thanks

[silviuaavram]

:tada: This issue has been resolved in version 4.0.8 :tada:

The release is available on:

• npm package (@latest dist-tag)
• GitHub release

Your semantic-release bot :package::rocket:

[denchen]

Just for clarification, this is still an issue since #912 was reverted, correct? I'm seeing these warnings on v5.0.0, which I assume has #930. Should this be re-opened?

[silviuaavram]

I thought this was re-opened, thanks for pointing that out.

I tried a small change from the original PR https://github.com/downshift-js/downshift/pull/952.

@TLadd @mufasa71 can you take a look? In the controlled docs example for me it seems to work. Maybe you can help me test this out. Thanks!

[mufasa71]

@silviuavram i will look into it

[silviuaavram]

Will continue to track this issue at https://github.com/downshift-js/downshift/issues/962 and will attempt to fix it.