Skip to content
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 logic error in parallel route catch-all normalization #63879

Conversation

ztanner
Copy link
Member

@ztanner ztanner commented Mar 29, 2024

What & Why

There was some code added in the catch-all route normalization that doesn't seem to make sense -- it was checking if the provided appPath depth was larger than the catch-all route depth, prior to inserting it. But it was comparing depths in an inconsistent way (.length vs .length - 1), and the catch all path was also considering the @slot and /page suffix as part of the path depth.

This means that if you had a @modal/[...catchAll] slot, it wouldn't be considered for a page like /foo/bar/baz, because /foo/bar/baz (depth: 4 with the current logic) and /@modal/[...catchAll]/page (depth: 3 with the current logic) signaled that the /foo/bar/baz route was "more specific" and shouldn't match the catch-all.

I think this was most likely added to resolve a bug where we were inserting optional catch-all ([[...catchAll]]) routes into parallel slots. However, optional catch-all routes are currently unsupported with parallel routes, so this feature didn't work properly and the partial support introduced a bug for regular catch-all routes.

How

This removes the confusing workaround and skips optional catch-all segments in this handling. Separately, we can add support for optional catch-all parallel routes, but doing so will require quite a bit more changes & also similar handling in Turbopack. Namely, if have a top-level optional catch-all, in both the Turbopack & current Webpack implementation, that top-level catch-all wouldn't be matched. And if you tried to have an optional catch-all slot, in both implementations, the app would error with:

You cannot define a route with the same specificity as a optional catch-all route ("/" and "/[[...catchAll]]")

because our route normalization logic does not treat slots specificity differently than pages.

Note: This keeps the test that was added when this logic was first introduced in #60776 to ensure that the case this was originally added for still passes.

Fixes #62948
Closes NEXT-2728

Copy link
Member Author

ztanner commented Mar 29, 2024

This stack of pull requests is managed by Graphite. Learn more about stacking.

Join @ztanner and the rest of your teammates on Graphite Graphite

@ijjk
Copy link
Member

ijjk commented Mar 29, 2024

Tests Passed

@ijjk
Copy link
Member

ijjk commented Mar 29, 2024

Stats from current PR

