Allow to delete URL search params in middleware rewrites #33725

Schniz · 2022-01-27T08:57:59Z

This allows to have a more streamlined API for rewrites.
Since middlewares are code and not static configurations, we can allow people to
delete query params and not just overwrite them.

⚠️ Warning ⚠️ this is a breaking change in query parameter behavior with middlewares, but the API will make more sense now. Compare the following:

  import {NextResponse} from 'next/server'
  export default function middleware({ nextUrl }) {
-   nextUrl.searchParams.set('ignored-query-param', '');
+   nextUrl.searchParams.delete('ignored-query-param'); // 🎉
    return NextResponse.rewrite(nextUrl);
  }

Since this is breaking change, we're adding a warning every time we find a query param that is missing, and eventually--the warning will be deleted.
I don't think we need to worry about older versions of Next.js as the current behavior is misleading: merging query parameters make sense for static rewrites, but not for middlewares where you have access to all the query parameters and can manipulate them freely.

This is the opposite of Merge query params in middleware redirects #33454
This supersedes Allow to delete URL search params in middleware rewrites #33574

This allows to have a more streamlined API for rewrites. Since middlewares are code and not static configurations, we can allow people to delete query params and not just overwrite them.

packages/next/server/next-server.ts

test/integration/middleware/core/test/index.test.js

packages/next/server/next-server.ts

Co-authored-by: JJ Kasper <jj@jjsweb.site>

ijjk

Looks good!

ijjk · 2022-01-27T20:50:20Z

Failing test suites

Commit: 5a92d5e

test/integration/ondemand/test/index.test.js

On Demand Entries > should navigate to pages with dynamic imports

Expand output

● On Demand Entries › should navigate to pages with dynamic imports

TIMED OUT: /Hello/

/nav/dynamic

  479 | }
  480 |
