fix: reset visible state when src/href changed #35287

SukkaW · 2022-03-13T12:33:01Z

Bug

Related issues linked using fixes next/image will not lazyload image when the component is being reused across updates #35286
Integration tests added
Errors have helpful link attached, see contributing.md

SukkaW · 2022-03-15T04:24:41Z

The PR is rebased to the latest canary.

SukkaW · 2022-03-23T14:47:43Z

Ping? Anyone, please review this?

styfle

Thanks for this PR!

Please add a test so we can verify that this fixes the issue.

You can add a page to the fixture in test/integration/image-component/default/pages/ and then test it in test/integration/image-component/default/test/index.test.js

SukkaW · 2022-04-04T14:23:15Z

Thanks for this PR!

Please add a test so we can verify that this fixes the issue.

You can add a page to the fixture in test/integration/image-component/default/pages/ and then test it in test/integration/image-component/default/test/index.test.js

The issue was caused by the next/image / next/link component (along with their corresponding HTMLImageElement / HTMLAnchorElement) being reused by React during reconciliation, so useIntersection can not reset the visible state.

Although I am able to provide a minimum reproduction, I am still having trouble making a unit test case for it.

styfle · 2022-04-04T14:43:25Z

We don't need a unit test, we want an integration test which will run in a browser.

The reproduction from #35286 should be sufficient, although I think it could be simplified to use the existing images and fewer components.

Take for example this integration fixture pages/update.js:

next.js/test/integration/image-component/default/pages/update.js

Line 16 in c8fa284

<button id="toggle" onClick={() => setToggled(true)}>

It simulates a click like this:

next.js/test/integration/image-component/default/test/index.test.js

Line 174 in 352e60d

it('should update the image on src change', async () => {

So we need is a new page in pages that demonstrates the bug and a new test in index.test.js that asserts the result.

test/integration/image-component/default/pages/lazy-src-change.js

test/integration/image-component/default/test/index.test.js

test/integration/image-component/default/pages/lazy-src-change.js

ijjk · 2022-04-04T16:16:04Z

Failing test suites

Commit: 091842a

yarn testheadless test/e2e/prerender.test.ts

Prerender > should handle manual revalidate for fallback: blocking

Expand output

● Prerender › should handle manual revalidate for fallback: blocking

expect(received).toMatch(expected)

Expected pattern: /(HIT|STALE)/
Received string:  "MISS"

  1982 |         const html2 = await res2.text()
  1983 |         const $2 = cheerio.load(html2)
> 1984 |         expect(res2.headers.get('x-nextjs-cache')).toMatch(/(HIT|STALE)/)
       |                                                    ^
  1985 |
  1986 |         expect(initialTime).toBe($2('#time').text())
  1987 |

  at Object.<anonymous> (e2e/prerender.test.ts:1984:52)

Read more about building and testing Next.js in contributing.md.

ijjk · 2022-04-04T17:31:59Z

Stats from current PR

Default Build (Increase detected ⚠️)

General Overall increase ⚠️

	vercel/next.js canary	SukkaW/next.js fix-issue-35286	Change
buildDuration	18s	17.9s	-92ms
buildDurationCached	6.9s	7s	⚠️ +58ms
nodeModulesSize	477 MB	477 MB	⚠️ +1.61 kB

Page Load Tests Overall decrease ⚠️

	vercel/next.js canary	SukkaW/next.js fix-issue-35286	Change
/ failed reqs	0	0	✓
/ total time (seconds)	3.634	3.678	⚠️ +0.04
/ avg req/sec	688.04	679.63	⚠️ -8.41
/error-in-render failed reqs	0	0	✓
/error-in-render total time (seconds)	1.564	1.617	⚠️ +0.05
/error-in-render avg req/sec	1598.66	1545.77	⚠️ -52.89

Client Bundles (main, webpack)

	vercel/next.js canary	SukkaW/next.js fix-issue-35286	Change
925.HASH.js gzip	179 B	179 B	✓
framework-HASH.js gzip	42 kB	42 kB	✓
main-HASH.js gzip	28.4 kB	28.4 kB	✓
webpack-HASH.js gzip	1.44 kB	1.44 kB	✓
Overall change	72.1 kB	72.1 kB	✓

Legacy Client Bundles (polyfills)

	vercel/next.js canary	SukkaW/next.js fix-issue-35286	Change
polyfills-HASH.js gzip	31 kB	31 kB	✓
Overall change	31 kB	31 kB	✓

Client Pages Overall increase ⚠️

	vercel/next.js canary	SukkaW/next.js fix-issue-35286	Change
_app-HASH.js gzip	1.36 kB	1.36 kB	✓
_error-HASH.js gzip	192 B	192 B	✓
amp-HASH.js gzip	309 B	309 B	✓
css-HASH.js gzip	327 B	327 B	✓
dynamic-HASH.js gzip	3.05 kB	3.05 kB	✓
head-HASH.js gzip	351 B	351 B	✓
hooks-HASH.js gzip	920 B	920 B	✓
image-HASH.js gzip	5.48 kB	5.52 kB	⚠️ +38 B
index-HASH.js gzip	263 B	263 B	✓
link-HASH.js gzip	2.26 kB	2.32 kB	⚠️ +58 B
routerDirect..HASH.js gzip	320 B	320 B	✓
script-HASH.js gzip	387 B	387 B	✓
withRouter-HASH.js gzip	319 B	319 B	✓
85e02e95b279..7e3.css gzip	107 B	107 B	✓
Overall change	15.6 kB	15.7 kB	⚠️ +96 B

Client Build Manifests

	vercel/next.js canary	SukkaW/next.js fix-issue-35286	Change
_buildManifest.js gzip	460 B	460 B	✓
Overall change	460 B	460 B	✓

Rendered Page Sizes

	vercel/next.js canary	SukkaW/next.js fix-issue-35286	Change
index.html gzip	532 B	532 B	✓
link.html gzip	545 B	545 B	✓
withRouter.html gzip	525 B	525 B	✓
Overall change	1.6 kB	1.6 kB	✓

Diffs

Diff for _buildManifest.js

@@ -12,8 +12,8 @@ self.__BUILD_MANIFEST = {
   ],
   "/head": ["static\u002Fchunks\u002Fpages\u002Fhead-96a5d6ed07cf5a83.js"],
   "/hooks": ["static\u002Fchunks\u002Fpages\u002Fhooks-9dfe734f583d4926.js"],
-  "/image": ["static\u002Fchunks\u002Fpages\u002Fimage-3ea54d03e8ed2e01.js"],
-  "/link": ["static\u002Fchunks\u002Fpages\u002Flink-a96e4060bca1ccfe.js"],
+  "/image": ["static\u002Fchunks\u002Fpages\u002Fimage-3503c757a30d7190.js"],
+  "/link": ["static\u002Fchunks\u002Fpages\u002Flink-a72b8ab130103a8f.js"],
   "/routerDirect": [
     "static\u002Fchunks\u002Fpages\u002FrouterDirect-cb15c8ac2322bf32.js"
   ],

Diff for image-HASH.js

@@ -253,10 +253,11 @@
               rootMargin: lazyBoundary,
               disabled: !isLazy
             }),
