Normalize root path according to trailingSlash option in default next/image loader #21337 by fliptheweb · Pull Request #22453 · vercel/next.js

fliptheweb · 2021-02-23T11:12:38Z

Normalize path by trailing slash in next/images package for default image loader according to trailingSlash option in config.
Otherwise, Next.js does unnecessary 308 redirects to the normalized path.

Fixes: #21337

Tests:
I wonder to cover that case by integration test, but don't want to add complexity with one more mode with custom next.config just with trailingSlash: true option.
Any other option to do that?
Perhaps I could make a new mode, but only with a case that checks getSrc, out of runTests function?

docforcedev · 2021-07-15T23:30:53Z

@fliptheweb Any updates on this?

eprothro · 2021-09-16T21:20:07Z

We're experiencing multiple 308s using the Image component as well. Any estimate on timing for this fix?

balazsorban44 · 2022-03-03T15:15:55Z

Hi @fliptheweb, are you still interested in getting this PR merged?

There seems to be a merge conflict, and #22453 (comment) kindly asked to add some tests so we don't regress on it.

Let me know if you want to finish this off, or you are OK with someone else doing it! 👍

fliptheweb · 2022-03-03T16:54:00Z

@balazsorban44 got it, gonna update the PR till next monday. Thank you for notifying me.

…n in defaul loader

fliptheweb · 2022-03-07T14:18:58Z

@balazsorban44 Balázs, please check it out😇

styfle

Great, thanks!

ijjk · 2022-03-07T17:48:52Z

Stats from current PR

Default Build (Increase detected ⚠️)

General Overall increase ⚠️

	vercel/next.js canary	fliptheweb/next.js fix-next-image-with-trailing-slash	Change
buildDuration	16.1s	16.1s	-38ms
buildDurationCached	6.2s	6.1s	-50ms
nodeModulesSize	372 MB	372 MB	⚠️ +194 B

Page Load Tests Overall decrease ⚠️

	vercel/next.js canary	fliptheweb/next.js fix-next-image-with-trailing-slash	Change
/ failed reqs	0	0	✓
/ total time (seconds)	3.175	3.144	-0.03
/ avg req/sec	787.42	795.16	+7.74
/error-in-render failed reqs	0	0	✓
/error-in-render total time (seconds)	1.376	1.397	⚠️ +0.02
/error-in-render avg req/sec	1817.23	1789.15	⚠️ -28.08

Client Bundles (main, webpack)

	vercel/next.js canary	fliptheweb/next.js fix-next-image-with-trailing-slash	Change
925.HASH.js gzip	179 B	179 B	✓
framework-HASH.js gzip	42 kB	42 kB	✓
main-HASH.js gzip	27.9 kB	27.9 kB	✓
webpack-HASH.js gzip	1.44 kB	1.44 kB	✓
Overall change	71.5 kB	71.5 kB	✓

Legacy Client Bundles (polyfills)

	vercel/next.js canary	fliptheweb/next.js fix-next-image-with-trailing-slash	Change
polyfills-HASH.js gzip	31 kB	31 kB	✓
Overall change	31 kB	31 kB	✓

Client Pages Overall increase ⚠️

	vercel/next.js canary	fliptheweb/next.js fix-next-image-with-trailing-slash	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	2.57 kB	2.57 kB	✓
head-HASH.js gzip	351 B	351 B	✓
hooks-HASH.js gzip	920 B	920 B	✓
image-HASH.js gzip	5.06 kB	5.09 kB	⚠️ +31 B
index-HASH.js gzip	263 B	263 B	✓
link-HASH.js gzip	2.26 kB	2.26 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	14.8 kB	14.8 kB	⚠️ +31 B

Client Build Manifests

	vercel/next.js canary	fliptheweb/next.js fix-next-image-with-trailing-slash	Change
_buildManifest.js gzip	460 B	460 B	✓
Overall change	460 B	460 B	✓

Rendered Page Sizes

	vercel/next.js canary	fliptheweb/next.js fix-next-image-with-trailing-slash	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,7 +12,7 @@ self.__BUILD_MANIFEST = {
   ],
   "/head": ["static\u002Fchunks\u002Fpages\u002Fhead-35c32b80abf212d2.js"],
   "/hooks": ["static\u002Fchunks\u002Fpages\u002Fhooks-8642d114a09c62c5.js"],
-  "/image": ["static\u002Fchunks\u002Fpages\u002Fimage-3b0e98cd8174c02a.js"],
+  "/image": ["static\u002Fchunks\u002Fpages\u002Fimage-6052261a372c369a.js"],
   "/link": ["static\u002Fchunks\u002Fpages\u002Flink-0b3d72804dab6202.js"],
   "/routerDirect": [
     "static\u002Fchunks\u002Fpages\u002FrouterDirect-00527c3f2207a902.js"

Diff for image-HASH.js

@@ -133,6 +133,7 @@
       var _useIntersection = __webpack_require__(9246);
       var _imageConfigContext = __webpack_require__(8730);
       var _utils = __webpack_require__(670);
+      var _normalizeTrailingSlash = __webpack_require__(2700);
       function Image(_param) {
         var src = _param.src,
           sizes = _param.sizes,
@@ -910,7 +911,12 @@
           return src;
         }
         return ""
-          .concat(config.path, "?url=")
+          .concat(
+            (0, _normalizeTrailingSlash).normalizePathTrailingSlash(
+              config.path
+            ),
+            "?url="
+          )
           .concat(encodeURIComponent(src), "&w=")
           .concat(width, "&q=")
           .concat(quality || 75);

Default Build with SWC (Increase detected ⚠️)

General Overall increase ⚠️

	vercel/next.js canary	fliptheweb/next.js fix-next-image-with-trailing-slash	Change
buildDuration	19.3s	19.4s	⚠️ +95ms
buildDurationCached	6.3s	6.3s	⚠️ +82ms
nodeModulesSize	372 MB	372 MB	⚠️ +194 B

Page Load Tests Overall decrease ⚠️

	vercel/next.js canary	fliptheweb/next.js fix-next-image-with-trailing-slash	Change
/ failed reqs	0	0	✓
/ total time (seconds)	3.251	3.159	-0.09
/ avg req/sec	768.97	791.49	+22.52
/error-in-render failed reqs	0	0	✓
/error-in-render total time (seconds)	1.39	1.415	⚠️ +0.03
/error-in-render avg req/sec	1798.88	1766.2	⚠️ -32.68

Client Bundles (main, webpack)

	vercel/next.js canary	fliptheweb/next.js fix-next-image-with-trailing-slash	Change
925.HASH.js gzip	178 B	178 B	✓
framework-HASH.js gzip	42.3 kB	42.3 kB	✓
main-HASH.js gzip	28.2 kB	28.2 kB	✓
webpack-HASH.js gzip	1.45 kB	1.45 kB	✓
Overall change	72.1 kB	72.1 kB	✓

Legacy Client Bundles (polyfills)

	vercel/next.js canary	fliptheweb/next.js fix-next-image-with-trailing-slash	Change
polyfills-HASH.js gzip	31 kB	31 kB	✓
Overall change	31 kB	31 kB	✓

Client Pages Overall increase ⚠️

	vercel/next.js canary	fliptheweb/next.js fix-next-image-with-trailing-slash	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	324 B	324 B	✓
dynamic-HASH.js gzip	2.56 kB	2.56 kB	✓
head-HASH.js gzip	351 B	351 B	✓
hooks-HASH.js gzip	921 B	921 B	✓
image-HASH.js gzip	5.2 kB	5.23 kB	⚠️ +28 B
index-HASH.js gzip	261 B	261 B	✓
link-HASH.js gzip	2.33 kB	2.33 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	14.9 kB	15 kB	⚠️ +28 B

Client Build Manifests Overall increase ⚠️

	vercel/next.js canary	fliptheweb/next.js fix-next-image-with-trailing-slash	Change
_buildManifest.js gzip	458 B	459 B	⚠️ +1 B
Overall change	458 B	459 B	⚠️ +1 B

Rendered Page Sizes

	vercel/next.js canary	fliptheweb/next.js fix-next-image-with-trailing-slash	Change
index.html gzip	530 B	530 B	✓
link.html gzip	544 B	544 B	✓
withRouter.html gzip	526 B	526 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-35c32b80abf212d2.js"],
   "/hooks": ["static\u002Fchunks\u002Fpages\u002Fhooks-8642d114a09c62c5.js"],
-  "/image": ["static\u002Fchunks\u002Fpages\u002Fimage-3b0e98cd8174c02a.js"],
+  "/image": ["static\u002Fchunks\u002Fpages\u002Fimage-6052261a372c369a.js"],
   "/link": ["static\u002Fchunks\u002Fpages\u002Flink-0b3d72804dab6202.js"],
   "/routerDirect": [
     "static\u002Fchunks\u002Fpages\u002FrouterDirect-00527c3f2207a902.js"

Diff for image-HASH.js

@@ -133,6 +133,7 @@
       var _useIntersection = __webpack_require__(9246);
       var _imageConfigContext = __webpack_require__(8730);
       var _utils = __webpack_require__(670);
+      var _normalizeTrailingSlash = __webpack_require__(2700);
       function Image(_param) {
         var src = _param.src,
           sizes = _param.sizes,
@@ -910,7 +911,12 @@
           return src;
         }
         return ""
-          .concat(config.path, "?url=")
+          .concat(
+            (0, _normalizeTrailingSlash).normalizePathTrailingSlash(
+              config.path
+            ),
+            "?url="
+          )
           .concat(encodeURIComponent(src), "&w=")
           .concat(width, "&q=")
           .concat(quality || 75);

Commit: 1c981d4

fliptheweb · 2022-03-07T17:59:13Z

@balazsorban44 some tests failed, was it a flaky test?

balazsorban44 · 2022-03-07T18:09:53Z

Thanks for the heads up, let's follow https://github.com/vercel/next.js/actions/runs/1947085695

I thought I saw all tests passing but I might have overlooked.

balazsorban44 · 2022-03-08T14:14:29Z

It was just a flaky test, all good. 👍

fliptheweb requested review from Timer, divmain, ijjk, lfades and timneutkens as code owners February 23, 2021 11:12

ijjk added the type: next label Feb 23, 2021

This comment has been minimized.

Sign in to view

fliptheweb changed the title ~~Normalize root path according to trailingSlash option in default loader #21337~~ Normalize root path according to trailingSlash option in default next/image loader #21337 Feb 24, 2021

fliptheweb requested review from huozhi, padmaia, shuding and styfle as code owners September 17, 2021 04:42

styfle reviewed Sep 19, 2021

View reviewed changes

Comment thread packages/next/client/image.tsx Outdated

Timer removed request for Timer, divmain and lfades December 10, 2021 02:58

fliptheweb force-pushed the fix-next-image-with-trailing-slash branch from cacb0d5 to 54a0496 Compare March 7, 2022 14:14

fliptheweb added 2 commits March 7, 2022 17:16

fix(next/image): normalize root path according to trailingSlash optio…

788a3d1

…n in defaul loader

test(image-optimizer): trailingSlash support

d23b27f

fliptheweb force-pushed the fix-next-image-with-trailing-slash branch from 54a0496 to d23b27f Compare March 7, 2022 14:16

This comment has been minimized.

Sign in to view

chore(next/image): prettier

76b9b39

styfle approved these changes Mar 7, 2022

View reviewed changes

This comment has been minimized.

Sign in to view

ijjk approved these changes Mar 7, 2022

View reviewed changes

Merge branch 'canary' into fix-next-image-with-trailing-slash

1c981d4

balazsorban44 merged commit 0e5e888 into vercel:canary Mar 7, 2022

fliptheweb deleted the fix-next-image-with-trailing-slash branch March 9, 2022 20:30

github-actions Bot locked as resolved and limited conversation to collaborators Apr 9, 2022

Conversation

fliptheweb commented Feb 23, 2021

Uh oh!

This comment has been minimized.

docforcedev commented Jul 15, 2021

Uh oh!

eprothro commented Sep 16, 2021

Uh oh!

Uh oh!

balazsorban44 commented Mar 3, 2022

Uh oh!

fliptheweb commented Mar 3, 2022

Uh oh!

fliptheweb commented Mar 7, 2022

Uh oh!

This comment has been minimized.

styfle left a comment

Choose a reason for hiding this comment

Uh oh!

This comment has been minimized.

ijjk commented Mar 7, 2022

Stats from current PR

Diffs

Diffs

Uh oh!

fliptheweb commented Mar 7, 2022

Uh oh!

balazsorban44 commented Mar 7, 2022

Uh oh!

balazsorban44 commented Mar 8, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants