Downshift: Add useCallback to all prop getters and actions

[kentcdodds]

If I identify a performance issue and want to memoize my components that use prop getters (like getItemProps), the prop getters are breaking memoization:

function Item({isHighlighted, getItemProps, item, index}) {
  return (
    <li
      style={isHighlighted ? {backgroundColor: '#bde4ff'} : {}}
      key={`${item}${index}`}
      {...getItemProps({item, index})}
    >
      {item}
    </li>
  )
}
Item = React.memo(Item)

https://codesandbox.io/s/usecombobox-usage-1fs67?file=/src/index.js:431-454

If you pull up the React DevTools and run a profile, you'll observe that Items are getting re-rendered because getItemProps changed:

The solution should be pretty straightforward. Simply apply useCallback to all prop getters and action functions. So long as all dependencies are included (is eslint-plugin-react-hooks installed on this repo?) then it should be a patch version update.

[kentcdodds]

To be clear, all of these should be memoized via useCallback:

https://github.com/downshift-js/downshift/blob/007db7bc8ac62c5774d55b2808bcdd6bf62f487a/src/hooks/useCombobox/index.js#L478-L492

[kentcdodds]

Could probably use the same treatment in all the hooks:

https://github.com/downshift-js/downshift/blob/007db7bc8ac62c5774d55b2808bcdd6bf62f487a/src/hooks/useSelect/index.js#L493-L505

https://github.com/downshift-js/downshift/blob/007db7bc8ac62c5774d55b2808bcdd6bf62f487a/src/hooks/useMultipleSelection/index.js#L236-L242

[kentcdodds]

Note, personally I wouldn't do the useCallback call in the code I linked

I would do it like this:

https://github.com/downshift-js/downshift/blob/007db7bc8ac62c5774d55b2808bcdd6bf62f487a/src/hooks/useCombobox/index.js#L295-L338

const getItemProps = React.useCallback(({
  item,
  index,
  refKey = 'ref',
  ref,
  onMouseMove,
  onClick,
  onPress,
  ...rest
} = {}) => {
  // ... contents of the file
}, [/* whatever the deps are goes here */])

[kentcdodds]

Actually, I just realized that this line will break memoization for getItemProps specifically:

https://github.com/downshift-js/downshift/blob/007db7bc8ac62c5774d55b2808bcdd6bf62f487a/src/hooks/useCombobox/index.js#L326

On every highlightedIndex change, all the Item components will get re-rendered because getItemProps will be new even though the props returned won't actually change for any but two items (the old highlighted one and the new highlighted one).

This can be side-stepped via useRef so we only reference the .current version of the highlightedIndex.

[kentcdodds]

Also, this will break memoization of all items when items changes:

https://github.com/downshift-js/downshift/blob/007db7bc8ac62c5774d55b2808bcdd6bf62f487a/src/hooks/useCombobox/index.js#L305-L308

We could require that people pass the index to avoid this. That's what the Downshift component did (and it didn't need users to pass the items at all).

[kentcdodds]

I'm going to go ahead and work on this. And I'm going to live stream it! https://youtu.be/UG3B5wjPuG0

[kentcdodds]

Got it done! :) #1051

[silviuaavram]

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

The release is available on:

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

Your semantic-release bot :package::rocket:

[kentcdodds]

Thanks!