-
Notifications
You must be signed in to change notification settings - Fork 26.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[image, link] fix ref merging for callback refs that return a cleanup function #68123
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
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 () => { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We don't have to handle backwards compatibility with React 18 so we can always return a cleanup. If we'd have to handle React 18, we'd have to only return a cleanup when we receive a cleanup and to trigger the default React warning in 18 where you can't return a cleanup function. |
||
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 | ||
} | ||
} | ||
} |
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. tbh this feels more convoluted than it needs to be, but the existing tests here all rely on checking "console.error wasn't called" and i don't want to set up a whole new test just for this |
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> | ||
) | ||
} |
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> | ||
) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the actual logic in the
ownRef
callback wasn't modified. i only removed the bits at the start that managed the user-providedforwardedRef
, becauseuseMergedRef
handles that now.(also note that the execution order remains the same)