(next/image): Merge query string params in imgix loader #26719

theostrahlen · 2021-06-29T09:55:22Z

If the Image src url had existing query params, the imgix loader would simply append another query string with ? causing both query strings to break.

This PR adds a way to safely merge query strings if needed using URLSearchParams.

Bug

fixes next/image imagix breaks querystrings #26288
Integration tests added

styfle

Great work, thanks! 🎉

ijjk · 2021-07-06T10:04:09Z

Failing test suites

Commit: 4cde0c2

test/integration/create-next-app/index.test.js

create next app > empty directory
create next app > valid example
create next app > should support typescript flag
create next app > should allow example with GitHub URL
create next app > should allow example with GitHub URL and example-path
create next app > should use --example-path over the file path in the GitHub URL
create next app > should fall back to default template
create next app > should allow an example named default
create next app > should create a project in the current directory
create next app > should ask the user for a name for the project if none supplied
create next app > should use npm as the package manager on supplying --use-npm
create next app > should use npm as the package manager on supplying --use-npm with example

Expand output

● create next app › empty directory

expect(received).toBeTruthy()

Received: false

  52 |         expect(
  53 |           fs.existsSync(path.join(cwd, projectName, 'pages/index.js'))
> 54 |         ).toBeTruthy()
     |           ^
  55 |         expect(
  56 |           fs.existsSync(path.join(cwd, projectName, '.eslintrc'))
  57 |         ).toBeTruthy()

  at integration/create-next-app/index.test.js:54:11
  at usingTempDir (integration/create-next-app/index.test.js:20:5)
  at Object.<anonymous> (integration/create-next-app/index.test.js:44:7)

● create next app › valid example

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

Expected: true
Received: false

   97 |       expect(
   98 |         fs.existsSync(path.join(cwd, projectName, 'node_modules/next'))
>  99 |       ).toBe(true)
      |         ^
  100 |     })
  101 |   })
  102 |

  at integration/create-next-app/index.test.js:99:9
  at usingTempDir (integration/create-next-app/index.test.js:20:5)
  at Object.<anonymous> (integration/create-next-app/index.test.js:82:5)

● create next app › should support typescript flag

expect(received).toBeTruthy()

Received: false

  112 |       expect(
  113 |         fs.existsSync(path.join(cwd, projectName, 'pages/index.tsx'))
> 114 |       ).toBeTruthy()
      |         ^
  115 |       expect(
  116 |         fs.existsSync(path.join(cwd, projectName, 'pages/_app.tsx'))
  117 |       ).toBeTruthy()

  at integration/create-next-app/index.test.js:114:9
  at usingTempDir (integration/create-next-app/index.test.js:20:5)
  at Object.<anonymous> (integration/create-next-app/index.test.js:104:5)

● create next app › should allow example with GitHub URL

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

Expected: true
Received: false

  174 |       expect(
  175 |         fs.existsSync(path.join(cwd, projectName, 'node_modules/next'))
> 176 |       ).toBe(true)
      |         ^
  177 |     })
  178 |   })
  179 |

  at integration/create-next-app/index.test.js:176:9
  at usingTempDir (integration/create-next-app/index.test.js:20:5)
  at Object.<anonymous> (integration/create-next-app/index.test.js:155:5)

● create next app › should allow example with GitHub URL and example-path

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

Expected: true
Received: false

  200 |       expect(
  201 |         fs.existsSync(path.join(cwd, projectName, 'node_modules/next'))
> 202 |       ).toBe(true)
      |         ^
  203 |     })
  204 |   })
  205 |

  at integration/create-next-app/index.test.js:202:9
  at usingTempDir (integration/create-next-app/index.test.js:20:5)
  at Object.<anonymous> (integration/create-next-app/index.test.js:181:5)

● create next app › should use --example-path over the file path in the GitHub URL

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

Expected: true
Received: false

  232 |       expect(
  233 |         fs.existsSync(path.join(cwd, projectName, 'node_modules/next'))
> 234 |       ).toBe(true)
      |         ^
  235 |     })
  236 |   })
  237 |

  at integration/create-next-app/index.test.js:234:9
  at usingTempDir (integration/create-next-app/index.test.js:20:5)
  at Object.<anonymous> (integration/create-next-app/index.test.js:207:5)

