Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Change experimental layout=raw to use native img lazy loading #36985

Merged
merged 10 commits into from
May 18, 2022

Conversation

styfle
Copy link
Member

@styfle styfle commented May 17, 2022

This PR changes the experimental layout=raw images to use the native lazy loading behavior (as opposed to the IntersectionObserver).

This will (eventually) lead to smaller client bundles and faster image loading since there is no JS needed to load the image.

However, we'll lose the lazyRoot and lazyBoundary behavior since those are specific to the IntersectionObserver implementation.

@ijjk
Copy link
Member

ijjk commented May 17, 2022

Stats from current PR

Default Build (Increase detected ⚠️)
General Overall increase ⚠️
vercel/next.js canary vercel/next.js layout-raw-native-lazy-loading Change
buildDuration 17.7s 16.8s -883ms
buildDurationCached 6.7s 6.7s -12ms
nodeModulesSize 478 MB 478 MB ⚠️ +1.42 kB
Page Load Tests Overall increase ✓
vercel/next.js canary vercel/next.js layout-raw-native-lazy-loading Change
/ failed reqs 0 0
/ total time (seconds) 4.655 4.688 ⚠️ +0.03
/ avg req/sec 537.09 533.32 ⚠️ -3.77
/error-in-render failed reqs 0 0
/error-in-render total time (seconds) 1.877 1.772 -0.1
/error-in-render avg req/sec 1331.94 1410.78 +78.84
Client Bundles (main, webpack)
vercel/next.js canary vercel/next.js layout-raw-native-lazy-loading Change
925.HASH.js gzip 179 B 179 B
framework-HASH.js gzip 42 kB 42 kB
main-HASH.js gzip 28.8 kB 28.8 kB
webpack-HASH.js gzip 1.44 kB 1.44 kB
Overall change 72.4 kB 72.4 kB
Legacy Client Bundles (polyfills)
vercel/next.js canary vercel/next.js layout-raw-native-lazy-loading Change
polyfills-HASH.js gzip 31 kB 31 kB
Overall change 31 kB 31 kB
Client Pages Overall increase ⚠️
vercel/next.js canary vercel/next.js layout-raw-native-lazy-loading Change
_app-HASH.js gzip 1.36 kB 1.36 kB
_error-HASH.js gzip 193 B 193 B
amp-HASH.js gzip 308 B 308 B
css-HASH.js gzip 327 B 327 B
dynamic-HASH.js gzip 2.71 kB 2.71 kB
head-HASH.js gzip 359 B 359 B
hooks-HASH.js gzip 920 B 920 B
image-HASH.js gzip 5.72 kB 5.73 kB ⚠️ +8 B
index-HASH.js gzip 263 B 263 B
link-HASH.js gzip 2.65 kB 2.65 kB
routerDirect..HASH.js gzip 320 B 320 B
script-HASH.js gzip 391 B 391 B
withRouter-HASH.js gzip 318 B 318 B
85e02e95b279..7e3.css gzip 107 B 107 B
Overall change 16 kB 16 kB ⚠️ +8 B
Client Build Manifests Overall decrease ✓
vercel/next.js canary vercel/next.js layout-raw-native-lazy-loading Change
_buildManifest.js gzip 460 B 458 B -2 B
Overall change 460 B 458 B -2 B
Rendered Page Sizes
vercel/next.js canary vercel/next.js layout-raw-native-lazy-loading Change
index.html gzip 531 B 531 B
link.html gzip 545 B 545 B
withRouter.html gzip 528 B 528 B
Overall change 1.6 kB 1.6 kB

Diffs

Diff for _buildManifest.js
@@ -12,7 +12,7 @@ self.__BUILD_MANIFEST = {
   ],
   "/head": ["static\u002Fchunks\u002Fpages\u002Fhead-544bb68363445a0e.js"],
   "/hooks": ["static\u002Fchunks\u002Fpages\u002Fhooks-c1372eeb4916d32c.js"],
