Deprecate and remove `useFocusZone` dependency array

[iansan5653]

useFocusZone accepts an uncommon optional second argument: an array of dependencies. While comparable to the dependency arrays passed to other React hooks, the optionality and unlinted nature of this argument make it easy to forget about or incorrectly configure. I have personally introduced a few bugs because I didn't know this argument needed to be considered.

I think with modern JavaScript this array is probably wholly unnecessary. All options passed to useFocusZone through the options object can easily be included as dependencies by using a shallow comparison. The trickier part is for the hook to know when the contents of the container element change, but this can now relatively easily be achieved by using a MutationObserver on the parent.

With MutationObserver, the dependencies array could be totally removed. This would simplify the API and clear up a potential footgun.

[lesliecdubs]

Thanks for filing, @iansan5653! Things are a bit quiet though the holidays, so I'm going to leave this in the Primer inbox to discuss with the team in the new year to see how we might be able to move this forward.

Open question: would this be a breaking change?

[hectahertz]

Hi @iansan5653, thanks for reporting this and for the suggestions.

Could you please share in which cases you've encountered bugs with this approach? It would help us decide where we want to go with this.

The way it is designed right now, it assumes you won't use the dependency array unless there is an explicit need to reset the zone.

I'm afraid if we were to implement this using a MutationObserver we could run into situations where we reset when we shouldn't, but I'd like to hear your thoughts.

[iansan5653]

The way it is designed right now, it assumes you won't use the dependency array unless there is an explicit need to reset the zone.

The problem with this approach is that it assumes by default that the contents of the zone will never change - that items will never be added, removed, or reordered. This runs contrary to the React philosophy of being reactive to changes. I think any use of useFocusZone without the dependency array is potentially buggy because contents of React components can always be assumed to be changing by default. But even with the dependency array it's difficult to ensure there are no bugs because this array is not lintable like the dependencies of hooks and memos.

MutationObserver is a logical solution because what we really care about is detecting DOM changes and updating the zone accordingly.

I'm afraid if we were to implement this using a MutationObserver we could run into situations where we reset when we shouldn't, but I'd like to hear your thoughts.

I think it wouldn't be too difficult to bail out in cases that don't require a reset. It's relatively inexpensive (performance-wise) to generate the list of elements involved in the zone. Then we can simply do a shallow comparison to see if any elements were added/removed/reordered, and only reset the zone in these cases.

[iansan5653]

I actually ran into a relatively standard situation today that appears impossible to handle with useFocusZone: I want to provide a wrapper component that creates a focus zone around its children. This seems pretty standard, but the only thing that the component could use as a dependency would be children, which would cause a reset on every single render - definitely not ideal:

export function ReferenceTokenList({children}: ReferenceTokenListProps) {
  const containerRef = useRef<HTMLDivElement>(null)
  useFocusZone(
    {
      containerRef,
      bindKeys: FocusKeys.ArrowHorizontal | FocusKeys.HomeAndEnd,
      focusInStrategy: 'previous',
      focusOutBehavior: 'stop',
    },
    [children], // What should go here? `children` always changes on every render
  )

  return <div>{children}</div>
}

[github-actions[bot]]

Hi! This issue has been marked as stale because it has been open with no activity for 180 days. You can comment on the issue or remove the stale label to keep it open. If you do nothing, this issue will be closed in 7 days.