● create next app › should fall back to default template

expect(received).toBeTruthy()

Received: false

  258 |         ]
  259 |         files.forEach((file) =>
> 260 |           expect(fs.existsSync(path.join(cwd, projectName, file))).toBeTruthy()
      |                                                                    ^
  261 |         )
  262 |       })
  263 |     })

  at forEach (integration/create-next-app/index.test.js:260:68)
      at Array.forEach (<anonymous>)
  at integration/create-next-app/index.test.js:259:15
  at usingTempDir (integration/create-next-app/index.test.js:20:5)
  at Object.<anonymous> (integration/create-next-app/index.test.js:242:7)

● create next app › should allow an example named default

expect(received).toBeTruthy()

Received: false

  275 |       expect(
  276 |         fs.existsSync(path.join(cwd, projectName, 'pages/index.js'))
> 277 |       ).toBeTruthy()
      |         ^
  278 |       // check we copied default `.gitignore`
  279 |       expect(
  280 |         fs.existsSync(path.join(cwd, projectName, '.gitignore'))

  at integration/create-next-app/index.test.js:277:9
  at usingTempDir (integration/create-next-app/index.test.js:20:5)
  at Object.<anonymous> (integration/create-next-app/index.test.js:267:5)

● create next app › should create a project in the current directory

expect(received).toBeTruthy()

Received: false

  326 |       ]
  327 |       files.forEach((file) =>
> 328 |         expect(fs.existsSync(path.join(cwd, file))).toBeTruthy()
      |                                                     ^
  329 |       )
  330 |     })
  331 |   })

  at forEach (integration/create-next-app/index.test.js:328:53)
      at Array.forEach (<anonymous>)
  at integration/create-next-app/index.test.js:327:13
  at usingTempDir (integration/create-next-app/index.test.js:20:5)
  at Object.<anonymous> (integration/create-next-app/index.test.js:316:5)

● create next app › should ask the user for a name for the project if none supplied

expect(received).toBeTruthy()

Received: false

  345 |       ]
  346 |       files.forEach((file) =>
> 347 |         expect(fs.existsSync(path.join(cwd, projectName, file))).toBeTruthy()
      |                                                                  ^
  348 |       )
  349 |     })
  350 |   })

  at forEach (integration/create-next-app/index.test.js:347:66)
      at Array.forEach (<anonymous>)
  at integration/create-next-app/index.test.js:346:13
  at usingTempDir (integration/create-next-app/index.test.js:20:5)
  at Object.<anonymous> (integration/create-next-app/index.test.js:334:5)

● create next app › should use npm as the package manager on supplying --use-npm

expect(received).toBeTruthy()

Received: false

  365 |       ]
  366 |       files.forEach((file) =>
> 367 |         expect(fs.existsSync(path.join(cwd, projectName, file))).toBeTruthy()
      |                                                                  ^
  368 |       )
  369 |     })
  370 |   })

  at forEach (integration/create-next-app/index.test.js:367:66)
      at Array.forEach (<anonymous>)
  at integration/create-next-app/index.test.js:366:13
  at usingTempDir (integration/create-next-app/index.test.js:20:5)
  at Object.<anonymous> (integration/create-next-app/index.test.js:353:5)

● create next app › should use npm as the package manager on supplying --use-npm with example

expect(received).toBeTruthy()

Received: false

  392 |       ]
  393 |       files.forEach((file) =>
> 394 |         expect(fs.existsSync(path.join(cwd, projectName, file))).toBeTruthy()
      |                                                                  ^
  395 |       )
  396 |     })
  397 |   })

  at forEach (integration/create-next-app/index.test.js:394:66)
      at Array.forEach (<anonymous>)
  at integration/create-next-app/index.test.js:393:13
  at usingTempDir (integration/create-next-app/index.test.js:20:5)
  at Object.<anonymous> (integration/create-next-app/index.test.js:373:5)

ijjk · 2021-07-06T19:28:27Z

Stats from current PR

Default Build (Increase detected ⚠️)

General Overall increase ⚠️

	vercel/next.js canary	theostrahlen/next.js fix/imgix-no-double-querystrings	Change
