sparksuite / react-accessible-dropdown-menu-hook

A simple Hook for creating fully accessible dropdown menus in React
http://sparksuite.github.io/react-accessible-dropdown-menu-hook
MIT License
112 stars 26 forks source link

Possibility of race conditions related to item refs #267

Closed corymharper closed 3 years ago

corymharper commented 3 years ago

Currently this is how our item refs are generated:

    // Initialize refs and update them when the item count changes
    useEffect(() => {
        itemRefs.current = Array.from({ length: itemCount }, () => createRef<HTMLAnchorElement>());
    }, [itemCount]);

As per the comment, it's even how we initially create our refs. This behavior was introduced to accommodate for implementations that might change the number of items in a menu variably. However, revisiting this code block, there are some key problems with it, specifically:

  1. The refs are held within a ref
  2. The ref is updated inside of a useEffect callback

The first is at issue because when a ref is updated, it does not force the component to update. This means that the component will not be required to have the most up to date ref array in order to finish it's render cycle. The second augments the issue making the update happen in a slightly unpredictable asynchronous manner, basically we can not guarantee the refs will be updated before the array is processed and the hook returns a value.

Solutions:

  1. Reconstruct the item refs on every recall of the hook, so instead of assigning it to a ref it would look more similar to const itemRefs = Array.from({ length: itemCount }, () => createRef<HTMLAnchorElement>()), that way it will be reevaluated accurately on every render. This could be further optimized very easily using useMemo.
  2. Convert itemRefs into state, this will guarantee the hook will return the most recent copy of the refs, but the downside is that it may result in more render cycles.
corymharper commented 3 years ago

Closed by #268