-            2
+            3
           ),
           setIntersection = ref1[0],
-          isIntersected = ref1[1];
+          isIntersected = ref1[1],
+          resetIntersected = ref1[2];
         var isVisible = !isLazy || isIntersected;
         var wrapperStyle = {
           boxSizing: "border-box",
@@ -404,6 +405,7 @@
           _obj);
         var useLayoutEffect = false ? 0 : _react.default.useLayoutEffect;
         var onLoadingCompleteRef = (0, _react).useRef(onLoadingComplete);
+        var previousImageSrc = (0, _react).useRef(src);
         var imgRef = (0, _react).useRef(null);
         (0, _react).useEffect(
           function() {
@@ -413,9 +415,13 @@
         );
         useLayoutEffect(
           function() {
+            if (previousImageSrc.current !== src) {
+              resetIntersected();
+              previousImageSrc.current = src;
+            }
             setIntersection(imgRef.current);
           },
-          [setIntersection]
+          [setIntersection, resetIntersected, src]
         );
         (0, _react).useEffect(
           function() {
@@ -1157,6 +1163,9 @@
           },
           [isDisabled, root, rootMargin, visible]
         );
+        var resetVisible = (0, _react).useCallback(function() {
+          setVisible(false);
+        }, []);
         (0, _react).useEffect(
           function() {
             if (!hasIntersectionObserver) {
@@ -1181,7 +1190,7 @@
           },
           [rootRef]
         );
-        return [setRef, visible];
+        return [setRef, visible, resetVisible];
       }
       function observe(element, callback, options) {
         var ref = createObserver(options),

Diff for link-HASH.js

@@ -198,6 +198,8 @@
           ),
           href = ref2.href,
           as = ref2.as;
+        var previousHref = _react.default.useRef(href);
+        var previousAs = _react.default.useRef(as);
         var children = props.children,
           replace = props.replace,
           shallow = props.shallow,
@@ -221,12 +223,19 @@
             (0, _useIntersection).useIntersection({
               rootMargin: "200px"
             }),
-            2
+            3
           ),
           setIntersectionRef = ref1[0],