buildDuration	11.2s	11.2s	⚠️ +79ms
buildDurationCached	2.7s	2.6s	-100ms
nodeModulesSize	49.3 MB	49.3 MB	⚠️ +369 B

Page Load Tests Overall increase ✓

	vercel/next.js canary	theostrahlen/next.js fix/imgix-no-double-querystrings	Change
/ failed reqs	0	0	✓
/ total time (seconds)	1.951	1.95	0
/ avg req/sec	1281.29	1282.26	+0.97
/error-in-render failed reqs	0	0	✓
/error-in-render total time (seconds)	1.174	1.139	-0.03
/error-in-render avg req/sec	2130.34	2193.95	+63.61

Client Bundles (main, webpack, commons)

	vercel/next.js canary	theostrahlen/next.js fix/imgix-no-double-querystrings	Change
359.HASH.js gzip	3.09 kB	3.09 kB	✓
framework-HASH.js gzip	42 kB	42 kB	✓
main-HASH.js gzip	20.6 kB	20.6 kB	✓
webpack-HASH.js gzip	1.49 kB	1.49 kB	✓
Overall change	67.2 kB	67.2 kB	✓

Legacy Client Bundles (polyfills)

	vercel/next.js canary	theostrahlen/next.js fix/imgix-no-double-querystrings	Change
polyfills-HASH.js gzip	31.1 kB	31.1 kB	✓
Overall change	31.1 kB	31.1 kB	✓

Client Pages

	vercel/next.js canary	theostrahlen/next.js fix/imgix-no-double-querystrings	Change
_app-HASH.js gzip	803 B	803 B	✓
_error-HASH.js gzip	3.18 kB	3.18 kB	✓
amp-HASH.js gzip	526 B	526 B	✓
css-HASH.js gzip	329 B	329 B	✓
hooks-HASH.js gzip	903 B	903 B	✓
index-HASH.js gzip	263 B	263 B	✓
link-HASH.js gzip	1.65 kB	1.65 kB	✓
routerDirect..HASH.js gzip	322 B	322 B	✓
withRouter-HASH.js gzip	320 B	320 B	✓
bb14e60e810b..30f.css gzip	125 B	125 B	✓
Overall change	8.42 kB	8.42 kB	✓

Client Build Manifests

	vercel/next.js canary	theostrahlen/next.js fix/imgix-no-double-querystrings	Change
_buildManifest.js gzip	390 B	390 B	✓
Overall change	390 B	390 B	✓

Rendered Page Sizes

	vercel/next.js canary	theostrahlen/next.js fix/imgix-no-double-querystrings	Change
index.html gzip	523 B	523 B	✓
link.html gzip	537 B	537 B	✓
withRouter.html gzip	515 B	515 B	✓
Overall change	1.57 kB	1.57 kB	✓

Webpack 4 Mode (Increase detected ⚠️)

General Overall increase ⚠️

	vercel/next.js canary	theostrahlen/next.js fix/imgix-no-double-querystrings	Change
buildDuration	9.7s	9.8s	⚠️ +56ms
buildDurationCached	4.1s	3.9s	-176ms
nodeModulesSize	49.3 MB	49.3 MB	⚠️ +369 B

Page Load Tests Overall increase ✓

	vercel/next.js canary	theostrahlen/next.js fix/imgix-no-double-querystrings	Change
/ failed reqs	0	0	✓
/ total time (seconds)	2.054	2.032	-0.02
/ avg req/sec	1217.21	1230.51	+13.3
/error-in-render failed reqs	0	0	✓
/error-in-render total time (seconds)	1.24	1.101	-0.14
/error-in-render avg req/sec	2015.54	2271.69	+256.15

Client Bundles (main, webpack, commons)

	vercel/next.js canary	theostrahlen/next.js fix/imgix-no-double-querystrings	Change
14.HASH.js gzip	3.11 kB	3.11 kB	✓
677f882d2ed8..HASH.js gzip	13.9 kB	13.9 kB	✓
framework.HASH.js gzip	41.8 kB	41.8 kB	✓
main-HASH.js gzip	7.81 kB	7.81 kB	✓
webpack-HASH.js gzip	1.19 kB	1.19 kB	✓
Overall change	67.8 kB	67.8 kB	✓

