-
Notifications
You must be signed in to change notification settings - Fork 30.3k
Fix an issue parsing catchall params #65277
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 an issue parsing catchall params #65277
Conversation
|
Allow CI Workflow Run
Note: this should only be enabled once the PR is ready to go and can only be enabled by a maintainer |
Stats from current PRDefault Build (Increase detected
|
| vercel/next.js canary | agadzik/next.js gadzik/complex-parallel-route-setup | Change | |
|---|---|---|---|
| buildDuration | 15.6s | 14.1s | N/A |
| buildDurationCached | 7.8s | 7.3s | N/A |
| nodeModulesSize | 360 MB | 360 MB | |
| nextStartRea..uration (ms) | 412ms | 388ms | N/A |
Client Bundles (main, webpack)
| vercel/next.js canary | agadzik/next.js gadzik/complex-parallel-route-setup | Change | |
|---|---|---|---|
| 1103-HASH.js gzip | 31.9 kB | 31.9 kB | N/A |
| 1a9f679d-HASH.js gzip | 53.5 kB | 53.5 kB | N/A |
| 335-HASH.js gzip | 5.05 kB | 5.05 kB | ✓ |
| 7953.HASH.js gzip | 169 B | 169 B | ✓ |
| framework-HASH.js gzip | 45.2 kB | 45.2 kB | ✓ |
| main-app-HASH.js gzip | 231 B | 227 B | N/A |
| main-HASH.js gzip | 31.6 kB | 31.6 kB | N/A |
| webpack-HASH.js gzip | 1.65 kB | 1.65 kB | N/A |
| Overall change | 50.4 kB | 50.4 kB | ✓ |
Legacy Client Bundles (polyfills)
| vercel/next.js canary | agadzik/next.js gadzik/complex-parallel-route-setup | Change | |
|---|---|---|---|
| polyfills-HASH.js gzip | 31 kB | 31 kB | ✓ |
| Overall change | 31 kB | 31 kB | ✓ |
Client Pages
| vercel/next.js canary | agadzik/next.js gadzik/complex-parallel-route-setup | Change | |
|---|---|---|---|
| _app-HASH.js gzip | 192 B | 193 B | N/A |
| _error-HASH.js gzip | 192 B | 193 B | N/A |
| amp-HASH.js gzip | 507 B | 511 B | N/A |
| css-HASH.js gzip | 341 B | 343 B | N/A |
| dynamic-HASH.js gzip | 2.52 kB | 2.52 kB | ✓ |
| edge-ssr-HASH.js gzip | 266 B | 265 B | N/A |
| head-HASH.js gzip | 362 B | 365 B | N/A |
| hooks-HASH.js gzip | 392 B | 392 B | ✓ |
| image-HASH.js gzip | 4.27 kB | 4.27 kB | ✓ |
| index-HASH.js gzip | 268 B | 268 B | ✓ |
| link-HASH.js gzip | 2.69 kB | 2.7 kB | N/A |
| routerDirect..HASH.js gzip | 329 B | 328 B | N/A |
| script-HASH.js gzip | 397 B | 397 B | ✓ |
| withRouter-HASH.js gzip | 324 B | 324 B | ✓ |
| 1afbb74e6ecf..834.css gzip | 106 B | 106 B | ✓ |
| Overall change | 8.27 kB | 8.27 kB | ✓ |
Client Build Manifests
| vercel/next.js canary | agadzik/next.js gadzik/complex-parallel-route-setup | Change | |
|---|---|---|---|
| _buildManifest.js gzip | 483 B | 484 B | N/A |
| Overall change | 0 B | 0 B | ✓ |
Rendered Page Sizes
| vercel/next.js canary | agadzik/next.js gadzik/complex-parallel-route-setup | Change | |
|---|---|---|---|
| index.html gzip | 527 B | 528 B | N/A |
| link.html gzip | 540 B | 540 B | ✓ |
| withRouter.html gzip | 522 B | 524 B | N/A |
| Overall change | 540 B | 540 B | ✓ |
Edge SSR bundle Size
| vercel/next.js canary | agadzik/next.js gadzik/complex-parallel-route-setup | Change | |
|---|---|---|---|
| edge-ssr.js gzip | 94.7 kB | 94.7 kB | N/A |
| page.js gzip | 182 kB | 182 kB | N/A |
| Overall change | 0 B | 0 B | ✓ |
Middleware size
| vercel/next.js canary | agadzik/next.js gadzik/complex-parallel-route-setup | Change | |
|---|---|---|---|
| middleware-b..fest.js gzip | 624 B | 622 B | N/A |
| middleware-r..fest.js gzip | 156 B | 156 B | ✓ |
| middleware.js gzip | 25.7 kB | 25.7 kB | N/A |
| edge-runtime..pack.js gzip | 839 B | 839 B | ✓ |
| Overall change | 995 B | 995 B | ✓ |
Next Runtimes
| vercel/next.js canary | agadzik/next.js gadzik/complex-parallel-route-setup | Change | |
|---|---|---|---|
| app-page-exp...dev.js gzip | 171 kB | 171 kB | N/A |
| app-page-exp..prod.js gzip | 98.7 kB | 98.7 kB | N/A |
| app-page-tur..prod.js gzip | 100 kB | 100 kB | N/A |
| app-page-tur..prod.js gzip | 94.5 kB | 94.5 kB | N/A |
| app-page.run...dev.js gzip | 157 kB | 157 kB | N/A |
| app-page.run..prod.js gzip | 93.2 kB | 93.2 kB | N/A |
| app-route-ex...dev.js gzip | 21.5 kB | 21.5 kB | ✓ |
| app-route-ex..prod.js gzip | 15.2 kB | 15.2 kB | ✓ |
| app-route-tu..prod.js gzip | 15.2 kB | 15.2 kB | ✓ |
| app-route-tu..prod.js gzip | 15 kB | 15 kB | ✓ |
| app-route.ru...dev.js gzip | 21.3 kB | 21.3 kB | ✓ |
| app-route.ru..prod.js gzip | 15 kB | 15 kB | ✓ |
| pages-api-tu..prod.js gzip | 9.55 kB | 9.55 kB | ✓ |
| pages-api.ru...dev.js gzip | 9.82 kB | 9.82 kB | ✓ |
| pages-api.ru..prod.js gzip | 9.55 kB | 9.55 kB | ✓ |
| pages-turbo...prod.js gzip | 21.5 kB | 21.5 kB | N/A |
| pages.runtim...dev.js gzip | 22.1 kB | 22.1 kB | N/A |
| pages.runtim..prod.js gzip | 21.5 kB | 21.5 kB | N/A |
| server.runti..prod.js gzip | 51.6 kB | 51.6 kB | N/A |
| Overall change | 132 kB | 132 kB | ✓ |
build cache Overall increase ⚠️
| vercel/next.js canary | agadzik/next.js gadzik/complex-parallel-route-setup | Change | |
|---|---|---|---|
| 0.pack gzip | 1.62 MB | 1.62 MB | |
| index.pack gzip | 113 kB | 114 kB | |
| Overall change | 1.73 MB | 1.74 MB |
Diff details
Diff for edge-ssr.js
Diff too large to display
Diff for main-HASH.js
Diff too large to display
Diff for app-page-exp..ntime.dev.js
failed to diffDiff for app-page-exp..time.prod.js
Diff too large to display
Diff for app-page-tur..time.prod.js
Diff too large to display
Diff for app-page-tur..time.prod.js
Diff too large to display
Diff for app-page.runtime.dev.js
Diff too large to display
Diff for app-page.runtime.prod.js
Diff too large to display
Diff for pages-turbo...time.prod.js
Diff too large to display
Diff for pages.runtime.dev.js
Diff too large to display
Diff for pages.runtime.prod.js
Diff too large to display
Diff for server.runtime.prod.js
Diff too large to display
| * - `[[...slug]]` -> `{ key: 'slug', repeat: true, optional: true }` | ||
| * - `[...slug]` -> `{ key: 'slug', repeat: true, optional: false }` | ||
| * - `[[foo]]` -> `{ key: 'foo', repeat: false, optional: true }` | ||
| * - `[bar]` -> `{ key: 'bar', repeat: false, optional: false }` |
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.
Looking at https://github.com/vercel/next.js/pull/65277/files#diff-6d649b04b65094a03477d17ce26c376ebc0a7657ddbd2787ef93dd1348fb5892L29, seems that all of these cases that use dynamic params should be considered "optional" right?
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.
No, based on the regexp
const PARAMETER_PATTERN = /\[((?:\[.*\])|.+)\]/We take the first match and pass that into parseMatchedParameter which in the example of [[...foo]] will be [...foo]. Thats why parseMatchedParameter checks for startsWith([) and endsWith(]) sets optional to true. In the example of [...foo], parseMatchParameter received ...foo and thus is not marked as optional
| export default function Page() { | ||
| return ( | ||
| <div> | ||
| <h2>/buzz/[...fizz] Page!</h2> |
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.
| <h2>/buzz/[...fizz] Page!</h2> | |
| <h2>/buzz/[[...fizz]] Page!</h2> |
test/e2e/app-dir/parallel-routes-breadcrumbs/parallel-routes-breadcrumbs.test.ts
Outdated
Show resolved
Hide resolved
Tests Passed |
This should fix the following scenarios,
app/foo/[...bar]/page.tsxapp/bar/[[...foo]]/page.tsxapp/@slot/[...catchall]/page.tsxIf you navigate to
/foo/bartheparamsprop in the parallel route would beAnd if you navigate to
/bar/footheparamsprop in the parallel route would beWith the fix in place, the
paramsprop in the parallel route will be,And
Respectively