Default Build
General
vercel/next.js canary vercel/next.js 03-29-fix_logic_error_in_parallel_route_catch-all_normalization Change
buildDuration 13.7s 13.9s ⚠️ +146ms
buildDurationCached 7.4s 6.2s N/A
nodeModulesSize 199 MB 199 MB N/A
nextStartRea..uration (ms) 396ms 405ms N/A
Client Bundles (main, webpack)
vercel/next.js canary vercel/next.js 03-29-fix_logic_error_in_parallel_route_catch-all_normalization Change
2453-HASH.js gzip 31.4 kB 31.4 kB N/A
3304.HASH.js gzip 181 B 181 B
3f784ff6-HASH.js gzip 53.7 kB 53.7 kB
8299-HASH.js gzip 5.04 kB 5.04 kB N/A
framework-HASH.js gzip 45.2 kB 45.2 kB
main-app-HASH.js gzip 242 B 242 B
main-HASH.js gzip 32.2 kB 32.2 kB N/A
webpack-HASH.js gzip 1.68 kB 1.68 kB N/A
Overall change 99.3 kB 99.3 kB
Legacy Client Bundles (polyfills)
vercel/next.js canary vercel/next.js 03-29-fix_logic_error_in_parallel_route_catch-all_normalization Change
polyfills-HASH.js gzip 31 kB 31 kB
Overall change 31 kB 31 kB
Client Pages
vercel/next.js canary vercel/next.js 03-29-fix_logic_error_in_parallel_route_catch-all_normalization Change
_app-HASH.js gzip 196 B 197 B N/A
_error-HASH.js gzip 184 B 184 B
amp-HASH.js gzip 505 B 505 B
css-HASH.js gzip 324 B 325 B N/A
dynamic-HASH.js gzip 2.5 kB 2.5 kB N/A
edge-ssr-HASH.js gzip 258 B 258 B
head-HASH.js gzip 352 B 352 B
hooks-HASH.js gzip 370 B 371 B N/A
image-HASH.js gzip 4.21 kB 4.21 kB
index-HASH.js gzip 259 B 259 B
link-HASH.js gzip 2.67 kB 2.67 kB N/A
routerDirect..HASH.js gzip 314 B 312 B N/A
script-HASH.js gzip 386 B 386 B
withRouter-HASH.js gzip 309 B 309 B
1afbb74e6ecf..834.css gzip 106 B 106 B
Overall change 6.57 kB 6.57 kB
Client Build Manifests
vercel/next.js canary vercel/next.js 03-29-fix_logic_error_in_parallel_route_catch-all_normalization Change
_buildManifest.js gzip 481 B 484 B N/A
Overall change 0 B 0 B
Rendered Page Sizes
vercel/next.js canary vercel/next.js 03-29-fix_logic_error_in_parallel_route_catch-all_normalization Change
index.html gzip 529 B 527 B N/A
link.html gzip 541 B 540 B N/A
withRouter.html gzip 524 B 522 B N/A
Overall change 0 B 0 B
Edge SSR bundle Size
vercel/next.js canary vercel/next.js 03-29-fix_logic_error_in_parallel_route_catch-all_normalization Change
edge-ssr.js gzip 95.3 kB 95.3 kB N/A
page.js gzip 3.04 kB 3.04 kB N/A
Overall change 0 B 0 B
Middleware size
vercel/next.js canary vercel/next.js 03-29-fix_logic_error_in_parallel_route_catch-all_normalization Change
middleware-b..fest.js gzip 624 B 625 B N/A
middleware-r..fest.js gzip 151 B 151 B
middleware.js gzip 25.5 kB 25.5 kB N/A
edge-runtime..pack.js gzip 839 B 839 B
Overall change 990 B 990 B
Next Runtimes
vercel/next.js canary vercel/next.js 03-29-fix_logic_error_in_parallel_route_catch-all_normalization Change
app-page-exp...dev.js gzip 170 kB 170 kB
app-page-exp..prod.js gzip 97 kB 97 kB
app-page-tur..prod.js gzip 98.8 kB 98.8 kB
app-page-tur..prod.js gzip 93 kB 93 kB
app-page.run...dev.js gzip 144 kB 144 kB
app-page.run..prod.js gzip 91.5 kB 91.5 kB
app-route-ex...dev.js gzip 21.4 kB 21.4 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 14.9 kB 14.9 kB
app-route.ru...dev.js gzip 21.1 kB 21.1 kB
app-route.ru..prod.js gzip 14.9 kB 14.9 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 22.5 kB 22.5 kB
pages.runtim...dev.js gzip 23.1 kB 23.1 kB
pages.runtim..prod.js gzip 22.4 kB 22.4 kB
server.runti..prod.js gzip 51 kB 51 kB
Overall change 945 kB 945 kB
build cache
vercel/next.js canary vercel/next.js 03-29-fix_logic_error_in_parallel_route_catch-all_normalization Change
0.pack gzip 1.58 MB 1.58 MB N/A
index.pack gzip 106 kB 106 kB N/A
Overall change 0 B 0 B
Diff details
Diff for middleware.js

Diff too large to display

Commit: c63e728

@ztanner ztanner force-pushed the 03-29-fix_logic_error_in_parallel_route_catch-all_normalization branch 4 times, most recently from 3ea2b13 to 68a86bb Compare April 1, 2024 15:09
@ztanner ztanner force-pushed the 03-29-fix_logic_error_in_parallel_route_catch-all_normalization branch from 68a86bb to c63e728 Compare April 1, 2024 15:19
@ztanner ztanner marked this pull request as ready for review April 1, 2024 15:29
@ztanner ztanner merged commit de049f8 into canary Apr 1, 2024
80 checks passed
@ztanner ztanner deleted the 03-29-fix_logic_error_in_parallel_route_catch-all_normalization branch April 1, 2024 15:50
@fax1ty
Copy link

fax1ty commented Apr 2, 2024

I don't know if these changes are related, but I still get the following error

[Error: Invalid segment Parallel("controls"), catch all segment must be the last segment (segments: [Dynamic("lang"), Dynamic("city"), Static("clinics"), OptionalCatchAll("args")])

Debug info:

  • Execution of get_entrypoints_with_issues failed
  • Execution of Project::entrypoints failed
  • Execution of AppProject::routes failed
  • Execution of directory_tree_to_entrypoints_internal failed
  • Execution of directory_tree_to_entrypoints_internal failed
  • Execution of directory_tree_to_entrypoints_internal failed
  • Execution of directory_tree_to_entrypoints_internal failed
  • Execution of directory_tree_to_entrypoints_internal failed
  • Execution of directory_tree_to_loader_tree failed
  • Invalid segment Parallel("controls"), catch all segment must be the last segment (segments: [Dynamic("lang"), Dynamic("city"), Static("clinics"), OptionalCatchAll("args")])] {
    code: 'GenericFailure'
    }

Project structure:
image

(The problem only occurs when using turbo)

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 17, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Parallel + Intercepting Route (Modal) does not trigger Catch All route in all cases
3 participants