Legacy Client Bundles (polyfills)

	vercel/next.js canary	theostrahlen/next.js fix/imgix-no-double-querystrings	Change
polyfills-HASH.js gzip	31.3 kB	31.3 kB	✓
Overall change	31.3 kB	31.3 kB	✓

Client Pages

	vercel/next.js canary	theostrahlen/next.js fix/imgix-no-double-querystrings	Change
_app-HASH.js gzip	791 B	791 B	✓
_error-HASH.js gzip	3.83 kB	3.83 kB	✓
amp-HASH.js gzip	531 B	531 B	✓
css-HASH.js gzip	333 B	333 B	✓
hooks-HASH.js gzip	910 B	910 B	✓
index-HASH.js gzip	227 B	227 B	✓
link-HASH.js gzip	1.64 kB	1.64 kB	✓
routerDirect..HASH.js gzip	295 B	295 B	✓
withRouter-HASH.js gzip	292 B	292 B	✓
e025d2764813..52f.css gzip	125 B	125 B	✓
Overall change	8.98 kB	8.98 kB	✓

Client Build Manifests

	vercel/next.js canary	theostrahlen/next.js fix/imgix-no-double-querystrings	Change
_buildManifest.js gzip	418 B	418 B	✓
Overall change	418 B	418 B	✓

Rendered Page Sizes

	vercel/next.js canary	theostrahlen/next.js fix/imgix-no-double-querystrings	Change
index.html gzip	569 B	569 B	✓
link.html gzip	581 B	581 B	✓
withRouter.html gzip	561 B	561 B	✓
Overall change	1.71 kB	1.71 kB	✓

Commit: e8b6f4b

If the `Image` src url had existing query params, the imgix loader would simply append another query string with `?` causing both query strings to break. This PR adds a way to safely merge query strings if needed using [URLSearchParams](https://developer.mozilla.org/en-US/docs/Web/API/URLSearchParams). ## Bug - [X] fixes vercel#26288 - [X] Integration tests added

Safely merge query string params in image src url

5b700f3

theostrahlen requested review from divmain, ijjk, lfades, shuding and timneutkens as code owners June 29, 2021 09:55

ijjk added the type: next label Jun 29, 2021

aarsxx approved these changes Jun 29, 2021

View reviewed changes

Merge branch 'canary' into fix/imgix-no-double-querystrings

6aed67f

This comment has been minimized.

Sign in to view

Merge branch 'canary' into fix/imgix-no-double-querystrings

97d859e

timneutkens requested review from huozhi, padmaia and styfle as code owners July 2, 2021 11:37

ijjk added the created-by: Next.js team PRs by the Next.js team. label Jul 2, 2021

This comment has been minimized.

Sign in to view

styfle approved these changes Jul 2, 2021

View reviewed changes

styfle and others added 3 commits July 2, 2021 15:19

Merge branch 'canary' into fix/imgix-no-double-querystrings

fdb9ed5

Merge branch 'canary' into fix/imgix-no-double-querystrings

5299aef

Merge branch 'canary' into fix/imgix-no-double-querystrings

0d8173e

This comment has been minimized.

Sign in to view

Merge branch 'canary' into fix/imgix-no-double-querystrings

4cde0c2

Merge branch 'canary' into fix/imgix-no-double-querystrings

e8b6f4b

styfle removed the created-by: Next.js team PRs by the Next.js team. label Jul 6, 2021

kodiakhq bot merged commit 38a4e56 into vercel:canary Jul 6, 2021

vercel locked as resolved and limited conversation to collaborators Jan 28, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

(next/image): Merge query string params in imgix loader #26719

(next/image): Merge query string params in imgix loader #26719

theostrahlen commented Jun 29, 2021

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

styfle left a comment

This comment has been minimized.

ijjk commented Jul 6, 2021

ijjk commented Jul 6, 2021

(next/image): Merge query string params in imgix loader #26719

(next/image): Merge query string params in imgix loader #26719

Conversation

theostrahlen commented Jun 29, 2021

Bug

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

styfle left a comment

Choose a reason for hiding this comment

This comment has been minimized.

ijjk commented Jul 6, 2021

Failing test suites

ijjk commented Jul 6, 2021

Stats from current PR