-          isVisible = ref1[1];
+          isVisible = ref1[1],
+          resetVisible = ref1[2];
         var setRef = _react.default.useCallback(
           function(el) {
+            // Before the link getting observed, check if visible state need to be reset
+            if (previousAs.current !== as || previousHref.current !== href) {
+              resetVisible();
+              previousAs.current = as;
+              previousHref.current = href;
+            }
             setIntersectionRef(el);
             if (childRef) {
               if (typeof childRef === "function") childRef(el);
@@ -235,7 +244,7 @@
               }
             }
           },
-          [childRef, setIntersectionRef]
+          [as, childRef, href, resetVisible, setIntersectionRef]
         );
         _react.default.useEffect(
           function() {
@@ -434,6 +443,9 @@
           },
           [isDisabled, root, rootMargin, visible]
         );
+        var resetVisible = (0, _react).useCallback(function() {
+          setVisible(false);
+        }, []);
         (0, _react).useEffect(
           function() {
             if (!hasIntersectionObserver) {
@@ -458,7 +470,7 @@
           },
           [rootRef]
         );
-        return [setRef, visible];
+        return [setRef, visible, resetVisible];
       }
       function observe(element, callback, options) {
         var ref = createObserver(options),

Diff for link.html

@@ -27,7 +27,7 @@
       defer=""
     ></script>
     <script
-      src="/_next/static/chunks/pages/link-a96e4060bca1ccfe.js"
+      src="/_next/static/chunks/pages/link-a72b8ab130103a8f.js"
       defer=""
     ></script>
     <script src="/_next/static/BUILD_ID/_buildManifest.js" defer=""></script>

Default Build with SWC (Increase detected ⚠️)

General Overall increase ⚠️

	vercel/next.js canary	SukkaW/next.js fix-issue-35286	Change
buildDuration	20.7s	21.4s	⚠️ +686ms
buildDurationCached	7.1s	7.3s	⚠️ +251ms
nodeModulesSize	477 MB	477 MB	⚠️ +1.61 kB

Page Load Tests Overall increase ✓

	vercel/next.js canary	SukkaW/next.js fix-issue-35286	Change
/ failed reqs	0	0	✓
/ total time (seconds)	3.659	3.645	-0.01
/ avg req/sec	683.28	685.84	+2.56
/error-in-render failed reqs	0	0	✓
/error-in-render total time (seconds)	1.628	1.58	-0.05
/error-in-render avg req/sec	1536.09	1582.04	+45.95

Client Bundles (main, webpack)

	vercel/next.js canary	SukkaW/next.js fix-issue-35286	Change
925.HASH.js gzip	178 B	178 B	✓
framework-HASH.js gzip	42.3 kB	42.3 kB	✓
main-HASH.js gzip	28.8 kB	28.8 kB	✓
webpack-HASH.js gzip	1.45 kB	1.45 kB	✓
Overall change	72.7 kB	72.7 kB	✓

Legacy Client Bundles (polyfills)

	vercel/next.js canary	SukkaW/next.js fix-issue-35286	Change
polyfills-HASH.js gzip	31 kB	31 kB	✓
Overall change	31 kB	31 kB	✓

Client Pages Overall increase ⚠️

	vercel/next.js canary	SukkaW/next.js fix-issue-35286	Change
_app-HASH.js gzip	1.35 kB	1.35 kB	✓
_error-HASH.js gzip	179 B	179 B	✓
amp-HASH.js gzip	313 B	313 B	✓
css-HASH.js gzip	325 B	325 B	✓
dynamic-HASH.js gzip	3.03 kB	3.03 kB	✓
head-HASH.js gzip	351 B	351 B	✓
hooks-HASH.js gzip	921 B	921 B	✓
image-HASH.js gzip	5.59 kB	5.63 kB	⚠️ +40 B
index-HASH.js gzip	261 B	261 B	✓
link-HASH.js gzip	2.33 kB	2.38 kB	⚠️ +53 B
routerDirect..HASH.js gzip	322 B	322 B	✓
script-HASH.js gzip	388 B	388 B	✓
withRouter-HASH.js gzip	317 B	317 B	✓
85e02e95b279..7e3.css gzip	107 B	107 B	✓
Overall change	15.8 kB	15.9 kB	⚠️ +93 B

Client Build Manifests Overall increase ⚠️

	vercel/next.js canary	SukkaW/next.js fix-issue-35286	Change
_buildManifest.js gzip	459 B	460 B	⚠️ +1 B
Overall change	459 B	460 B	⚠️ +1 B

Rendered Page Sizes Overall decrease ✓

	vercel/next.js canary	SukkaW/next.js fix-issue-35286	Change
index.html gzip	531 B	531 B	✓
link.html gzip	545 B	544 B	-1 B
withRouter.html gzip	526 B	526 B	✓
Overall change	1.6 kB	1.6 kB	-1 B

Diffs

Diff for _buildManifest.js

@@ -12,8 +12,8 @@ self.__BUILD_MANIFEST = {
   ],
   "/head": ["static\u002Fchunks\u002Fpages\u002Fhead-96a5d6ed07cf5a83.js"],
   "/hooks": ["static\u002Fchunks\u002Fpages\u002Fhooks-9dfe734f583d4926.js"],
-  "/image": ["static\u002Fchunks\u002Fpages\u002Fimage-3ea54d03e8ed2e01.js"],
-  "/link": ["static\u002Fchunks\u002Fpages\u002Flink-a96e4060bca1ccfe.js"],
+  "/image": ["static\u002Fchunks\u002Fpages\u002Fimage-3503c757a30d7190.js"],
+  "/link": ["static\u002Fchunks\u002Fpages\u002Flink-a72b8ab130103a8f.js"],
   "/routerDirect": [
     "static\u002Fchunks\u002Fpages\u002FrouterDirect-cb15c8ac2322bf32.js"
   ],

Diff for image-HASH.js

@@ -253,10 +253,11 @@
               rootMargin: lazyBoundary,
               disabled: !isLazy
             }),
