zendeskgarden / react-components

:seedling: garden React components
https://zendeskgarden.github.io/react-components/
Apache License 2.0
1.1k stars 89 forks source link

`Tabs` component crashes in React.StrictMode #1407

Closed eysterbrian closed 1 year ago

eysterbrian commented 2 years ago

Similar to the issue with ButtonGroup https://github.com/zendeskgarden/react-components/issues/1162, the Tabs component crashes when run in React's StrictMode (with React v17 or v18).

Expectations

The Tabs component should allow different tabs to be selected, when run using <React.StrictMode>.

Reality

But when built using React.StrictMode (and run in a dev build) clicking any tabs yields the error: "Cannot read properties of null (reading 'focus')"

image

Steps to Reproduce

  1. Here's a fork of the Tabs "Default" docs example, with the only change being that the component is wrapped in <React.StrictMode> in index.tsx: https://codesandbox.io/s/peaceful-proskuriakova-40cvdr

Fine Print

melanieseltzer commented 2 years ago

It's likely because of React 18's new dev-only Strict Mode behaviors RE: unmounting/remounting.

I was curious and did a little debugging with some logs, am getting different results for these array lengths in Strict Mode vs. non Strict Mode.

Non strict mode result:

Screen Shot 2022-09-12 at 5 55 53 PM

Strict mode result:

Screen Shot 2022-09-12 at 5 42 32 PM

I guess with the new remounting behavior in Strict Mode, the syncing between arrays is off, with this no longer being a reliable way to find the focused index:

const focusedIndex = items.indexOf(controlledFocusedItem);
refs[focusedIndex] && refs[focusedIndex].current!.focus(); // `current` could be null

Not 100% sure of a solution, but just thought this breakdown might be helpful 🙂

eysterbrian commented 2 years ago

Thanks for looking into this, @melanieseltzer.

I wondered, too, about that unmounting/remounting behavior in React 18 StrictMode. But I did find that this problem exists with React v17 as well as v18 fwiw.

Francois-Esquire commented 2 years ago

Hey @eysterbrian thanks for bringing this up, and @melanieseltzer thank you for looking more into this. We have had plans to address the React 18 migration, however it's not at the top of our priority list at the moment - it is on the list though, and we will be addressing this soon!

jzempel commented 1 year ago

Fixed by #1580