Fix leaking internal config to user-defined `loader` prop in `next/image` #36013

styfle · 2022-04-08T16:04:57Z

The config was changed from a global variable to a function parameter of the loader() function in PR #33559 as an implementation detail, not as a public API. It was not meant to be used by the end user which is why it was undocumented.

This config is meant for the default Image Optimization API. Since the loader prop bypasses the default Image Optimization API in favor of a custom function, that config is no longer be relevant because the function can implement the optimization url however it desires.

Fixes The config arg should not be passed to user-defined loader function in `next/image #35115

…age`

ijjk · 2022-04-08T17:07:38Z

Failing test suites

Commit: 998b3af

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

Image Component Tests > SSR Image Component Tests > should automatically load images if observer does not exist

Expand output

● Image Component Tests › SSR Image Component Tests › should automatically load images if observer does not exist

expect(received).toBe(expected) // Object.is equality

Expected: "https://example.vercel.sh/success/foo.jpg?width=300"
Received: "https://example.vercel.sh/success/foo.jpg?width=1024"

  322 |       expect(
  323 |         await browser.elementById('loader-prop-img').getAttribute('src')
> 324 |       ).toBe('https://example.vercel.sh/success/foo.jpg?width=300')
      |         ^
  325 |       expect(
  326 |         await browser.elementById('lazy-no-observer').getAttribute('srcset')
  327 |       ).toBe(

  at Object.<anonymous> (integration/image-component/basic/test/index.test.js:324:9)

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

packages/next/client/image.tsx

ijjk · 2022-04-08T22:19:24Z

Stats from current PR

Default Build (Increase detected ⚠️)

General Overall increase ⚠️

	vercel/next.js canary	vercel/next.js fix-custom-loader-prop-remove-config	Change
buildDuration	15.2s	15.1s	-83ms
buildDurationCached	6s	6s	⚠️ +76ms
nodeModulesSize	478 MB	478 MB	⚠️ +946 B

Page Load Tests Overall increase ✓

	vercel/next.js canary	vercel/next.js fix-custom-loader-prop-remove-config	Change
/ failed reqs	0	0	✓
/ total time (seconds)	3.035	3.053	⚠️ +0.02
/ avg req/sec	823.74	818.91	⚠️ -4.83
/error-in-render failed reqs	0	0	✓
/error-in-render total time (seconds)	1.155	1.149	-0.01
/error-in-render avg req/sec	2165.19	2175.89	+10.7

Client Bundles (main, webpack)

	vercel/next.js canary	vercel/next.js fix-custom-loader-prop-remove-config	Change
925.HASH.js gzip	179 B	179 B	✓
framework-HASH.js gzip	42 kB	42 kB	✓
main-HASH.js gzip	28 kB	28 kB	✓
webpack-HASH.js gzip	1.44 kB	1.44 kB	✓
Overall change	71.7 kB	71.7 kB	✓

Legacy Client Bundles (polyfills)

	vercel/next.js canary	vercel/next.js fix-custom-loader-prop-remove-config	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 fix-custom-loader-prop-remove-config	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.64 kB	5.68 kB	⚠️ +41 B
index-HASH.js gzip	263 B	263 B	✓
link-HASH.js gzip	2.32 kB	2.32 kB	✓
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.9 kB	15.9 kB	⚠️ +41 B

Client Build Manifests Overall increase ⚠️

	vercel/next.js canary	vercel/next.js fix-custom-loader-prop-remove-config	Change
_buildManifest.js gzip	459 B	460 B	⚠️ +1 B
Overall change	459 B	460 B	⚠️ +1 B

Rendered Page Sizes

	vercel/next.js canary	vercel/next.js fix-custom-loader-prop-remove-config	Change
index.html gzip	531 B	531 B	✓
link.html gzip	544 B	544 B	✓
withRouter.html gzip	524 B	524 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-96a5d6ed07cf5a83.js"],
   "/hooks": ["static\u002Fchunks\u002Fpages\u002Fhooks-9dfe734f583d4926.js"],
-  "/image": ["static\u002Fchunks\u002Fpages\u002Fimage-f39504b2095f9b7d.js"],
+  "/image": ["static\u002Fchunks\u002Fpages\u002Fimage-0e5a6c840e535a1d.js"],
   "/link": ["static\u002Fchunks\u002Fpages\u002Flink-a72b8ab130103a8f.js"],
   "/routerDirect": [
     "static\u002Fchunks\u002Fpages\u002FrouterDirect-cb15c8ac2322bf32.js"

Diff for image-HASH.js

@@ -155,8 +155,6 @@
           objectPosition = _param.objectPosition,
           onLoadingComplete = _param.onLoadingComplete,
           onError = _param.onError,
-          _loader = _param.loader,
-          loader = _loader === void 0 ? defaultImageLoader : _loader,
           _placeholder = _param.placeholder,
           placeholder = _placeholder === void 0 ? "empty" : _placeholder,
           blurDataURL = _param.blurDataURL,
@@ -177,7 +175,6 @@
             "objectPosition",
             "onLoadingComplete",
             "onError",
-            "loader",
             "placeholder",
             "blurDataURL"
           ]);
