Support colons in Middleware NextResponse.rewrite path#32543
Support colons in Middleware NextResponse.rewrite path#32543ijjk merged 25 commits intovercel:canaryfrom
Conversation
this addresses the symptom but the real systemic issue is that prepareDestination is called on rewrite/redirect URLs, which have no defined special behavior for colons and they should not be compiled at all
this is a bit nicer than just escaping colons, but ideally we find a way to obviate prepareDestination
|
please advise @javivelasco |
|
Is there any way to get official eyeballs on this PR? Seems like it addresses the colon problem. |
|
Ready to merge again :] |
Failing test suitesCommit: 8788b63
Expand output● AMP Validation on Export › should have shown errors during build ● AMP Validation on Export › should export AMP pages Read more about building and testing Next.js in contributing.md.
Expand output● AMP Fragment Styles › adds styles from fragment in AMP mode correctly Read more about building and testing Next.js in contributing.md.
Expand output● AMP Custom Validator › should run in dev mode successfully Read more about building and testing Next.js in contributing.md. |
This comment has been minimized.
This comment has been minimized.
javivelasco
left a comment
There was a problem hiding this comment.
Gonna be checking this in detail asap
nkzawa
left a comment
There was a problem hiding this comment.
This looks awesome aside from a comment.
Co-authored-by: Naoyuki Kanezawa <naoyuki.kanezawa@gmail.com>
javivelasco
left a comment
There was a problem hiding this comment.
I think it looks good overall and all tests are passing so 🚀
Thank you!
Stats from current PRDefault Build (Decrease detected ✓)General Overall decrease ✓
Page Load Tests Overall increase ✓
Client Bundles (main, webpack)
Legacy Client Bundles (polyfills)
Client Pages
Client Build Manifests
Rendered Page Sizes
Default Build with SWC (Increase detected
|
| vercel/next.js canary | klarstrup/next.js support-colons-in-rewrite-paths | Change | |
|---|---|---|---|
| buildDuration | 23.1s | 23s | -101ms |
| buildDurationCached | 7.5s | 7.7s | |
| nodeModulesSize | 372 MB | 372 MB | -198 B |
Page Load Tests Overall increase ✓
| vercel/next.js canary | klarstrup/next.js support-colons-in-rewrite-paths | Change | |
|---|---|---|---|
| / failed reqs | 0 | 0 | ✓ |
| / total time (seconds) | 3.842 | 3.8 | -0.04 |
| / avg req/sec | 650.62 | 657.96 | +7.34 |
| /error-in-render failed reqs | 0 | 0 | ✓ |
| /error-in-render total time (seconds) | 1.82 | 1.656 | -0.16 |
| /error-in-render avg req/sec | 1373.6 | 1509.8 | +136.2 |
Client Bundles (main, webpack)
| vercel/next.js canary | klarstrup/next.js support-colons-in-rewrite-paths | Change | |
|---|---|---|---|
| 925.HASH.js gzip | 178 B | 178 B | ✓ |
| framework-HASH.js gzip | 42.3 kB | 42.3 kB | ✓ |
| main-HASH.js gzip | 28.1 kB | 28.1 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 | klarstrup/next.js support-colons-in-rewrite-paths | Change | |
|---|---|---|---|
| polyfills-HASH.js gzip | 31 kB | 31 kB | ✓ |
| Overall change | 31 kB | 31 kB | ✓ |
Client Pages
| vercel/next.js canary | klarstrup/next.js support-colons-in-rewrite-paths | Change | |
|---|---|---|---|
| _app-HASH.js gzip | 1.36 kB | 1.36 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.2 kB | ✓ |
| 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 | 14.9 kB | ✓ |
Client Build Manifests
| vercel/next.js canary | klarstrup/next.js support-colons-in-rewrite-paths | Change | |
|---|---|---|---|
| _buildManifest.js gzip | 458 B | 458 B | ✓ |
| Overall change | 458 B | 458 B | ✓ |
Rendered Page Sizes
| vercel/next.js canary | klarstrup/next.js support-colons-in-rewrite-paths | 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 | ✓ |
Fixes #31523 which was introduced here javivelasco@8ae63b8#diff-ea09101c272788873ce720075e4839f0254179af0a07259a107752cafb193949R1230-R1235
It's problematic to be using code intended for the router with substitution handling etc. here, skipping
prepareDestinationis basically a soft revert of part of the above commitBug
fixes #numberDocumentation / Examples
yarn lint