Downshift: Infinite loop when setting state in `onStateChange`

[TLadd]

• downshift version: 4.0.8
• node version: 10.16.3
• npm (or yarn) version: yarn v1.21.1

Relevant code or config

What you did:

I passed onStateChange into useSelect.

import React from "react";
import { render } from "react-dom";
import { useSelect } from "downshift";
import { items, menuStyles } from "./utils";

function DropdownSelect() {
  const [counter, setCounter] = React.useState(0);

  const {
    isOpen,
    selectedItem,
    getToggleButtonProps,
    getLabelProps,
    getMenuProps,
    highlightedIndex,
    getItemProps
  } = useSelect({
    items,
    onStateChange: () => {
      setCounter(prev => prev + 1);
    }
  });
  return (
    <div>
      {counter}

      <label {...getLabelProps()}>Choose an element:</label>
      <button {...getToggleButtonProps()}>{selectedItem || "Elements"}</button>
      <ul {...getMenuProps()} style={menuStyles}>
        {isOpen &&
          items.map((item, index) => (
            <li
              style={
                highlightedIndex === index ? { backgroundColor: "#bde4ff" } : {}
              }
              key={`${item}${index}`}
              {...getItemProps({ item, index })}
            >
              {item}
            </li>
          ))}
      </ul>
      {/* if you Tab from menu, focus goes on button, and it shouldn't. only happens here. */}
      <div tabIndex="0" />
    </div>
  );
}

render(<DropdownSelect />, document.getElementById("root"));

What happened:

onStateChange gets called in an infinite loop

Reproduction repository:
https://codesandbox.io/s/useselect-usage-slt20

Problem description:
Passing in onStateChange to useSelect resulst in an infinite render loop. Related to the changes in https://github.com/downshift-js/downshift/pull/912. The issue is that the props in the useEffect's dependency array is never going to be referentially equal to the previous version. So onStateChange will be called every time a render occurs. And if onStateChange triggers another render (which it would for a lot of typical use cases), we end up in a loop.

Suggested solution:
I believe the check

  if (props.onStateChange && changes !== undefined && changes !== state) {
    props.onStateChange(changes);
  }

in callOnChangeProps would prevent the infinite loop because we do not call onStateChange if the state didn't actually change.

[silviuaavram]

I am looking at

  useEffect(() => {
    if (prevState.current) {
      callOnChangeProps(props, prevState.current, state)
    }
    prevState.current = state
  }, [state, props])

and I don't know if we need to add props to the array. Will that make sense and fix this issue?

[mufasa71]

Removing props from the array should fix this issue, but i would rather add check in:

 if (prevState.current && prevState.current !== state) {
      callOnChangeProps(props, prevState.current, state)
 }

[TLadd]

Yeah, I guess any of those changes would also work. I thought removing props would be bad since it is used in the effect and I thought there might be a danger of having a stale version of a callback passed in. However, since we don't want to call anything unless the state changes, it would work out because it would get the updated props the next time the state changes.

So all three proposed solutions are basically adding a check for previous state and state being equal; just a question if we want it done in callOnChangeProps, at the call site of callOnChangeProps, or in the effect that triggers `callOnChangePros.

I think I like @mufasa71's suggestion the best because

• Even though removing props from deps works, it's generally a best practice to include everything in the deps array that is utilized within an effect (recommendation from React team)

• The check is more explicit. I could see a future where someone looks at props not being in the array and think it's a mistake whereas the prevState.current !== state check is explicit and easier to tell what's going on.

• Doing it at the call site prevents needlessly checking each key of state for changes.

Going to update my PR to @mufasa71's suggestion.

[silviuaavram]

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

The release is available on:

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

Your semantic-release bot :package::rocket: