-
Notifications
You must be signed in to change notification settings - Fork 27.4k
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
fix(30724): clear "x-middleware-next" header when chaining middlewares #30866
fix(30724): clear "x-middleware-next" header when chaining middlewares #30866
Conversation
129d0b8
to
bab995f
Compare
bab995f
to
cafff5e
Compare
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this contribution! I'd change the semantics a bit though, seems more straightforward.
This comment has been minimized.
This comment has been minimized.
Stats from current PRDefault Build (Decrease detected ✓)General Overall increase
|
vercel/next.js canary | hi-ogawa/next.js 30724-clear-next-header-when-chaining-middlewares | Change | |
---|---|---|---|
buildDuration | 18.6s | 18.2s | -381ms |
buildDurationCached | 3.7s | 3.5s | -212ms |
nodeModulesSize | 335 MB | 335 MB |
Page Load Tests Overall decrease ⚠️
vercel/next.js canary | hi-ogawa/next.js 30724-clear-next-header-when-chaining-middlewares | Change | |
---|---|---|---|
/ failed reqs | 0 | 0 | ✓ |
/ total time (seconds) | 3.016 | 3.008 | -0.01 |
/ avg req/sec | 828.83 | 831.04 | +2.21 |
/error-in-render failed reqs | 0 | 0 | ✓ |
/error-in-render total time (seconds) | 1.468 | 1.474 | |
/error-in-render avg req/sec | 1702.46 | 1695.91 |
Client Bundles (main, webpack, commons)
vercel/next.js canary | hi-ogawa/next.js 30724-clear-next-header-when-chaining-middlewares | Change | |
---|---|---|---|
450.HASH.js gzip | 179 B | 179 B | ✓ |
framework-HASH.js gzip | 42.2 kB | 42.2 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 | hi-ogawa/next.js 30724-clear-next-header-when-chaining-middlewares | Change | |
---|---|---|---|
polyfills-HASH.js gzip | 31 kB | 31 kB | ✓ |
Overall change | 31 kB | 31 kB | ✓ |
Client Pages
vercel/next.js canary | hi-ogawa/next.js 30724-clear-next-header-when-chaining-middlewares | 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 | 327 B | 327 B | ✓ |
dynamic-HASH.js gzip | 2.38 kB | 2.38 kB | ✓ |
head-HASH.js gzip | 350 B | 350 B | ✓ |
hooks-HASH.js gzip | 635 B | 635 B | ✓ |
image-HASH.js gzip | 4.44 kB | 4.44 kB | ✓ |
index-HASH.js gzip | 263 B | 263 B | ✓ |
link-HASH.js gzip | 1.87 kB | 1.87 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 | 13.3 kB | 13.3 kB | ✓ |
Client Build Manifests
vercel/next.js canary | hi-ogawa/next.js 30724-clear-next-header-when-chaining-middlewares | Change | |
---|---|---|---|
_buildManifest.js gzip | 460 B | 460 B | ✓ |
Overall change | 460 B | 460 B | ✓ |
Rendered Page Sizes
vercel/next.js canary | hi-ogawa/next.js 30724-clear-next-header-when-chaining-middlewares | Change | |
---|---|---|---|
index.html gzip | 524 B | 524 B | ✓ |
link.html gzip | 535 B | 535 B | ✓ |
withRouter.html gzip | 517 B | 517 B | ✓ |
Overall change | 1.58 kB | 1.58 kB | ✓ |
Default Build with SWC (Decrease detected ✓)
General Overall increase ⚠️
vercel/next.js canary | hi-ogawa/next.js 30724-clear-next-header-when-chaining-middlewares | Change | |
---|---|---|---|
buildDuration | 19.1s | 19s | -86ms |
buildDurationCached | 3.6s | 3.6s | |
nodeModulesSize | 335 MB | 335 MB |
Page Load Tests Overall decrease ⚠️
vercel/next.js canary | hi-ogawa/next.js 30724-clear-next-header-when-chaining-middlewares | Change | |
---|---|---|---|
/ failed reqs | 0 | 0 | ✓ |
/ total time (seconds) | 2.855 | 2.911 | |
/ avg req/sec | 875.7 | 858.67 | |
/error-in-render failed reqs | 0 | 0 | ✓ |
/error-in-render total time (seconds) | 1.368 | 1.439 | |
/error-in-render avg req/sec | 1826.84 | 1737.77 |
Client Bundles (main, webpack, commons)
vercel/next.js canary | hi-ogawa/next.js 30724-clear-next-header-when-chaining-middlewares | Change | |
---|---|---|---|
450.HASH.js gzip | 179 B | 179 B | ✓ |
framework-HASH.js gzip | 42.3 kB | 42.3 kB | ✓ |
main-HASH.js gzip | 28.5 kB | 28.5 kB | ✓ |
webpack-HASH.js gzip | 1.43 kB | 1.43 kB | ✓ |
Overall change | 72.5 kB | 72.5 kB | ✓ |
Legacy Client Bundles (polyfills)
vercel/next.js canary | hi-ogawa/next.js 30724-clear-next-header-when-chaining-middlewares | Change | |
---|---|---|---|
polyfills-HASH.js gzip | 31 kB | 31 kB | ✓ |
Overall change | 31 kB | 31 kB | ✓ |
Client Pages
vercel/next.js canary | hi-ogawa/next.js 30724-clear-next-header-when-chaining-middlewares | 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.38 kB | 2.38 kB | ✓ |
head-HASH.js gzip | 342 B | 342 B | ✓ |
hooks-HASH.js gzip | 622 B | 622 B | ✓ |
image-HASH.js gzip | 4.46 kB | 4.46 kB | ✓ |
index-HASH.js gzip | 256 B | 256 B | ✓ |
link-HASH.js gzip | 1.91 kB | 1.91 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 | 13.2 kB | 13.2 kB | ✓ |
Client Build Manifests
vercel/next.js canary | hi-ogawa/next.js 30724-clear-next-header-when-chaining-middlewares | Change | |
---|---|---|---|
_buildManifest.js gzip | 459 B | 459 B | ✓ |
Overall change | 459 B | 459 B | ✓ |
Rendered Page Sizes
vercel/next.js canary | hi-ogawa/next.js 30724-clear-next-header-when-chaining-middlewares | Change | |
---|---|---|---|
index.html gzip | 521 B | 521 B | ✓ |
link.html gzip | 533 B | 533 B | ✓ |
withRouter.html gzip | 515 B | 515 B | ✓ |
Overall change | 1.57 kB | 1.57 kB | ✓ |
allHeaders.append(key, value) | ||
if (key !== 'x-middleware-next') { | ||
allHeaders.append(key, value) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why was this change made?
It means that a middleware cannot longer set headers without breaking the middleware chain as we have below:
if (!result.response.headers.has('x-middleware-next')) {
break
}
This will break some of the examples, like this one.
It also means that a middleware cannot simply add a cookie and keep the rest as is. For example, you may want to add a csrf
cookie if missing, but not break the middleware chain or return a custom Response
.
Fixes #30724
The linked issue wasn't triaged yet, so I wasn't sure if the current behavior was by design.
But, it seemed the issue was relatively simple, so I submit a possible fix here.
I hope this fix aligns to the intended behavior, but if that's not the case, I would appreciate any guidance to improve my code.
Thanks for the review!
Bug
fixes #number
contributing.md
Feature
fixes #number
contributing.md
Documentation / Examples
yarn lint