> 481 | export class File {
      |               ^
  482 |   constructor(path) {
  483 |     this.path = path
  484 |     this.originalContent = existsSync(this.path)

  at Object.check (lib/next-test-utils.js:481:15)
  at Object.<anonymous> (integration/ondemand/test/index.test.js:102:13)

test/integration/pnpm-support/test/index.test.js

pnpm support > should execute client-side JS on each page

Expand output

● pnpm support › should execute client-side JS on each page

thrown: "Exceeded timeout of 90000 ms for a test.
Use jest.setTimeout(newTimeout) to increase the timeout value, if this is a long-running test."

  123 |
  124 |       const packageJsonPath = path.join(appDir, 'package.json')
> 125 |       const packageJson = await fs.readJson(packageJsonPath)
      |     ^
  126 |       expect(packageJson.dependencies['next']).toMatch(/^file:/)
  127 |       for (const dependency of [
  128 |         '@next/env',

  at integration/pnpm-support/test/index.test.js:125:5
  at Object.<anonymous> (integration/pnpm-support/test/index.test.js:104:1)

Schniz · 2022-01-27T20:53:37Z

Timed out 🤔

ijjk · 2022-01-27T21:55:56Z

Stats from current PR

Default Build (Decrease detected ✓)

General Overall increase ⚠️

	vercel/next.js canary	Schniz/next.js dont-merge-qp-in-rewrites	Change
buildDuration	12.2s	12.2s	-42ms
buildDurationCached	3.1s	3.1s	⚠️ +37ms
nodeModulesSize	358 MB	358 MB	⚠️ +3.18 kB

Page Load Tests Overall decrease ⚠️

	vercel/next.js canary	Schniz/next.js dont-merge-qp-in-rewrites	Change
/ failed reqs	0	0	✓
/ total time (seconds)	2.749	2.759	⚠️ +0.01
/ avg req/sec	909.37	906.01	⚠️ -3.36
/error-in-render failed reqs	0	0	✓
/error-in-render total time (seconds)	1.164	1.182	⚠️ +0.02
/error-in-render avg req/sec	2148	2115.22	⚠️ -32.78

Client Bundles (main, webpack, commons)

	vercel/next.js canary	Schniz/next.js dont-merge-qp-in-rewrites	Change
450.HASH.js gzip	179 B	179 B	✓
framework-HASH.js gzip	42.2 kB	42.2 kB	✓
main-HASH.js gzip	27.2 kB	27.2 kB	✓
webpack-HASH.js gzip	1.44 kB	1.44 kB	✓
Overall change	71 kB	71 kB	✓

Legacy Client Bundles (polyfills)

	vercel/next.js canary	Schniz/next.js dont-merge-qp-in-rewrites	Change
polyfills-HASH.js gzip	31 kB	31 kB	✓
Overall change	31 kB	31 kB	✓

Client Pages

	vercel/next.js canary	Schniz/next.js dont-merge-qp-in-rewrites	Change
_app-HASH.js gzip	1.37 kB	1.37 kB	✓
_error-HASH.js gzip	194 B	194 B	✓
amp-HASH.js gzip	312 B	312 B	✓
css-HASH.js gzip	326 B	326 B	✓
dynamic-HASH.js gzip	2.37 kB	2.37 kB	✓
head-HASH.js gzip	350 B	350 B	✓
hooks-HASH.js gzip	919 B	919 B	✓
image-HASH.js gzip	4.94 kB	4.94 kB	✓
index-HASH.js gzip	263 B	263 B	✓
link-HASH.js gzip	2.19 kB	2.19 kB	✓
routerDirect..HASH.js gzip	321 B	321 B	✓
script-HASH.js gzip	383 B	383 B	✓
withRouter-HASH.js gzip	318 B	318 B	✓
85e02e95b279..7e3.css gzip	107 B	107 B	✓
Overall change	14.4 kB	14.4 kB	✓

Client Build Manifests

	vercel/next.js canary	Schniz/next.js dont-merge-qp-in-rewrites	Change
_buildManifest.js gzip	460 B	460 B	✓
Overall change	460 B	460 B	✓

Rendered Page Sizes

	vercel/next.js canary	Schniz/next.js dont-merge-qp-in-rewrites	Change
index.html gzip	532 B	532 B	✓
link.html gzip	546 B	546 B	✓
withRouter.html gzip	527 B	527 B	✓
Overall change	1.6 kB	1.6 kB	✓

Default Build with SWC

General Overall increase ⚠️

	vercel/next.js canary	Schniz/next.js dont-merge-qp-in-rewrites	Change
buildDuration	15.6s	15.5s	-88ms
buildDurationCached	3.1s	3.1s	⚠️ +15ms
nodeModulesSize	358 MB	358 MB	⚠️ +3.18 kB

Page Load Tests Overall increase ✓

	vercel/next.js canary	Schniz/next.js dont-merge-qp-in-rewrites	Change
/ failed reqs	0	0	✓
/ total time (seconds)	2.76	2.828	⚠️ +0.07
/ avg req/sec	905.7	883.92	⚠️ -21.78
/error-in-render failed reqs	0	0	✓
/error-in-render total time (seconds)	1.201	1.189	-0.01
/error-in-render avg req/sec	2081.02	2103.14	+22.12

Client Bundles (main, webpack, commons)

	vercel/next.js canary	Schniz/next.js dont-merge-qp-in-rewrites	Change
450.HASH.js gzip	179 B	179 B	✓
framework-HASH.js gzip	42.3 kB	42.3 kB	✓
main-HASH.js gzip	27.3 kB	27.3 kB	✓
webpack-HASH.js gzip	1.44 kB	1.44 kB	✓
Overall change	71.3 kB	71.3 kB	✓

Legacy Client Bundles (polyfills)

	vercel/next.js canary	Schniz/next.js dont-merge-qp-in-rewrites	Change
polyfills-HASH.js gzip	31 kB	31 kB	✓
Overall change	31 kB	31 kB	✓

Client Pages

	vercel/next.js canary	Schniz/next.js dont-merge-qp-in-rewrites	Change
_app-HASH.js gzip	1.35 kB	1.35 kB	✓
_error-HASH.js gzip	180 B	180 B	✓
amp-HASH.js gzip	305 B	305 B	✓
css-HASH.js gzip	321 B	321 B	✓
dynamic-HASH.js gzip	2.36 kB	2.36 kB	✓
head-HASH.js gzip	342 B	342 B	✓
hooks-HASH.js gzip	911 B	911 B	✓
image-HASH.js gzip	4.97 kB	4.97 kB	✓
index-HASH.js gzip	256 B	256 B	✓
link-HASH.js gzip	2.21 kB	2.21 kB	✓
routerDirect..HASH.js gzip	314 B	314 B	✓
script-HASH.js gzip	375 B	375 B	✓
withRouter-HASH.js gzip	309 B	309 B	✓
85e02e95b279..7e3.css gzip	107 B	107 B	✓
Overall change	14.3 kB	14.3 kB	✓

Client Build Manifests

	vercel/next.js canary	Schniz/next.js dont-merge-qp-in-rewrites	Change
_buildManifest.js gzip	459 B	459 B	✓
Overall change	459 B	459 B	✓

Rendered Page Sizes

	vercel/next.js canary	Schniz/next.js dont-merge-qp-in-rewrites	Change
index.html gzip	530 B	530 B	✓
link.html gzip	543 B	543 B	✓
withRouter.html gzip	525 B	525 B	✓
Overall change	1.6 kB	1.6 kB	✓

Commit: 6ee5b2f

This allows to have a more streamlined API for rewrites. Since middlewares are code and not static configurations, we can allow people to delete query params and not just overwrite them. **⚠️ Warning ⚠️** this is a breaking change in query parameter behavior with middlewares, but the API will make more sense now. Compare the following: ```diff import {NextResponse} from 'next/server' export default function middleware({ nextUrl }) { - nextUrl.searchParams.set('ignored-query-param', ''); + nextUrl.searchParams.delete('ignored-query-param'); // 🎉 return NextResponse.rewrite(nextUrl); } ``` Since this is breaking change, we're adding a warning every time we find a query param that is missing, and eventually--the warning will be deleted. I don't think we need to worry about older versions of Next.js as the current behavior is misleading: merging query parameters make sense for static rewrites, but not for middlewares where you have access to all the query parameters and can manipulate them freely. Related: * This is the opposite of vercel#33454 * This supersedes vercel#33574

Schniz added 2 commits January 27, 2022 10:54

Don't merge queryparams in middleware rewrites

6269320

This allows to have a more streamlined API for rewrites. Since middlewares are code and not static configurations, we can allow people to delete query params and not just overwrite them.

add a test case

29bc67b

Schniz requested review from huozhi, ijjk, shuding and timneutkens as code owners January 27, 2022 08:58

ijjk added area: documentation type: next labels Jan 27, 2022

This comment has been minimized.

Sign in to view

ijjk reviewed Jan 27, 2022

View reviewed changes

packages/next/server/next-server.ts Outdated Show resolved Hide resolved

ijjk reviewed Jan 27, 2022

View reviewed changes

test/integration/middleware/core/test/index.test.js Show resolved Hide resolved

ijjk reviewed Jan 27, 2022

View reviewed changes

packages/next/server/next-server.ts Outdated Show resolved Hide resolved

Schniz and others added 3 commits January 27, 2022 21:45

Update packages/next/server/next-server.ts

ce31465

Co-authored-by: JJ Kasper <jj@jjsweb.site>

Update packages/next/server/next-server.ts

0ec02c9

Co-authored-by: JJ Kasper <jj@jjsweb.site>

warn once, and test that it actually warns

1615cc3

Schniz requested a review from ijjk January 27, 2022 20:10

fix lint

608d3b5

ijjk approved these changes Jan 27, 2022

View reviewed changes

Merge branch 'canary' into dont-merge-qp-in-rewrites

5a92d5e

This comment has been minimized.

Sign in to view

Merge branch 'canary' into dont-merge-qp-in-rewrites

6ee5b2f

kodiakhq bot merged commit 7b2ea48 into vercel:canary Jan 27, 2022

Schniz mentioned this pull request Feb 1, 2022

Query params filter infinite redirect vercel/examples#45

Closed

github-actions bot locked as resolved and limited conversation to collaborators Feb 27, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow to delete URL search params in middleware rewrites #33725

Allow to delete URL search params in middleware rewrites #33725

Schniz commented Jan 27, 2022

This comment has been minimized.

ijjk left a comment

This comment has been minimized.

ijjk commented Jan 27, 2022 •

edited

Schniz commented Jan 27, 2022

ijjk commented Jan 27, 2022

Allow to delete URL search params in middleware rewrites #33725

Allow to delete URL search params in middleware rewrites #33725

Conversation

Schniz commented Jan 27, 2022

This comment has been minimized.

ijjk left a comment

Choose a reason for hiding this comment

This comment has been minimized.

ijjk commented Jan 27, 2022 • edited

Failing test suites

Schniz commented Jan 27, 2022

ijjk commented Jan 27, 2022

Stats from current PR

ijjk commented Jan 27, 2022 •

edited