useResizeObserver fails to unobserve due to ref being null

[karlpetersson]

🐛 Bug Report

The cleanup function of the useEffect in useResizeObserver can in some cases call unobserve with an empty ref, which will throw a TypeError (Failed to execute 'unobserve' on 'ResizeObserver': parameter 1 is not of type 'Element'.).

react-spectrum/packages/@react-aria/utils/src/useResizeObserver.ts

Lines 36 to 38 in 6e9f55a

	return () => {
	resizeObserverInstance.unobserve(ref.current);
	};

At least, this is what I think happens. If I add a check for ref.current existing, it works fine.

This seems like it happens when for example rendering a ScrollView inside a modal/popover that unmounts before the cleanup effect is called, in my case I was trying to use a Virtualizer inside a Popover (see code example). For some reason that I can't figure out, this doesn't happen for the react-spectrum Picker component, although it uses a Virtualizer in this same sense.

Here's a codesandbox where this issue is present. Opening then closing the Select throws the mentioned error. https://codesandbox.io/s/affectionate-vaughan-bcy0f?file=/src/App.tsx

🤔 Expected Behavior

😯 Current Behavior

💁 Possible Solution

I think changing the cleanup function in the useEffect in useResizeObserver to something like this could suffice:

  return () => {
    if (ref.current !== null) {
      resizeObserverInstance.unobserve(ref.current)
    }
  };

🔦 Context

I am building a custom Select component using the react-aria hooks as well as the Virtualizer component.

💻 Code Sample

https://codesandbox.io/s/affectionate-vaughan-bcy0f?file=/src/App.tsx

🌍 Your Environment

Software	Version(s)
react-spectrum
Browser
Operating System

🧢 Your Company/Team

Visly

🕷 Tracking Issue (optional)

[LFDanLu]

@karlpetersson Thanks for reporting this, I think your solution is reasonable. My guess would be that it works for Picker due to the transition the overlay wrapping the popover has.

[snowystinger]

I think we actually need to just store the ref.current that we use in the setup so that we can re-use it for the tear down
following the pattern described here
https://reactjs.org/blog/2020/08/10/react-v17-rc.html#effect-cleanup-timing

[snowystinger]

Looks like @LFDanLu has it in his PR already https://github.com/adobe/react-spectrum/pull/1145/files#diff-fa757bcd677038369ad3ed26cca2ff8d645c63dc009266f3d1aeb0e7bdca8a0dR16

[snowystinger]

The PR has been merged, can you verify that it fixed your issue?