From 5daba1c262ef79f68b6709d857624f778cecd8f1 Mon Sep 17 00:00:00 2001 From: Janka Uryga Date: Mon, 20 Jan 2025 03:08:46 +0100 Subject: [PATCH 1/4] Revert "fix: always ensure element before set to weakmap (#75012)" This reverts commit 7229934b8118a6f1a2baef1822e32ed0e70c4423. --- packages/next/src/client/app-dir/link.tsx | 11 +++-------- 1 file changed, 3 insertions(+), 8 deletions(-) diff --git a/packages/next/src/client/app-dir/link.tsx b/packages/next/src/client/app-dir/link.tsx index cf7706995cb36..adf90964deb75 100644 --- a/packages/next/src/client/app-dir/link.tsx +++ b/packages/next/src/client/app-dir/link.tsx @@ -640,17 +640,12 @@ const Link = React.forwardRef( // currently mounted instances, e.g. so we can re-prefetch them after // a revalidation or refresh. const observeLinkVisibilityOnMount = React.useCallback( - (element: HTMLAnchorElement | SVGAElement | null) => { + (element: HTMLAnchorElement | SVGAElement) => { if (prefetchEnabled && router !== null) { - // FIXME: element still can be null here in some cases. Require further investigation. - if (element) { - mountLinkInstance(element, href, router, appPrefetchKind) - } + mountLinkInstance(element, href, router, appPrefetchKind) } return () => { - if (element) { - unmountLinkInstance(element) - } + unmountLinkInstance(element) } }, [prefetchEnabled, href, router, appPrefetchKind] From 274d91802d807ce906079c1ea672cfafb1f348bd Mon Sep 17 00:00:00 2001 From: Janka Uryga Date: Mon, 20 Jan 2025 16:08:03 +0100 Subject: [PATCH 2/4] test: reproduce link crash --- .../app/layout.tsx | 13 +++ .../app/link-target/page.tsx | 11 +++ .../app/page.tsx | 87 +++++++++++++++++++ .../index.test.ts | 25 ++++++ .../next.config.mjs | 2 + 5 files changed, 138 insertions(+) create mode 100644 test/production/next-link-legacybehavior-ref-merging/app/layout.tsx create mode 100644 test/production/next-link-legacybehavior-ref-merging/app/link-target/page.tsx create mode 100644 test/production/next-link-legacybehavior-ref-merging/app/page.tsx create mode 100644 test/production/next-link-legacybehavior-ref-merging/index.test.ts create mode 100644 test/production/next-link-legacybehavior-ref-merging/next.config.mjs diff --git a/test/production/next-link-legacybehavior-ref-merging/app/layout.tsx b/test/production/next-link-legacybehavior-ref-merging/app/layout.tsx new file mode 100644 index 0000000000000..5874c94ecfcf9 --- /dev/null +++ b/test/production/next-link-legacybehavior-ref-merging/app/layout.tsx @@ -0,0 +1,13 @@ +import * as React from 'react' + +export default function RootLayout({ + children, +}: { + children: React.ReactNode +}) { + return ( + + {children} + + ) +} diff --git a/test/production/next-link-legacybehavior-ref-merging/app/link-target/page.tsx b/test/production/next-link-legacybehavior-ref-merging/app/link-target/page.tsx new file mode 100644 index 0000000000000..d489d4cedfa79 --- /dev/null +++ b/test/production/next-link-legacybehavior-ref-merging/app/link-target/page.tsx @@ -0,0 +1,11 @@ +import * as React from 'react' +import Link from 'next/link' + +export default function Page() { + return ( + <> +

Navigation worked!

+ Go back + + ) +} diff --git a/test/production/next-link-legacybehavior-ref-merging/app/page.tsx b/test/production/next-link-legacybehavior-ref-merging/app/page.tsx new file mode 100644 index 0000000000000..9e33f8f609297 --- /dev/null +++ b/test/production/next-link-legacybehavior-ref-merging/app/page.tsx @@ -0,0 +1,87 @@ +'use client' +import Link from 'next/link' +import * as React from 'react' +import { + useCallback, + useState, + type RefCallback, + type Ref, + type ComponentPropsWithRef, + type ReactNode, +} from 'react' + +export default function Page() { + return ( + <> +

Home

+ + + + Go to /link-target + + + + + ) +} + +function ToggleVisibility({ children }: { children: ReactNode }) { + const [isVisible, setIsVisible] = useState(true) + return ( + <> +
+ +
+ {isVisible ? children : null} + + ) +} + +function AnchorThatDoesRefMerging({ + ref, + children, + ...anchorProps +}: ComponentPropsWithRef<'a'>) { + const customRef: RefCallback = useCallback((el) => { + if (el) { + console.log('hello friends i am here') + } else { + console.log('goodbye friends i am gone') + } + }, []) + + const finalRef = useBuggyRefMerge(customRef, ref ?? null) + return ( + + {children} + + ) +} + +/** A ref-merging function that doesn't account for cleanup refs (added in React 19) + * https://react.dev/blog/2024/12/05/react-19#cleanup-functions-for-refs + */ +function useBuggyRefMerge( + refA: Ref, + refB: Ref +): RefCallback { + return useCallback( + (current: TElement | null) => { + for (const ref of [refA, refB]) { + if (!ref) { + continue + } + if (typeof ref === 'object') { + ref.current = current + } else { + // BUG!!! + // This would work in 18, but in 19 it can return a cleanup which will get swallowed here + ref(current) + } + } + }, + [refA, refB] + ) +} diff --git a/test/production/next-link-legacybehavior-ref-merging/index.test.ts b/test/production/next-link-legacybehavior-ref-merging/index.test.ts new file mode 100644 index 0000000000000..5639c46420304 --- /dev/null +++ b/test/production/next-link-legacybehavior-ref-merging/index.test.ts @@ -0,0 +1,25 @@ +import { nextTestSetup } from 'e2e-utils' + +// NOTE: this test is checking for a bug in prefetching code, +// so we only enable it in production + +describe('Link with legacyBehavior - handles buggy userspace ref merging', () => { + const { next } = nextTestSetup({ + files: __dirname, + }) + it('does not crash when Link unmounts', async () => { + const browser = await next.browser('/') + expect(await browser.elementByCss('h1').text()).toEqual('Home') + expect(await browser.hasElementByCssSelector('#test-link')).toBe(true) + + // hide the link, unmounting it + await browser.elementByCss('button').click() + expect(await browser.hasElementByCssSelector('#test-link')).toBe(false) + + // shouldn't cause a crash + expect(await browser.elementByCss('h1').text()).toEqual('Home') + expect(await browser.elementByCss('body').text()).not.toContain( + 'Application error: a client-side exception has occurred (see the browser console for more information).' + ) + }) +}) diff --git a/test/production/next-link-legacybehavior-ref-merging/next.config.mjs b/test/production/next-link-legacybehavior-ref-merging/next.config.mjs new file mode 100644 index 0000000000000..e722bfa183143 --- /dev/null +++ b/test/production/next-link-legacybehavior-ref-merging/next.config.mjs @@ -0,0 +1,2 @@ +/** @type {import('next').NextConfig} */ +export default {} From 87b10a6222dd915810e6db78515b2197284bf68a Mon Sep 17 00:00:00 2001 From: Janka Uryga Date: Mon, 20 Jan 2025 03:19:52 +0100 Subject: [PATCH 3/4] fix useMergedRef --- packages/next/src/client/use-merged-ref.ts | 46 +++++++++++++++------- 1 file changed, 31 insertions(+), 15 deletions(-) diff --git a/packages/next/src/client/use-merged-ref.ts b/packages/next/src/client/use-merged-ref.ts index 65bb8dd4dcb42..c06beb4d8ad83 100644 --- a/packages/next/src/client/use-merged-ref.ts +++ b/packages/next/src/client/use-merged-ref.ts @@ -1,4 +1,4 @@ -import { useMemo, useRef, type Ref } from 'react' +import { useCallback, useRef, type Ref } from 'react' // This is a compatibility hook to support React 18 and 19 refs. // In 19, a cleanup function from refs may be returned. @@ -11,24 +11,40 @@ export function useMergedRef( refA: Ref, refB: Ref ): Ref { - const cleanupA = useRef<() => void>(() => {}) - const cleanupB = useRef<() => void>(() => {}) + const cleanupA = useRef<(() => void) | null>(null) + const cleanupB = useRef<(() => void) | null>(null) - return useMemo(() => { - if (!refA || !refB) { - return refA || refB - } - - return (current: TElement | null): void => { + // NOTE: In theory, we could skip the wrapping if only one of the refs is non-null. + // (this happens often if the user doesn't pass a ref to Link/Form/Image) + // But this can cause us to leak a cleanup-ref into user code (e.g. via ``), + // and the user might pass that ref into ref-merging library that doesn't support cleanup refs + // (because it hasn't been updated for React 19) + // which can then cause things to blow up, because a cleanup-returning ref gets called with `null`. + // So in practice, it's safer to be defensive and always wrap the ref, even on React 19. + return useCallback( + (current: TElement | null): void => { if (current === null) { - cleanupA.current() - cleanupB.current() + const cleanupFnA = cleanupA.current + if (cleanupFnA) { + cleanupA.current = null + cleanupFnA() + } + const cleanupFnB = cleanupB.current + if (cleanupFnB) { + cleanupB.current = null + cleanupFnB() + } } else { - cleanupA.current = applyRef(refA, current) - cleanupB.current = applyRef(refB, current) + if (refA) { + cleanupA.current = applyRef(refA, current) + } + if (refB) { + cleanupB.current = applyRef(refB, current) + } } - } - }, [refA, refB]) + }, + [refA, refB] + ) } function applyRef( From 7bf5d12634cec7a92d6b6b4667510d682a315edf Mon Sep 17 00:00:00 2001 From: Janka Uryga Date: Mon, 20 Jan 2025 03:28:57 +0100 Subject: [PATCH 4/4] random type fix --- packages/next/src/client/link.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/next/src/client/link.tsx b/packages/next/src/client/link.tsx index ee7eab27195f5..685033eedd557 100644 --- a/packages/next/src/client/link.tsx +++ b/packages/next/src/client/link.tsx @@ -466,7 +466,7 @@ const Link = React.forwardRef( }) const setIntersectionWithResetRef = React.useCallback( - (el: Element) => { + (el: Element | null) => { // Before the link getting observed, check if visible state need to be reset if (previousAs.current !== as || previousHref.current !== href) { resetVisible()