-            2
+            3
           ),
           setIntersection = ref1[0],
-          isIntersected = ref1[1];
+          isIntersected = ref1[1],
+          resetIntersected = ref1[2];
         var isVisible = !isLazy || isIntersected;
         var wrapperStyle = {
           boxSizing: "border-box",
@@ -404,6 +405,7 @@
           _obj);
         var useLayoutEffect = false ? 0 : _react.default.useLayoutEffect;
         var onLoadingCompleteRef = (0, _react).useRef(onLoadingComplete);
+        var previousImageSrc = (0, _react).useRef(src);
         var imgRef = (0, _react).useRef(null);
         (0, _react).useEffect(
           function() {
@@ -413,9 +415,13 @@
         );
         useLayoutEffect(
           function() {
+            if (previousImageSrc.current !== src) {
+              resetIntersected();
+              previousImageSrc.current = src;
+            }
             setIntersection(imgRef.current);
           },
-          [setIntersection]
+          [setIntersection, resetIntersected, src]
         );
         (0, _react).useEffect(
           function() {
@@ -1157,6 +1163,9 @@
           },
           [isDisabled, root, rootMargin, visible]
         );
+        var resetVisible = (0, _react).useCallback(function() {
+          setVisible(false);
+        }, []);
         (0, _react).useEffect(
           function() {
             if (!hasIntersectionObserver) {
@@ -1181,7 +1190,7 @@
           },
           [rootRef]
         );
-        return [setRef, visible];
+        return [setRef, visible, resetVisible];
       }
       function observe(element, callback, options) {
         var ref = createObserver(options),

Diff for link-HASH.js

@@ -198,6 +198,8 @@
           ),
           href = ref2.href,
           as = ref2.as;
+        var previousHref = _react.default.useRef(href);
+        var previousAs = _react.default.useRef(as);
         var children = props.children,
           replace = props.replace,
           shallow = props.shallow,
@@ -221,12 +223,19 @@
             (0, _useIntersection).useIntersection({
               rootMargin: "200px"
             }),
-            2
+            3
           ),
           setIntersectionRef = ref1[0],
-          isVisible = ref1[1];
+          isVisible = ref1[1],
+          resetVisible = ref1[2];
         var setRef = _react.default.useCallback(
           function(el) {
+            // Before the link getting observed, check if visible state need to be reset
+            if (previousAs.current !== as || previousHref.current !== href) {
+              resetVisible();
+              previousAs.current = as;
+              previousHref.current = href;
+            }
             setIntersectionRef(el);
             if (childRef) {
               if (typeof childRef === "function") childRef(el);
@@ -235,7 +244,7 @@
               }
             }
           },
-          [childRef, setIntersectionRef]
+          [as, childRef, href, resetVisible, setIntersectionRef]
         );
         _react.default.useEffect(
           function() {
@@ -434,6 +443,9 @@
           },
           [isDisabled, root, rootMargin, visible]
         );