@@ -208,8 +205,26 @@
         if ("layout" in rest) {
           // Override default layout if the user specified one:
           if (rest.layout) layout = rest.layout;
-          // Remove property so it's not spread into image:
-          delete rest["layout"];
+          // Remove property so it's not spread on <img>:
+          delete rest.layout;
+        }
+        var loader = defaultImageLoader;
+        if ("loader" in rest) {
+          if (rest.loader) {
+            var customImageLoader = rest.loader;
+            var _tmp;
+            (_tmp = function(obj) {
+              var _ = obj.config,
+                opts = _objectWithoutProperties(obj, ["config"]);
+              // The config object is internal only so we must
+              // not pass it to the user-defined loader()
+              return customImageLoader(opts);
+            }),
+              (loader = _tmp),
+              _tmp;
+          }
+          // Remove property so it's not spread on <img>
+          delete rest.loader;
         }
         var staticSrc = "";
         if (isStaticImport(src)) {

Default Build with SWC (Increase detected ⚠️)

General Overall increase ⚠️

	vercel/next.js canary	vercel/next.js fix-custom-loader-prop-remove-config	Change
buildDuration	17.9s	17.9s	⚠️ +17ms
buildDurationCached	6s	6s	⚠️ +7ms
nodeModulesSize	478 MB	478 MB	⚠️ +946 B

Page Load Tests Overall decrease ⚠️

	vercel/next.js canary	vercel/next.js fix-custom-loader-prop-remove-config	Change
/ failed reqs	0	0	✓
/ total time (seconds)	3.024	3.035	⚠️ +0.01
/ avg req/sec	826.73	823.83	⚠️ -2.9
/error-in-render failed reqs	0	0	✓
/error-in-render total time (seconds)	1.148	1.168	⚠️ +0.02
/error-in-render avg req/sec	2178.25	2140.71	⚠️ -37.54

Client Bundles (main, webpack)

	vercel/next.js canary	vercel/next.js fix-custom-loader-prop-remove-config	Change
925.HASH.js gzip	178 B	178 B	✓
framework-HASH.js gzip	42.3 kB	42.3 kB	✓
main-HASH.js gzip	28.3 kB	28.3 kB	✓
webpack-HASH.js gzip	1.45 kB	1.45 kB	✓
Overall change	72.2 kB	72.2 kB	✓

Legacy Client Bundles (polyfills)

	vercel/next.js canary	vercel/next.js fix-custom-loader-prop-remove-config	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 fix-custom-loader-prop-remove-config	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.7 kB	5.74 kB	⚠️ +35 B
index-HASH.js gzip	261 B	261 B	✓
link-HASH.js gzip	2.38 kB	2.38 kB	✓
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.9 kB	16 kB	⚠️ +35 B

Client Build Manifests

	vercel/next.js canary	vercel/next.js fix-custom-loader-prop-remove-config	Change
_buildManifest.js gzip	459 B	459 B	✓
Overall change	459 B	459 B	✓

Rendered Page Sizes

	vercel/next.js canary	vercel/next.js fix-custom-loader-prop-remove-config	Change
index.html gzip	531 B	531 B	✓
link.html gzip	544 B	544 B	✓
withRouter.html gzip	527 B	527 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-96a5d6ed07cf5a83.js"],
   "/hooks": ["static\u002Fchunks\u002Fpages\u002Fhooks-9dfe734f583d4926.js"],
-  "/image": ["static\u002Fchunks\u002Fpages\u002Fimage-f39504b2095f9b7d.js"],
+  "/image": ["static\u002Fchunks\u002Fpages\u002Fimage-0e5a6c840e535a1d.js"],
   "/link": ["static\u002Fchunks\u002Fpages\u002Flink-a72b8ab130103a8f.js"],
   "/routerDirect": [
     "static\u002Fchunks\u002Fpages\u002FrouterDirect-cb15c8ac2322bf32.js"

Diff for image-HASH.js

@@ -155,8 +155,6 @@
           objectPosition = _param.objectPosition,
           onLoadingComplete = _param.onLoadingComplete,
           onError = _param.onError,
-          _loader = _param.loader,
-          loader = _loader === void 0 ? defaultImageLoader : _loader,
           _placeholder = _param.placeholder,
           placeholder = _placeholder === void 0 ? "empty" : _placeholder,
           blurDataURL = _param.blurDataURL,
@@ -177,7 +175,6 @@
             "objectPosition",
             "onLoadingComplete",
             "onError",
-            "loader",
             "placeholder",
             "blurDataURL"
           ]);
