From c879fce0d465cd96d0d05e329c0a8a83f46e29d9 Mon Sep 17 00:00:00 2001 From: Steven Date: Thu, 20 Jan 2022 10:29:34 -0500 Subject: [PATCH] Fix multiple calls to image `onLoadingComplete()` (#33474) The image prop `onLoadingComplete()` was unexpectedly called multiple times because it uses a [callback ref](https://reactjs.org/docs/refs-and-the-dom.html#callback-refs). This could lead to an infinite loop if `onLoadingComplete()` calls `setState()` as demonstrated in the updated test. The solution is to handle refs with `useRef()` and `useEffect` so `onLoadingComplete()` is called at most once per component instance. - Fixes #33463 --- packages/next/client/image.tsx | 64 +++++--- .../default/pages/on-loading-complete.js | 150 ++++++++++++------ .../default/test/index.test.js | 59 +++++-- 3 files changed, 188 insertions(+), 85 deletions(-) diff --git a/packages/next/client/image.tsx b/packages/next/client/image.tsx index d242b731f2501..04f2f9a175fdd 100644 --- a/packages/next/client/image.tsx +++ b/packages/next/client/image.tsx @@ -1,4 +1,4 @@ -import React from 'react' +import React, { useRef, useEffect } from 'react' import Head from '../shared/lib/head' import { ImageConfigComplete, @@ -8,7 +8,6 @@ import { } from '../server/image-config' import { useIntersection } from './use-intersection' -const loadedImageURLs = new Set() const allImgs = new Map< string, { src: string; priority: boolean; placeholder: string } @@ -250,31 +249,34 @@ function defaultImageLoader(loaderProps: ImageLoaderProps) { // See https://stackoverflow.com/q/39777833/266535 for why we use this ref // handler instead of the img's onLoad attribute. function handleLoading( - img: HTMLImageElement | null, + imgRef: React.RefObject, src: string, layout: LayoutValue, placeholder: PlaceholderValue, - onLoadingComplete?: OnLoadingComplete + onLoadingCompleteRef: React.MutableRefObject ) { - if (!img) { - return - } const handleLoad = () => { + const img = imgRef.current + if (!img) { + return + } if (img.src !== emptyDataURL) { const p = 'decode' in img ? img.decode() : Promise.resolve() p.catch(() => {}).then(() => { + if (!imgRef.current) { + return + } if (placeholder === 'blur') { img.style.filter = '' img.style.backgroundSize = '' img.style.backgroundImage = '' img.style.backgroundPosition = '' } - loadedImageURLs.add(src) - if (onLoadingComplete) { + if (onLoadingCompleteRef.current) { const { naturalWidth, naturalHeight } = img // Pass back read-only primitive values but not the // underlying DOM element because it could be misused. - onLoadingComplete({ naturalWidth, naturalHeight }) + onLoadingCompleteRef.current({ naturalWidth, naturalHeight }) } if (process.env.NODE_ENV !== 'production') { if (img.parentElement?.parentElement) { @@ -299,13 +301,15 @@ function handleLoading( }) } } - if (img.complete) { - // If the real image fails to load, this will still remove the placeholder. - // This is the desired behavior for now, and will be revisited when error - // handling is worked on for the image component itself. - handleLoad() - } else { - img.onload = handleLoad + if (imgRef.current) { + if (imgRef.current.complete) { + // If the real image fails to load, this will still remove the placeholder. + // This is the desired behavior for now, and will be revisited when error + // handling is worked on for the image component itself. + handleLoad() + } else { + imgRef.current.onload = handleLoad + } } } @@ -328,6 +332,7 @@ export default function Image({ blurDataURL, ...all }: ImageProps) { + const imgRef = useRef(null) let rest: Partial = all let layout: NonNullable = sizes ? 'responsive' : 'intrinsic' if ('layout' in rest) { @@ -376,7 +381,7 @@ export default function Image({ unoptimized = true isLazy = false } - if (typeof window !== 'undefined' && loadedImageURLs.has(src)) { + if (typeof window !== 'undefined' && imgRef.current?.complete) { isLazy = false } @@ -504,7 +509,7 @@ export default function Image({ } } - const [setRef, isIntersected] = useIntersection({ + const [setIntersection, isIntersected] = useIntersection({ rootMargin: lazyBoundary, disabled: !isLazy, }) @@ -657,6 +662,22 @@ export default function Image({ [imageSizesPropName]: imgAttributes.sizes, } + const useLayoutEffect = + typeof window === 'undefined' ? React.useEffect : React.useLayoutEffect + const onLoadingCompleteRef = useRef(onLoadingComplete) + + useEffect(() => { + onLoadingCompleteRef.current = onLoadingComplete + }, [onLoadingComplete]) + + useLayoutEffect(() => { + setIntersection(imgRef.current) + }, [setIntersection]) + + useEffect(() => { + handleLoading(imgRef, srcString, layout, placeholder, onLoadingCompleteRef) + }, [srcString, layout, placeholder, isVisible]) + return ( {hasSizer ? ( @@ -687,10 +708,7 @@ export default function Image({ decoding="async" data-nimg={layout} className={className} - ref={(img) => { - setRef(img) - handleLoading(img, srcString, layout, placeholder, onLoadingComplete) - }} + ref={imgRef} style={{ ...imgStyle, ...blurStyle }} /> {isLazy && ( diff --git a/test/integration/image-component/default/pages/on-loading-complete.js b/test/integration/image-component/default/pages/on-loading-complete.js index 25bea69b714f3..88ad4cc2a54a8 100644 --- a/test/integration/image-component/default/pages/on-loading-complete.js +++ b/test/integration/image-component/default/pages/on-loading-complete.js @@ -1,74 +1,122 @@ import { useState } from 'react' import Image from 'next/image' -const Page = () => ( -
-

On Loading Complete Test

- -
- -
- -
-
+const Page = () => { + // Hoisted state to count each image load callback + const [idToCount, setIdToCount] = useState({}) + const [clicked, setClicked] = useState(false) + + return ( +
+

On Loading Complete Test

+ + + + + + -
-
- -
-
+ + + + + + + + + - -) + ) +} -function ImageWithMessage({ id, ...props }) { +function ImageWithMessage({ id, idToCount, setIdToCount, ...props }) { const [msg, setMsg] = useState('[LOADING]') + const style = + props.layout === 'fill' + ? { position: 'relative', width: '64px', height: '64px' } + : {} return ( <> - - setMsg( - `loaded img${id} with dimensions ${naturalWidth}x${naturalHeight}` - ) - } - {...props} - /> +
+ { + let count = idToCount[id] || 0 + count++ + idToCount[id] = count + setIdToCount(idToCount) + const msg = `loaded ${count} img${id} with dimensions ${naturalWidth}x${naturalHeight}` + setMsg(msg) + console.log(msg) + }} + {...props} + /> +

{msg}

+
) } diff --git a/test/integration/image-component/default/test/index.test.js b/test/integration/image-component/default/test/index.test.js index cb053ff5524c8..d7d2c050f7c49 100644 --- a/test/integration/image-component/default/test/index.test.js +++ b/test/integration/image-component/default/test/index.test.js @@ -193,47 +193,84 @@ function runTests(mode) { try { browser = await webdriver(appPort, '/on-loading-complete') - await browser.eval(`document.getElementById("footer").scrollIntoView()`) + await browser.eval( + `document.getElementById("footer").scrollIntoView({behavior: "smooth"})` + ) await check( - () => browser.eval(`document.getElementById("img1").src`), + () => browser.eval(`document.getElementById("img1").currentSrc`), /test(.*)jpg/ ) await check( - () => browser.eval(`document.getElementById("img2").src`), + () => browser.eval(`document.getElementById("img2").currentSrc`), /test(.*).png/ ) await check( - () => browser.eval(`document.getElementById("img3").src`), + () => browser.eval(`document.getElementById("img3").currentSrc`), /test(.*)svg/ ) await check( - () => browser.eval(`document.getElementById("img4").src`), + () => browser.eval(`document.getElementById("img4").currentSrc`), /test(.*)ico/ ) await check( () => browser.eval(`document.getElementById("msg1").textContent`), - 'loaded img1 with dimensions 128x128' + 'loaded 1 img1 with dimensions 128x128' ) await check( () => browser.eval(`document.getElementById("msg2").textContent`), - 'loaded img2 with dimensions 400x400' + 'loaded 1 img2 with dimensions 400x400' ) await check( () => browser.eval(`document.getElementById("msg3").textContent`), - 'loaded img3 with dimensions 266x266' + 'loaded 1 img3 with dimensions 266x266' ) await check( () => browser.eval(`document.getElementById("msg4").textContent`), - 'loaded img4 with dimensions 21x21' + 'loaded 1 img4 with dimensions 21x21' ) await check( () => browser.eval(`document.getElementById("msg5").textContent`), - 'loaded img5 with dimensions 3x5' + 'loaded 1 img5 with dimensions 3x5' ) await check( () => browser.eval(`document.getElementById("msg6").textContent`), - 'loaded img6 with dimensions 3x5' + 'loaded 1 img6 with dimensions 3x5' + ) + await check( + () => browser.eval(`document.getElementById("msg7").textContent`), + 'loaded 1 img7 with dimensions 400x400' + ) + await check( + () => browser.eval(`document.getElementById("msg8").textContent`), + 'loaded 1 img8 with dimensions 640x373' + ) + await check( + () => + browser.eval( + `document.getElementById("img8").getAttribute("data-nimg")` + ), + 'intrinsic' + ) + await check( + () => browser.eval(`document.getElementById("img8").currentSrc`), + /wide.png/ + ) + await browser.eval('document.getElementById("toggle").click()') + await check( + () => browser.eval(`document.getElementById("msg8").textContent`), + 'loaded 2 img8 with dimensions 400x300' + ) + await check( + () => + browser.eval( + `document.getElementById("img8").getAttribute("data-nimg")` + ), + 'fixed' + ) + await check( + () => browser.eval(`document.getElementById("img8").currentSrc`), + /test-rect.jpg/ ) } finally { if (browser) {