-  "/image": ["static\u002Fchunks\u002Fpages\u002Fimage-e9333123dfdaa509.js"],
+  "/image": ["static\u002Fchunks\u002Fpages\u002Fimage-478ba29a386d0870.js"],
   "/link": ["static\u002Fchunks\u002Fpages\u002Flink-14cc686179ced0b5.js"],
   "/routerDirect": [
     "static\u002Fchunks\u002Fpages\u002FrouterDirect-113a7082ae16fbcb.js"
Diff for image-HASH.js
@@ -139,8 +139,7 @@
           loading = _param.loading,
           _lazyRoot = _param.lazyRoot,
           lazyRoot = _lazyRoot === void 0 ? null : _lazyRoot,
-          _lazyBoundary = _param.lazyBoundary,
-          lazyBoundary = _lazyBoundary === void 0 ? "200px" : _lazyBoundary,
+          lazyBoundary = _param.lazyBoundary,
           className = _param.className,
           quality = _param.quality,
           width = _param.width,
@@ -263,7 +262,7 @@
         var ref1 = _slicedToArray(
             (0, _useIntersection).useIntersection({
               rootRef: lazyRoot,
-              rootMargin: lazyBoundary,
+              rootMargin: lazyBoundary || "200px",
               disabled: !isLazy
             }),
             3
@@ -271,7 +270,7 @@
           setIntersection = ref1[0],
           isIntersected = ref1[1],
           resetIntersected = ref1[2];
-        var isVisible = !isLazy || isIntersected;
+        var isVisible = !isLazy || isIntersected || layout === "raw";
         var wrapperStyle = {
           boxSizing: "border-box",
           display: "block",
@@ -884,7 +883,8 @@
           blurStyle = _param.blurStyle,
           isLazy = _param.isLazy,
           placeholder = _param.placeholder,
-          loading = _param.loading,
+          _loading = _param.loading,
+          loading = _loading === void 0 ? "lazy" : _loading,
           srcString = _param.srcString,
           config = _param.config,
           unoptimized = _param.unoptimized,
@@ -937,6 +937,8 @@
                 decoding: "async",
                 "data-nimg": layout,
                 className: className,
+                // @ts-ignore - TODO: upgrade to `@types/react@17`
+                loading: layout === "raw" ? loading : undefined,
                 style: _objectSpread({}, imgStyle, blurStyle),
                 ref: (0, _react).useCallback(
                   function(img) {
@@ -1020,7 +1022,7 @@
                     style: imgStyle,
                     className: className,
                     // @ts-ignore - TODO: upgrade to `@types/react@17`
-                    loading: loading || "lazy"
+                    loading: loading
                   }
                 )
               )

Default Build with SWC (Increase detected ⚠️)
General Overall increase ⚠️
vercel/next.js canary vercel/next.js layout-raw-native-lazy-loading Change
buildDuration 19.6s 19.4s -215ms
buildDurationCached 6.6s 6.6s ⚠️ +27ms
nodeModulesSize 478 MB 478 MB ⚠️ +1.42 kB
Page Load Tests Overall increase ✓
vercel/next.js canary vercel/next.js layout-raw-native-lazy-loading Change
/ failed reqs 0 0
/ total time (seconds) 4.658 4.614 -0.04
/ avg req/sec 536.73 541.79 +5.06
/error-in-render failed reqs 0 0
/error-in-render total time (seconds) 1.786 1.725 -0.06
/error-in-render avg req/sec 1399.69 1449.41 +49.72
Client Bundles (main, webpack)
vercel/next.js canary vercel/next.js layout-raw-native-lazy-loading Change
925.HASH.js gzip 178 B 178 B
framework-HASH.js gzip 42.7 kB 42.7 kB
main-HASH.js gzip 29.2 kB 29.2 kB
webpack-HASH.js gzip 1.45 kB 1.45 kB
Overall change 73.5 kB 73.5 kB
Legacy Client Bundles (polyfills)
vercel/next.js canary vercel/next.js layout-raw-native-lazy-loading Change
polyfills-HASH.js gzip 31 kB 31 kB
Overall change 31 kB 31 kB
Client Pages Overall increase ⚠️
vercel/next.js canary vercel/next.js layout-raw-native-lazy-loading Change
_app-HASH.js gzip 1.35 kB 1.35 kB
_error-HASH.js gzip 179 B 179 B
amp-HASH.js gzip 311 B 311 B
css-HASH.js gzip 324 B 324 B
dynamic-HASH.js gzip 2.89 kB 2.89 kB
head-HASH.js gzip 357 B 357 B
hooks-HASH.js gzip 920 B 920 B
image-HASH.js gzip 5.81 kB 5.82 kB ⚠️ +4 B
index-HASH.js gzip 261 B 261 B
link-HASH.js gzip 2.78 kB 2.78 kB
routerDirect..HASH.js gzip 322 B 322 B
script-HASH.js gzip 392 B 392 B
withRouter-HASH.js gzip 317 B 317 B
85e02e95b279..7e3.css gzip 107 B 107 B
Overall change 16.3 kB 16.3 kB ⚠️ +4 B
Client Build Manifests
vercel/next.js canary vercel/next.js layout-raw-native-lazy-loading Change
_buildManifest.js gzip 457 B 457 B
Overall change 457 B 457 B
Rendered Page Sizes
vercel/next.js canary vercel/next.js layout-raw-native-lazy-loading Change
index.html gzip 531 B 531 B
link.html gzip 546 B 546 B
withRouter.html gzip 528 B 528 B
Overall change 1.6 kB 1.6 kB

Diffs

Diff for _buildManifest.js
@@ -12,7 +12,7 @@ self.__BUILD_MANIFEST = {
   ],
   "/head": ["static\u002Fchunks\u002Fpages\u002Fhead-544bb68363445a0e.js"],
   "/hooks": ["static\u002Fchunks\u002Fpages\u002Fhooks-c1372eeb4916d32c.js"],
-  "/image": ["static\u002Fchunks\u002Fpages\u002Fimage-e9333123dfdaa509.js"],
+  "/image": ["static\u002Fchunks\u002Fpages\u002Fimage-478ba29a386d0870.js"],
   "/link": ["static\u002Fchunks\u002Fpages\u002Flink-14cc686179ced0b5.js"],
   "/routerDirect": [
     "static\u002Fchunks\u002Fpages\u002FrouterDirect-113a7082ae16fbcb.js"
Diff for image-HASH.js
@@ -139,8 +139,7 @@
           loading = _param.loading,
           _lazyRoot = _param.lazyRoot,
           lazyRoot = _lazyRoot === void 0 ? null : _lazyRoot,
-          _lazyBoundary = _param.lazyBoundary,
-          lazyBoundary = _lazyBoundary === void 0 ? "200px" : _lazyBoundary,
+          lazyBoundary = _param.lazyBoundary,
           className = _param.className,
           quality = _param.quality,
           width = _param.width,
@@ -263,7 +262,7 @@
         var ref1 = _slicedToArray(
             (0, _useIntersection).useIntersection({
               rootRef: lazyRoot,
-              rootMargin: lazyBoundary,
+              rootMargin: lazyBoundary || "200px",
               disabled: !isLazy
             }),
             3
@@ -271,7 +270,7 @@
           setIntersection = ref1[0],
           isIntersected = ref1[1],
           resetIntersected = ref1[2];
-        var isVisible = !isLazy || isIntersected;
+        var isVisible = !isLazy || isIntersected || layout === "raw";
         var wrapperStyle = {
           boxSizing: "border-box",
           display: "block",
@@ -884,7 +883,8 @@
           blurStyle = _param.blurStyle,
           isLazy = _param.isLazy,
           placeholder = _param.placeholder,
-          loading = _param.loading,
+          _loading = _param.loading,
+          loading = _loading === void 0 ? "lazy" : _loading,
           srcString = _param.srcString,
           config = _param.config,
           unoptimized = _param.unoptimized,
@@ -937,6 +937,8 @@
                 decoding: "async",
                 "data-nimg": layout,
                 className: className,
+                // @ts-ignore - TODO: upgrade to `@types/react@17`
+                loading: layout === "raw" ? loading : undefined,
                 style: _objectSpread({}, imgStyle, blurStyle),
                 ref: (0, _react).useCallback(
                   function(img) {
@@ -1020,7 +1022,7 @@
                     style: imgStyle,
                     className: className,
                     // @ts-ignore - TODO: upgrade to `@types/react@17`
-                    loading: loading || "lazy"
+                    loading: loading
                   }
                 )
               )
Commit: 18b7b27

atcastle
atcastle previously approved these changes May 17, 2022
Copy link
Collaborator

@atcastle atcastle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good

@styfle styfle marked this pull request as ready for review May 17, 2022 21:00
@ijjk
Copy link
Member

ijjk commented May 17, 2022

Failing test suites

Commit: ee1d56c

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

  • Image Component Tests > dev mode > should lazy load layout=raw and placeholder=blur
  • Image Component Tests > server mode > should lazy load layout=raw and placeholder=blur
  • Image Component Tests > serverless mode > should lazy load layout=raw and placeholder=blur
Expand output

● Image Component Tests › dev mode › should lazy load layout=raw and placeholder=blur

expect(received).toMatch(expected)

Expected substring: "data:image/"
Received string:    "/_next/image?url=%2F_next%2Fstatic%2Fmedia%2Ftest.fab2915d.jpg&w=828&q=75"

  761 |
  762 |     // raw1
> 763 |     expect(await browser.elementById('raw1').getAttribute('src')).toMatch(
      |                                                                   ^
  764 |       'data:image/'
  765 |     )
  766 |     expect(await browser.elementById('raw1').getAttribute('srcset')).toBeNull()

  at Object.<anonymous> (integration/image-component/default/test/index.test.js:763:67)
      at runMicrotasks (<anonymous>)

● Image Component Tests › server mode › should lazy load layout=raw and placeholder=blur

expect(received).toMatch(expected)

Expected substring: "data:image/"
Received string:    "/_next/image?url=%2F_next%2Fstatic%2Fmedia%2Ftest.fab2915d.jpg&w=828&q=75"

  761 |
  762 |     // raw1
> 763 |     expect(await browser.elementById('raw1').getAttribute('src')).toMatch(
      |                                                                   ^
  764 |       'data:image/'
  765 |     )
  766 |     expect(await browser.elementById('raw1').getAttribute('srcset')).toBeNull()

  at Object.<anonymous> (integration/image-component/default/test/index.test.js:763:67)

● Image Component Tests › serverless mode › should lazy load layout=raw and placeholder=blur

expect(received).toMatch(expected)

Expected substring: "data:image/"
Received string:    "/_next/image?url=%2F_next%2Fstatic%2Fmedia%2Ftest.fab2915d.jpg&w=828&q=75"

  761 |
  762 |     // raw1
> 763 |     expect(await browser.elementById('raw1').getAttribute('src')).toMatch(
      |                                                                   ^
  764 |       'data:image/'
  765 |     )
  766 |     expect(await browser.elementById('raw1').getAttribute('srcset')).toBeNull()

  at Object.<anonymous> (integration/image-component/default/test/index.test.js:763:67)
      at runMicrotasks (<anonymous>)

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

@styfle styfle requested a review from atcastle May 18, 2022 20:58
Copy link
Member

@ijjk ijjk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good!

@kodiakhq kodiakhq bot merged commit 9f0024a into canary May 18, 2022
@kodiakhq kodiakhq bot deleted the layout-raw-native-lazy-loading branch May 18, 2022 21:05
@jakejarvis
Copy link
Contributor

@styfle, with this addition, will it be possible to set priority={true} on these at some point as well?

Completely agree with setting loading=lazy by default, but omitting this in specific cases would still be helpful, and having consistency with the existing <head> preloading behavior of the "classic" layouts would be great too! Looks like the raw image element is currently returned before reaching this priority condition:

{priority ? (
// Note how we omit the `href` attribute, as it would only be relevant
// for browsers that do not support `imagesrcset`, and in those cases
// it would likely cause the incorrect image to be preloaded.
//
// https://html.spec.whatwg.org/multipage/semantics.html#attr-link-imagesrcset
<Head>
<link
key={
'__nimg-' +
imgAttributes.src +
imgAttributes.srcSet +
imgAttributes.sizes
}
rel="preload"
as="image"
href={imgAttributes.srcSet ? undefined : imgAttributes.src}
{...linkProps}
/>
</Head>
) : null}

...but feel free to ignore if there's a reason for this :)

@atcastle
Copy link
Collaborator

@jakejarvis Could you clarify what you mean about the image element returning before reaching the priority condition? That's part of the JSX so it should get rendered any time the priority attribute is set to true on the image component. Or are you asking for setting priority = true by default? That wouldn't work super well with lazy loading on by default, because priority deactivates lazy loading.
The main idea behind priority is that you should use it on images which you think are likely to be prominent in the viewport when the page loads, in order to improve performance on metrics like LCP

@jakejarvis
Copy link
Contributor

@atcastle Sorry, I see why that was confusing! Agree with you about the default, and I completely misread the ternaries in the snippet I linked to, so that part can be ignored -- the preload tag is indeed in the head! But loading="lazy" is still set on the img tag even when priority is present, unless I'm missing something.

Here's a quick snippet from 12.1.7-canary.8 but let me know if a whole repro and a new issue would be helpful:

JSX:

<NextImage src={selfieJpg} layout="raw" width={50} height={50} priority />

HTML (note the very end):

<img srcset="/_next/image/?url=%2F_next%2Fstatic%2Fmedia%2Fselfie.7d009bf4.jpg&amp;w=64&amp;q=60 1x, /_next/image/?url=%2F_next%2Fstatic%2Fmedia%2Fselfie.7d009bf4.jpg&amp;w=128&amp;q=60 2x" src="/_next/image/?url=%2F_next%2Fstatic%2Fmedia%2Fselfie.7d009bf4.jpg&amp;w=128&amp;q=60" height="50" width="50" decoding="async" data-nimg="raw" loading="lazy">

@styfle
Copy link
Member Author

styfle commented Jun 1, 2022

@jakejarvis Thanks for pointing this out! I created PR #37381 with the fix 👍

@jakejarvis
Copy link
Contributor

Thanks so much @styfle!!

styfle added a commit that referenced this pull request Jun 4, 2022
Fixes a regression from #36985 which switched to native lazy loading but didn't account for the case when the user sets `priority` which changes the default behavior of `loading` prop.

See #36985 (comment)
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 4, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants