-
Notifications
You must be signed in to change notification settings - Fork 26.9k
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
fix ref merging for callback refs that return a cleanup function (#68123
) ### What Fix handling of user-provided refs in `Image` and `Link` to support [refs with cleanup functions](https://react.dev/blog/2024/04/25/react-19#cleanup-functions-for-refs) (new in React 19) ### Why React 19 allows [a new form of callback ref](https://react.dev/blog/2024/04/25/react-19#cleanup-functions-for-refs), which returns a cleanup function: ```ts (element: HTMLElement) => { console.log('hi ref!') return () => { console.log('bye ref!') } } ``` Unfortunately, this can be a breaking change for code that attempts to combine ("merge") multiple callback refs into one. With old-style callback refs, we never had to care about the return values. But with the introduction of cleanup functions, ignoring the return value can lead to logic errors (because the cleanup will never be called)! This was the case for `Image` and `Link`, both of which attempt to combine user-provided refs with their own callback refs, but were ignoring the return values, so they'd break if the user's ref returned a cleanup function. ### How This PR introduces a `useMergedRef(refA, refB)` hook that combines all forms of refs correctly (thanks @eps1lon for sketching it out!) and refactors Image and Link to use it.
- Loading branch information
1 parent
16a55ae
commit ccbcfff
Showing
7 changed files
with
218 additions
and
61 deletions.
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,46 @@ | ||
| import { useMemo, type Ref } from 'react' | ||
|
|
||
| export function useMergedRef<TElement>( | ||
| refA: Ref<TElement>, | ||
| refB: Ref<TElement> | ||
| ): Ref<TElement> { | ||
| return useMemo(() => mergeRefs(refA, refB), [refA, refB]) | ||
| } | ||
|
|
||
| export function mergeRefs<TElement>( | ||
| refA: Ref<TElement>, | ||
| refB: Ref<TElement> | ||
| ): Ref<TElement> { | ||
| if (!refA || !refB) { | ||
| return refA || refB | ||
| } | ||
|
|
||
| return (current: TElement) => { | ||
| const cleanupA = applyRef(refA, current) | ||
| const cleanupB = applyRef(refB, current) | ||
|
|
||
| return () => { | ||
| cleanupA() | ||
| cleanupB() | ||
| } | ||
| } | ||
| } | ||
|
|
||
| function applyRef<TElement>( | ||
| refA: NonNullable<Ref<TElement>>, | ||
| current: TElement | ||
| ) { | ||
| if (typeof refA === 'function') { | ||
| const cleanup = refA(current) | ||
| if (typeof cleanup === 'function') { | ||
| return cleanup | ||
| } else { | ||
| return () => refA(null) | ||
| } | ||
| } else { | ||
| refA.current = current | ||
| return () => { | ||
| refA.current = null | ||
| } | ||
| } | ||
| } |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,52 @@ | ||
| import React from 'react' | ||
| import Link from 'next/link' | ||
| import { useCallback, useRef, useEffect, useState } from 'react' | ||
| import { flushSync } from 'react-dom' | ||
|
|
||
| export default function Page() { | ||
| const [isVisible, setIsVisible] = useState(true) | ||
|
|
||
| const statusRef = useRef({ wasInitialized: false, wasCleanedUp: false }) | ||
|
|
||
| const refWithCleanup = useCallback((el) => { | ||
| if (!el) { | ||
| console.error( | ||
| 'callback refs that returned a cleanup should never be called with null' | ||
| ) | ||
| return | ||
| } | ||
|
|
||
| statusRef.current.wasInitialized = true | ||
| return () => { | ||
| statusRef.current.wasCleanedUp = true | ||
| } | ||
| }, []) | ||
|
|
||
| useEffect(() => { | ||
| const timeout = setTimeout( | ||
| () => { | ||
| flushSync(() => { | ||
| setIsVisible(false) | ||
| }) | ||
| if (!statusRef.current.wasInitialized) { | ||
| console.error('callback ref was not initialized') | ||
| } | ||
| if (!statusRef.current.wasCleanedUp) { | ||
| console.error('callback ref was not cleaned up') | ||
| } | ||
| }, | ||
| 100 // if we hide the Link too quickly, the prefetch won't fire, failing a test | ||
| ) | ||
| return () => clearTimeout(timeout) | ||
| }, []) | ||
|
|
||
| if (!isVisible) { | ||
| return null | ||
| } | ||
|
|
||
| return ( | ||
| <Link href="/" ref={refWithCleanup}> | ||
| Click me | ||
| </Link> | ||
| ) | ||
| } |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
43 changes: 43 additions & 0 deletions
43
test/integration/next-image-new/app-dir/app/ref-cleanup/page.js
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,43 @@ | ||
| 'use client' | ||
| import Image from 'next/image' | ||
| import { useCallback, useEffect, useState } from 'react' | ||
|
|
||
| export default function Home() { | ||
| const [displayImage, setDisplayImage] = useState(true) | ||
|
|
||
| const refWithCleanup = useCallback((el) => { | ||
| if (!el) { | ||
| throw new Error( | ||
| 'callback refs that returned a cleanup should never be called with null' | ||
| ) | ||
| } | ||
|
|
||
| return () => { | ||
| console.log('callback ref was cleaned up') | ||
| } | ||
| }, []) | ||
|
|
||
| useEffect(() => { | ||
| setDisplayImage(false) | ||
| }, []) | ||
|
|
||
| return ( | ||
| <main> | ||
| <h1>Should call ref cleanup on unmount</h1> | ||
| <section> | ||
| {displayImage ? ( | ||
| <div style={{ position: 'relative', width: 10, height: 10 }}> | ||
| <Image | ||
| ref={refWithCleanup} | ||
| priority | ||
| fill | ||
| src="/test.jpg" | ||
| alt="alt" | ||
| sizes="10px" | ||
| /> | ||
| </div> | ||
| ) : null} | ||
| </section> | ||
| </main> | ||
| ) | ||
| } |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters