diff --git a/packages/next/src/client/image-component.tsx b/packages/next/src/client/image-component.tsx index a4ef2efe32f8b..d5b8ca56ebdcc 100644 --- a/packages/next/src/client/image-component.tsx +++ b/packages/next/src/client/image-component.tsx @@ -31,6 +31,7 @@ import { RouterContext } from '../shared/lib/router-context.shared-runtime' // @ts-ignore - This is replaced by webpack alias import defaultLoader from 'next/dist/shared/lib/image-loader' +import { useMergedRef } from './use-merged-ref' // This is replaced by webpack define plugin const configEnv = process.env.__NEXT_IMAGE_OPTS as any as ImageConfigComplete @@ -206,6 +207,54 @@ const ImageElement = forwardRef( }, forwardedRef ) => { + const ownRef = useCallback( + (img: ImgElementWithDataProp | null) => { + if (!img) { + return + } + if (onError) { + // If the image has an error before react hydrates, then the error is lost. + // The workaround is to wait until the image is mounted which is after hydration, + // then we set the src again to trigger the error handler (if there was an error). + // eslint-disable-next-line no-self-assign + img.src = img.src + } + if (process.env.NODE_ENV !== 'production') { + if (!src) { + console.error(`Image is missing required "src" property:`, img) + } + if (img.getAttribute('alt') === null) { + console.error( + `Image is missing required "alt" property. Please add Alternative Text to describe the image for screen readers and search engines.` + ) + } + } + if (img.complete) { + handleLoading( + img, + placeholder, + onLoadRef, + onLoadingCompleteRef, + setBlurComplete, + unoptimized, + sizesInput + ) + } + }, + [ + src, + placeholder, + onLoadRef, + onLoadingCompleteRef, + setBlurComplete, + onError, + unoptimized, + sizesInput, + ] + ) + + const ref = useMergedRef(forwardedRef, ownRef) + return ( ( sizes={sizes} srcSet={srcSet} src={src} - ref={useCallback( - (img: ImgElementWithDataProp | null) => { - if (forwardedRef) { - if (typeof forwardedRef === 'function') forwardedRef(img) - else if (typeof forwardedRef === 'object') { - // @ts-ignore - .current is read only it's usually assigned by react internally - forwardedRef.current = img - } - } - if (!img) { - return - } - if (onError) { - // If the image has an error before react hydrates, then the error is lost. - // The workaround is to wait until the image is mounted which is after hydration, - // then we set the src again to trigger the error handler (if there was an error). - // eslint-disable-next-line no-self-assign - img.src = img.src - } - if (process.env.NODE_ENV !== 'production') { - if (!src) { - console.error(`Image is missing required "src" property:`, img) - } - if (img.getAttribute('alt') === null) { - console.error( - `Image is missing required "alt" property. Please add Alternative Text to describe the image for screen readers and search engines.` - ) - } - } - if (img.complete) { - handleLoading( - img, - placeholder, - onLoadRef, - onLoadingCompleteRef, - setBlurComplete, - unoptimized, - sizesInput - ) - } - }, - [ - src, - placeholder, - onLoadRef, - onLoadingCompleteRef, - setBlurComplete, - onError, - unoptimized, - sizesInput, - forwardedRef, - ] - )} + ref={ref} onLoad={(event) => { const img = event.currentTarget as ImgElementWithDataProp handleLoading( diff --git a/packages/next/src/client/link.tsx b/packages/next/src/client/link.tsx index e84a9d6ab1de5..9bb7bc5e457f2 100644 --- a/packages/next/src/client/link.tsx +++ b/packages/next/src/client/link.tsx @@ -22,6 +22,7 @@ import { useIntersection } from './use-intersection' import { getDomainLocale } from './get-domain-locale' import { addBasePath } from './add-base-path' import { PrefetchKind } from './components/router-reducer/router-reducer-types' +import { useMergedRef } from './use-merged-ref' type Url = string | UrlObject type RequiredKeys = { @@ -546,7 +547,7 @@ const Link = React.forwardRef( rootMargin: '200px', }) - const setRef = React.useCallback( + const setIntersectionWithResetRef = React.useCallback( (el: Element) => { // Before the link getting observed, check if visible state need to be reset if (previousAs.current !== as || previousHref.current !== href) { @@ -556,16 +557,12 @@ const Link = React.forwardRef( } setIntersectionRef(el) - if (childRef) { - if (typeof childRef === 'function') childRef(el) - else if (typeof childRef === 'object') { - childRef.current = el - } - } }, - [as, childRef, href, resetVisible, setIntersectionRef] + [as, href, resetVisible, setIntersectionRef] ) + const setRef = useMergedRef(setIntersectionWithResetRef, childRef) + // Prefetch the URL if we haven't already and it's visible. React.useEffect(() => { // in dev, we only prefetch on hover to avoid wasting resources as the prefetch will trigger compiling the page. diff --git a/packages/next/src/client/use-merged-ref.ts b/packages/next/src/client/use-merged-ref.ts new file mode 100644 index 0000000000000..7fce7fb1f0ee1 --- /dev/null +++ b/packages/next/src/client/use-merged-ref.ts @@ -0,0 +1,46 @@ +import { useMemo, type Ref } from 'react' + +export function useMergedRef( + refA: Ref, + refB: Ref +): Ref { + return useMemo(() => mergeRefs(refA, refB), [refA, refB]) +} + +export function mergeRefs( + refA: Ref, + refB: Ref +): Ref { + if (!refA || !refB) { + return refA || refB + } + + return (current: TElement) => { + const cleanupA = applyRef(refA, current) + const cleanupB = applyRef(refB, current) + + return () => { + cleanupA() + cleanupB() + } + } +} + +function applyRef( + refA: NonNullable>, + 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 + } + } +} diff --git a/test/integration/link-ref/pages/child-ref-func-cleanup.js b/test/integration/link-ref/pages/child-ref-func-cleanup.js new file mode 100644 index 0000000000000..50d9be294c717 --- /dev/null +++ b/test/integration/link-ref/pages/child-ref-func-cleanup.js @@ -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 ( + + Click me + + ) +} diff --git a/test/integration/link-ref/test/index.test.js b/test/integration/link-ref/test/index.test.js index 387e08ae5303a..433a3fcec87db 100644 --- a/test/integration/link-ref/test/index.test.js +++ b/test/integration/link-ref/test/index.test.js @@ -84,6 +84,10 @@ describe('Invalid hrefs', () => { it('should handle child ref that is a function', async () => { await noError('/child-ref-func') }) + + it('should handle child ref that is a function that returns a cleanup function', async () => { + await noError('/child-ref-func-cleanup') + }) } ) ;(process.env.TURBOPACK_DEV ? describe.skip : describe)( @@ -109,6 +113,10 @@ describe('Invalid hrefs', () => { it('should preload with child ref with function', async () => { await didPrefetch('/child-ref-func') }) + + it('should preload with child ref with function that returns a cleanup function', async () => { + await didPrefetch('/child-ref-func-cleanup') + }) } ) }) diff --git a/test/integration/next-image-new/app-dir/app/ref-cleanup/page.js b/test/integration/next-image-new/app-dir/app/ref-cleanup/page.js new file mode 100644 index 0000000000000..98b0dca18430f --- /dev/null +++ b/test/integration/next-image-new/app-dir/app/ref-cleanup/page.js @@ -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 ( +
+

Should call ref cleanup on unmount

+
+ {displayImage ? ( +
+ alt +
+ ) : null} +
+
+ ) +} diff --git a/test/integration/next-image-new/app-dir/test/index.test.ts b/test/integration/next-image-new/app-dir/test/index.test.ts index baa09158b5df2..9917e4d24ddf6 100644 --- a/test/integration/next-image-new/app-dir/test/index.test.ts +++ b/test/integration/next-image-new/app-dir/test/index.test.ts @@ -14,6 +14,7 @@ import { nextBuild, nextStart, renderViaHTTP, + retry, waitFor, } from 'next-test-utils' import webdriver from 'next-webdriver' @@ -1574,6 +1575,19 @@ function runTests(mode) { } } }) + + it('should call callback ref cleanups when unmounting', async () => { + const browser = await webdriver(appPort, '/ref-cleanup') + const getLogs = async () => (await browser.log()).map((log) => log.message) + + await retry(async () => { + expect(await getLogs()).toContain('callback ref was cleaned up') + }) + + expect(await getLogs()).not.toContain( + 'callback refs that returned a cleanup should never be called with null' + ) + }) } describe('Image Component Default Tests', () => {