fix(ComboBox): add missing dependencies to useEffect

[emyarod]

Closes #8306

This PR adds missing dependencies to the Combobox useEffect to avoid a render loop when the component is controlled and rerendered on input changes

Testing / Reviewing

Substitute the Combobox component in the example code sandbox with a built version of the component from this PR, and observe that removing the setTimeout in the example app no longer causes the component to render infinitely

[netlify]

Deploy preview for carbon-elements ready!

Built with commit dde99eb

https://deploy-preview-8308--carbon-elements.netlify.app

[netlify]

Deploy preview for carbon-components-react ready!

Built with commit dde99eb

https://deploy-preview-8308--carbon-components-react.netlify.app

[joshblack]

Would it be helpful if we added a useSavedCallback type hook for these situations? It seems like the effect is parameterized by inputValue and we wouldn't want to re-run this effect when onInputChange happens (especially if we're passed a function that's updated each render)

useSavedCallback could look like:

const savedOnInputChange = useSavedCallback(onInputChange);

// ...

useEffect(() => {
  const { current: onInputChange } = savedOnInputChange;
  if (onInputChange) {
    onInputChange(inputValue);
  }
}, [inputValue]);

And the implementation:

function useSavedCallback(callback) {
  const ref = useRef(callback);

  useEffect(() => {
    ref.current = callback;
  });

  return ref;
}

[emyarod]

I'm not sure if I see this optimization as strictly necessary in this case but it may be worth keeping in mind

[tay1orjones]

Would useCallback be enough to memoize onInputChange and avoid unnecessarily re-running the effect?

[joshblack]

@emyarod I think it's something that's helpful for us to do. In this case, the effect would synchronize with the input value changing and not the handler changing (although it would need that to correctly dispatch the update).

A totally extreme example of this idea would be if the parent component had a timer:

function Parent() {
  const [count, setCount] = useState(0);

  useEffect(() => {
    const intervalId = window.setInterval(() => {
      setCount((count) => count + 1);
    }, 1000);
    return () => {
      window.clearInterval(intervalId);
    };
  }, []);

  return (
    <>
      <p>The current count is: {count}</p>
      <ComboBox onChange={() => { /* ... */ }} />
    </>
  );
}

Because we have the effect synchronizing with the callback, we end up with the effect being called repeatedly with the same value. Instead, if the callback is saved and then invoked by an effect parameterized by inputValue it would only run when the value changes.

Here is a quick demo of this idea: https://codesandbox.io/s/vigorous-goldwasser-rp3fe?file=/src/App.js

Screen.Recording.2021-04-08.at.11.01.58.mov

[emyarod]

wouldn't saving the handler in a ref inside the parent function accomplish the same effect?

[joshblack]

I'm not sure, would need to explore. In this scenario, I'm imagining that the parent would be the person using Carbon so just was illustrating that splitting the effect out in this way would help out in the default case without them having to change how they author their code.

[emyarod]

something like this I assume https://codesandbox.io/s/pensive-sun-4ijti

given that the event handler is provided by the Carbon consumer, I am not sure about optimizing it for them by default (including useCallback usage as @tay1orjones suggested). although I think I am less hesitant about baking useCallback into our component if we are certain we want to optimize the handler no matter what

[tay1orjones]

I'm not opposed to optimizing it now to prevent problems down the line, though if it is an uncommon use case that the majority of consumers won't run into we can point them here (or other docs?) to instruct on how they can fix downstream in their application code.

Having this hook could be useful though, it would apply to any callback placed in a useEffect that has a dependency on one or more variables/props. I'm not sure how common that is in the codebase right now though.

[joshblack]

Definitely meant the counter example as the extreme case, the big thing is that in a default setup this implementation would:

• Have the effect and handler be called for non-related updates
  • The effect would be called because the identity changes for function declarations or arrow functions
• Have the handler be called twice in intended situations like if the parent is using the value from onChange to update state
  • State update triggers a render that then creates a new reference for a function declaration or arrow function, this then calls the effect to be run which is the point above

And it seems like introducing this convention would:

• Make sure the handler is only called when the value is changed and at no other time
• Make sure the handler is only called once per input change

[nertzy]

Would it be possible to have a first pass of a fix that only considers the inputValue changing and not the callback function?

As a consumer, I only expect onInputChange to be called when the input changes. If I pass a new function on every render, I would still only expect one call, to whatever the function is set to at the time the input change occurs.

In my application, onInputChange is getting called unconditionally on every render, even though both inputValue and the callback function are not changing. When I type in an unrelated separate text field, it triggers onInputChange on every combo box on the page. We essentially had to write our own condition in our callback to remember the previous value of the input and only continue if the input value actually changed, which feels like a waste of effort.

Thanks for looking at this so deeply. I hope that we can figure out a good solution for the long term.

[joshblack]

@nertzy would this thread address what you're describing or would it be another change? 👀 #8308 (comment)

[nertzy]

@nertzy would this thread address what you're describing or would it be another change? 👀 #8308 (comment)

If I understand it correctly, I think it would work for my use case. As long as [inputValue] is the second argument to the useEffect call, then my application won't receive a call to the callback on every render.

I don't know enough about the useSavedCallback concept to have any advice on whether that would be necessary.

[emyarod]

@joshblack @tay1orjones circling back on this, so is the expectation here to include a new useSavedCallback hook or can this fix go in first?

[joshblack]

@emyarod I don't think you need to do the hook explicitly, but I think saving the callback in a ref and calling it would be great for this PR and then we can get it in 👍 (Like in this pattern: https://github.com/carbon-design-system/carbon/blob/main/docs/style.md#hooks-that-use-a-callback)