+        var resetVisible = (0, _react).useCallback(function() {
+          setVisible(false);
+        }, []);
         (0, _react).useEffect(
           function() {
             if (!hasIntersectionObserver) {
@@ -458,7 +470,7 @@
           },
           [rootRef]
         );
-        return [setRef, visible];
+        return [setRef, visible, resetVisible];
       }
       function observe(element, callback, options) {
         var ref = createObserver(options),

Diff for link.html

@@ -27,7 +27,7 @@
       defer=""
     ></script>
     <script
-      src="/_next/static/chunks/pages/link-a96e4060bca1ccfe.js"
+      src="/_next/static/chunks/pages/link-a72b8ab130103a8f.js"
       defer=""
     ></script>
     <script src="/_next/static/BUILD_ID/_buildManifest.js" defer=""></script>

Commit: 7657a90

styfle

Great work, thanks! 🎉

SukkaW requested review from timneutkens, ijjk, shuding, styfle and huozhi as code owners March 13, 2022 12:33

ijjk added the type: next label Mar 13, 2022

This comment has been minimized.

Sign in to view

SukkaW changed the title ~~fix(#35286): reset visible state when image src changed~~ fix(#35286): reset visible state when src/href changed Mar 13, 2022

SukkaW force-pushed the fix-issue-35286 branch from eb74a28 to e553c07 Compare March 14, 2022 11:45

This comment has been minimized.

Sign in to view

fix(vercel#35286): reset visible state when image src changed

bc85e4a

SukkaW force-pushed the fix-issue-35286 branch from e553c07 to 4c7d014 Compare March 15, 2022 04:24

This comment has been minimized.

Sign in to view

fix(vercel#35286): reset visible state when link href changed

96ae758

SukkaW force-pushed the fix-issue-35286 branch from 4c7d014 to 96ae758 Compare March 15, 2022 06:12

This comment has been minimized.

Sign in to view

balazsorban44 changed the title ~~fix(#35286): reset visible state when src/href changed~~ fix: reset visible state when src/href changed Mar 18, 2022

This comment has been minimized.

Sign in to view

styfle reviewed Apr 4, 2022

View reviewed changes

test(image): image with new src should be re-lazyloaded

f3443bd

SukkaW force-pushed the fix-issue-35286 branch from c85db73 to f3443bd Compare April 4, 2022 14:44

styfle reviewed Apr 4, 2022

View reviewed changes

test/integration/image-component/default/pages/lazy-src-change.js Show resolved Hide resolved

styfle reviewed Apr 4, 2022

View reviewed changes

test/integration/image-component/default/test/index.test.js Show resolved Hide resolved

styfle reviewed Apr 4, 2022

View reviewed changes

test/integration/image-component/default/test/index.test.js Show resolved Hide resolved

This comment has been minimized.

Sign in to view

styfle reviewed Apr 4, 2022

View reviewed changes

test/integration/image-component/default/pages/lazy-src-change.js Outdated Show resolved Hide resolved

This comment has been minimized.

Sign in to view

test: apply code suggestions from @styfle's review

091842a

SukkaW force-pushed the fix-issue-35286 branch from 2490820 to 091842a Compare April 4, 2022 15:48

Merge branch 'canary' into fix-issue-35286

d048e61

This comment has been minimized.

Sign in to view

test: incorrect condition of image load check

7657a90

styfle approved these changes Apr 4, 2022

View reviewed changes

styfle merged commit 081e7b0 into vercel:canary Apr 4, 2022

github-actions bot locked as resolved and limited conversation to collaborators May 5, 2022

SukkaW deleted the fix-issue-35286 branch October 24, 2023 08:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: reset visible state when src/href changed #35287

fix: reset visible state when src/href changed #35287

SukkaW commented Mar 13, 2022

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

SukkaW commented Mar 15, 2022

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

SukkaW commented Mar 23, 2022

styfle left a comment

SukkaW commented Apr 4, 2022

styfle commented Apr 4, 2022

This comment has been minimized.

This comment has been minimized.

ijjk commented Apr 4, 2022

This comment has been minimized.

ijjk commented Apr 4, 2022

Diffs

Diffs

styfle left a comment

fix: reset visible state when src/href changed #35287

fix: reset visible state when src/href changed #35287

Conversation

SukkaW commented Mar 13, 2022

Bug

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

SukkaW commented Mar 15, 2022

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

SukkaW commented Mar 23, 2022

styfle left a comment

Choose a reason for hiding this comment

SukkaW commented Apr 4, 2022

styfle commented Apr 4, 2022

This comment has been minimized.

This comment has been minimized.

ijjk commented Apr 4, 2022

Failing test suites

This comment has been minimized.

ijjk commented Apr 4, 2022

Stats from current PR

Diffs

Diffs

styfle left a comment

Choose a reason for hiding this comment