@@ -208,8 +205,26 @@
         if ("layout" in rest) {
           // Override default layout if the user specified one:
           if (rest.layout) layout = rest.layout;
-          // Remove property so it's not spread into image:
-          delete rest["layout"];
+          // Remove property so it's not spread on <img>:
+          delete rest.layout;
+        }
+        var loader = defaultImageLoader;
+        if ("loader" in rest) {
+          if (rest.loader) {
+            var customImageLoader = rest.loader;
+            var _tmp;
+            (_tmp = function(obj) {
+              var _ = obj.config,
+                opts = _objectWithoutProperties(obj, ["config"]);
+              // The config object is internal only so we must
+              // not pass it to the user-defined loader()
+              return customImageLoader(opts);
+            }),
+              (loader = _tmp),
+              _tmp;
+          }
+          // Remove property so it's not spread on <img>
+          delete rest.loader;
         }
         var staticSrc = "";
         if (isStaticImport(src)) {

Commit: b41388d

…age` (vercel#36013) The `config` was changed from a global variable to a function parameter of the `loader()` function in PR vercel#33559 as an implementation detail, not as a public API. It was not meant to be used by the end user which is why it was [undocumented](https://nextjs.org/docs/api-reference/next/image#loader). This config is meant for the default Image Optimization API. Since the `loader` prop bypasses the default Image Optimization API in favor of a custom function, that config is no longer be relevant because the function can implement the optimization url however it desires. - Fixes vercel#35115

TheThirdRace · 2022-04-24T16:10:01Z

This decision costed me 1 hour of refactoring...

The values in next.config.js definitely can be used with a custom loader.

NextJs image solution consist of 2 distinct parts:

the server part, which is in charge of optimizing the images and acts as a SERVICE
the client part is in charge of asking the server part what it needs and display the image efficiently

There are tons of problems with the client part. Problems the community is asking fixes for more than a year...

While a lot of improvements have been done with the client part, it still has glaring issues that can warrant a complete replacement of next/image as a display solution.

By making next.config.js image values private to NextJs, it only makes it more difficult for people to replace next/image as a display solution.

It's like NextJs never anticipated that we could use NextJs image optimization service, the server part, with a better fitting display solution for our specific needs...

In any case, I just wanted to point out these kind of decisions are not improving flexibility, it just makes it frustrating to work with NextJs Image solution.

LZL0 · 2022-04-24T17:07:38Z

next/image desperately lacks out-of-the-box support for modern CSS frameworks like TailwindCSS. We need to use workarounds and define static widths and heights. I feel that instead of assisting the developer, it's just a chore to work with it most of the time.

styfle · 2022-04-25T17:15:40Z

Thanks for the feedback!

Please see my comment #35115 (comment) as well as #35115 (comment).

Fix leaking internal config to user-defined loader prop in `next/im…

e46a97b

…age`

ijjk added created-by: Next.js team PRs by the Next.js team type: next labels Apr 8, 2022

styfle changed the title ~~Fix leaking internal config to user-defined loader prop in `next/im…~~ Fix leaking internal config to user-defined loader prop in next/image Apr 8, 2022

This comment has been minimized.

Sign in to view

Fix test

37f8f17

This comment has been minimized.

Sign in to view

Fix when loader = undefined

998b3af

This comment has been minimized.

Sign in to view

styfle added 2 commits April 8, 2022 17:37

Fix test

9ecf9e1

Merge branch 'canary' into fix-custom-loader-prop-remove-config

cf72a3f

styfle marked this pull request as ready for review April 8, 2022 21:39

styfle requested review from timneutkens, ijjk, shuding and huozhi as code owners April 8, 2022 21:39

ijjk reviewed Apr 8, 2022

View reviewed changes

packages/next/client/image.tsx Outdated Show resolved Hide resolved

Add new internal type ImageLoaderWithConfig

b41388d

styfle requested a review from ijjk April 8, 2022 22:03

ijjk approved these changes Apr 8, 2022

View reviewed changes

kodiakhq bot merged commit 0a04ab7 into canary Apr 8, 2022

kodiakhq bot deleted the fix-custom-loader-prop-remove-config branch April 8, 2022 22:19

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix leaking internal config to user-defined `loader` prop in `next/image` #36013

Fix leaking internal config to user-defined `loader` prop in `next/image` #36013

styfle commented Apr 8, 2022 •

edited

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

ijjk commented Apr 8, 2022

This comment has been minimized.

ijjk commented Apr 8, 2022

Diffs

Diffs

TheThirdRace commented Apr 24, 2022

LZL0 commented Apr 24, 2022

styfle commented Apr 25, 2022

Fix leaking internal config to user-defined loader prop in next/image #36013

Fix leaking internal config to user-defined loader prop in next/image #36013

Conversation

styfle commented Apr 8, 2022 • edited

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

ijjk commented Apr 8, 2022

Failing test suites

This comment has been minimized.

ijjk commented Apr 8, 2022

Stats from current PR

Diffs

Diffs

TheThirdRace commented Apr 24, 2022

LZL0 commented Apr 24, 2022

styfle commented Apr 25, 2022

Fix leaking internal config to user-defined `loader` prop in `next/image` #36013

Fix leaking internal config to user-defined `loader` prop in `next/image` #36013

styfle commented Apr 8, 2022 •

edited