fix(hooks): prevent overwrite of itemRefs when hooks rerender

[jorgemoya]

What:
Fixes #1178.

Why:
When memoizing the components, the refs would be reset on the hook rerender, and since not all items call the getItemProps prop getter, the itemRefs object would be incomplete, causing issues when keyboard navigating.

How:
Initialized the refs with a value so that the value isn't being overwritten. Testing locally I didn't see any issues with this (useSelect and useCombobox), on both memoized and non memozied components. Tests passed.

Checklist:

• Documentation N/A
• Tests
• TypeScript Types N/A
• Flow Types N/A
• Ready to be merged READ BELOW
  

When trying to commit I kept getting this error:

Error: package.json » ./node_modules/kcd-scripts/eslint.js » /Users/jorge.moya/dev/downshift/node_modules/kcd-scripts/node_modules/eslint-config-kentcdodds/jest.js:
        Environment key "jest/globals" is unknown

I had to --no-verify to be able to open the PR, I haven't been able to figure out why it is doing that. Found this issue 🤷

[jorgemoya]

I'm a little cautious because I don't know the reason of why the refs were being initialized this way.

[jorgemoya]

Another solution would be adding this (instead of the proposed changes) so that it resets if items change.

  // Make initial ref an empty object.
  useEffect(() => {
    itemsRef.current = {}
  }, [items])

[silviuaavram]

Hi! Thanks for looking into this!

Maybe resetting the itemRefs when isOpen changes and becomes false? Would that make sense?

In any case these changes will need testing.

[jorgemoya]

Maybe resetting the itemRefs when isOpen changes and becomes false? Would that make sense?

We can do that but to me it makes more sense to reset when items change. What do you think?

In any case these changes will need testing.

Added tests that verifies my fix works on memoized components.

[silviuaavram]

I think it makes more sense to clear the refs on isOpen as false, since the items elements will not exist at that point.

Also I'd make a separate test component with the memoized components. And use that for memo-related tests. And leave the previous test component as it is.

[jorgemoya]

@silviuaavram Added the effect and updated the test logic per your comments. For some reason I'm getting some validation hangups, though.

[silviuaavram]

What validation hangups?

I see that there is coverage missing from the new code so the build fails.

I am thinking about this recently and about #1176

I am very inclined to go ahead with having a isItemDisabled prop instead of the whole disabled on each item API. It's what react-select also does, and it will also fix selection with select closed. Right now there is no way for us to know if items are disabled if we don't open the menu.

I also think this will simplify the code a bit as well. It will be a breaking change, but moving from one API to another should not be a big deal.

[jorgemoya]

Oh, I completely missed the coverage error. I can add the missing tests.

I'm not very familiar with react-select, looking at the API it seems they accept an isItemDisabled function but options also have an isDisabled prop?

I'm all for having the isItemDisabled function but isn't the use case for when you want to disabled some options based on a criteria? In my use cases I rarely disabled more than one or two items, and there's no common criteria for them. It can be doable but it complicates the API imo.

[silviuaavram]

Even though they have disabled on item, they also do not support operations with the menu closed, such as selecting or opening on the next / previous item with Down/Up arrow.

And regarding to your use case, I imagine you have the disabled state stored in the same object as the rest of your item data. So it can just be isItemSelected: item => item.disabled. If you do this or just have in getItemProps({..., disabled: item.disabled}) it does not seem a big change.

[jorgemoya]

We could potentially default it to that logic and add the prop as an override? So that we don't need to pass in isItemDisabled function if we're simply adding disabled to some items?

[silviuaavram]

I agree, it does keep things easier as well. I just don't feel confident of providing 2 APIs that don't do the same thing. The current disabled API does not work fully with what we claim, which is selection with menu closed or skipping a disabled item when you open with Arrows.

[codecov-io]

Codecov Report

Merging #1205 (5e7a3ed) into master (0d87ecb) will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##            master     #1205   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           18        18           
  Lines         1631      1674   +43     
  Branches       464       474   +10     
=========================================
+ Hits          1631      1674   +43     

Impacted Files	Coverage Δ
src/hooks/useCombobox/index.js	100.00% <100.00%> (ø)
src/hooks/useCombobox/testUtils.js	100.00% <100.00%> (ø)
src/hooks/useSelect/index.js	100.00% <100.00%> (ø)
src/hooks/useSelect/testUtils.js	100.00% <100.00%> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0d87ecb...fa80bbd. Read the comment docs.

[jorgemoya]

@silviuaavram So I don't know what the conclusion ultimately was, but I fixed the coverage issues I was having. I just want to Downshift work smoothly with my memoized items, right now it is a big performance hit on my components.

I still can't figure out why I get my lint breaks when I run it locally, does it run well for you?

I pulled latest from master and I get an error on one of the useSelect tests, with or without my changes. I'm surprised validation passed. I don't know what is going on my env, it should be pretty barebones.

Ultimately I want to know if we want to merge these changes or if I should close and wait for another solution.

[silviuaavram]

Great, thanks!

So I think we can probably go ahead with this fix. Going further in v7 we should switch to isItemDisabled API. Currently adding disabled to getItemProps is just not good enough for our scenarios: memo-izing the Item components and changing highlightedIndex before the menu is open (so no information about the items and if they are disabled).

Will review this and merge if ok.

[silviuaavram]

Closing this one for #1219 as I refactored the tests a little bit. Apart from this refactor everything looks good to me!