fix: use useImperativeHandle instead of mutating a parent ref

[stipsan]

Description

React Compiler isn't able to optimize <Pane> when it mutates the forwardedRef value returned by useForwardedRef.
That's fine, because we can use useImperativeHandle to forward refs to the parent in a way that is sound, and that React Compiler is able to handle.
We should probably deprecate the useForwardedRef hook in @sanity/ui as useImperativeHandle is a much better alternative (useForwardedRef for example needs a useEffect cycle to handle forwarding):

-import {useForwardedRef} from '@sanity/ui'
-import {forwardRef} from 'react'
+import {forwardRef, useImperativeHandle} from 'react'

export default forwardRef(function Component(props, ref) {
-  const nodeRef = useForwardedRef(ref)
+  const nodeRef = useRef()
+  useImperativeHandle(ref, () => nodeRef.current)

  return <div ref={nodeRef} />
})

What to review

Does it make sense?

Testing

Existing tests should be sufficient, as the refactor doesn't have practical differences, beyond being slightly faster in general, and noticeably faster when React Compiler is enabled.

Notes for release

N/A

[vercel]

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
page-building-studio	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	May 30, 2024 2:42pm
performance-studio	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	May 30, 2024 2:42pm
test-compiled-studio	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	May 30, 2024 2:42pm
test-next-studio	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	May 30, 2024 2:42pm
test-studio	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	May 30, 2024 2:42pm

1 Ignored Deployment

Name	Status	Preview	Comments	Updated (UTC)
studio-workshop	⬜️ Ignored (Inspect)			May 30, 2024 2:42pm

[socket-security]

New and removed dependencies detected. Learn more about Socket for GitHub ↗︎

Package	New capabilities	Transitives	Size	Publisher

🚮 Removed packages: npm/@sanity/[email protected], npm/@sanity/[email protected], npm/@sanity/[email protected], npm/@sanity/[email protected], npm/@sanity/[email protected], npm/@sanity/[email protected], npm/@sanity/[email protected], npm/@tanstack/[email protected], npm/@tanstack/[email protected], npm/@testing-library/[email protected], npm/@testing-library/[email protected], npm/@testing-library/[email protected], npm/[email protected]

View full report↗︎

[github-actions]

No changes to documentation

[github-actions]

Component Testing Report Updated May 30, 2024 2:50 PM (UTC)

File	Status	Duration	Passed	Skipped	Failed
comments/CommentInput.spec.tsx	✅ Passed (Inspect)	35s	15	0	0
formBuilder/ArrayInput.spec.tsx	✅ Passed (Inspect)	6s	3	0	0
formBuilder/inputs/PortableText/Annotations.spec.tsx	✅ Passed (Inspect)	26s	6	0	0
formBuilder/inputs/PortableText/copyPaste/CopyPaste.spec.tsx	✅ Passed (Inspect)	31s	11	7	0
formBuilder/inputs/PortableText/Decorators.spec.tsx	✅ Passed (Inspect)	14s	6	0	0
formBuilder/inputs/PortableText/DisableFocusAndUnset.spec.tsx	✅ Passed (Inspect)	8s	3	0	0
formBuilder/inputs/PortableText/FocusTracking.spec.tsx	✅ Passed (Inspect)	37s	15	0	0
formBuilder/inputs/PortableText/Input.spec.tsx	✅ Passed (Inspect)	1m 13s	21	0	0
formBuilder/inputs/PortableText/ObjectBlock.spec.tsx	✅ Passed (Inspect)	1m 4s	18	0	0
formBuilder/inputs/PortableText/PresenceCursors.spec.tsx	✅ Passed (Inspect)	7s	3	9	0
formBuilder/inputs/PortableText/RangeDecoration.spec.tsx	✅ Passed (Inspect)	20s	9	0	0
formBuilder/inputs/PortableText/Styles.spec.tsx	✅ Passed (Inspect)	14s	6	0	0
formBuilder/inputs/PortableText/Toolbar.spec.tsx	✅ Passed (Inspect)	30s	12	0	0

[juice49]

This is pedantic, but would it be more correct to type the imperative handle as HTMLDivElement | null, rather than using a non-null assertion? My understanding is that rootElement! asserts rootElement is never null, but in reality, it is.

Suggested change

	useImperativeHandle(ref, () => rootElement!, [rootElement])
	useImperativeHandle<HTMLDivElement | null, HTMLDivElement | null>(ref, () => rootElement, [
	rootElement,
	])

[stipsan]

Yes, you're correct. The parent ref: ForwardedRef<HTMLDivElement> mandates it's always assigned, so I considered changing it as well so it's fully consistent. But it caused other side-effects to the type checks that felt out of scope. Do you want me to do it in a follow-up?

[juice49]

I wondered about that, too, but ForwardedRef appears to always produce a nullable result (which is correct for consumers of this component).

IMO it's probably not worth spending more time on 😃.

[juice49]

This looks good to me! I left one small comment, but it's not a blocker.