vercel · lubieowoce · Jan 21, 2025 · Jan 20, 2025 · Jan 20, 2025 · Jan 20, 2025
diff --git 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<HTMLAnchorElement, LinkPropsReal>(
     // currently mounted <Link> 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]

diff --git a/packages/next/src/client/link.tsx b/packages/next/src/client/link.tsx
@@ -466,7 +466,7 @@ const Link = React.forwardRef<HTMLAnchorElement, LinkPropsReal>(
     })
 
     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()

diff --git 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<TElement>(
   refA: Ref<TElement>,
   refB: Ref<TElement>
 ): Ref<TElement> {
-  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 `<Link legacyBehavior>`),
+  // 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<TElement>(

diff --git a/test/production/next-link-legacybehavior-ref-merging/app/layout.tsx 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 (
+    <html lang="en">
+      <body>{children}</body>
+    </html>
+  )
+}
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
@@ -0,0 +1,11 @@
+import * as React from 'react'
+import Link from 'next/link'
+
+export default function Page() {
+  return (
+    <>
+      <h1>Navigation worked!</h1>
+      <Link href="/">Go back</Link>
+    </>
+  )
+}
diff --git a/test/production/next-link-legacybehavior-ref-merging/app/page.tsx 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 (
+    <>
+      <h1>Home</h1>
+      <ToggleVisibility>
+        <Link href="/link-target" legacyBehavior>
+          <AnchorThatDoesRefMerging id="test-link">
+            Go to /link-target
+          </AnchorThatDoesRefMerging>
+        </Link>
+      </ToggleVisibility>
+    </>
+  )
+}
+
+function ToggleVisibility({ children }: { children: ReactNode }) {
+  const [isVisible, setIsVisible] = useState(true)
+  return (
+    <>
+      <div>
+        <button type="button" onClick={() => setIsVisible((prev) => !prev)}>
+          {isVisible ? 'Hide content' : 'Show content'}
+        </button>
+      </div>
+      {isVisible ? children : null}
+    </>
+  )
+}
+
+function AnchorThatDoesRefMerging({
+  ref,
+  children,
+  ...anchorProps
+}: ComponentPropsWithRef<'a'>) {
+  const customRef: RefCallback<HTMLAnchorElement> = 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 (
+    <a ref={finalRef} {...anchorProps}>
+      {children}
+    </a>
+  )
+}
+
+/** 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<TElement>(
+  refA: Ref<TElement>,
+  refB: Ref<TElement>
+): RefCallback<TElement> {
+  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
@@ -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
@@ -0,0 +1,2 @@
+/** @type {import('next').NextConfig} */
+export default {}
Original file line number	Diff line number	Diff line change
		@@ -0,0 +1,2 @@
		/** @type {import('next').NextConfig} */
		export default {}