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 catch-all route normalization for default parallel routes #60240

Conversation

ztanner
Copy link
Member

@ztanner ztanner commented Jan 4, 2024

What?

When relying on a default route as a fallback, greedier catch-all segments in the application hierarchy would take precedence, causing unexpected errors/matching behavior.

Why?

When performing parallel route catch-all normalization, we push potential catch-all matches to paths without considering that a path might instead be matched by a default page. Because of this, the catch-all take precedence and the app will not try and load the default.

For example, given this structure:

{
  "/": ["/page"],
  "/[[...catchAll]]": ["/[[...catchAll]]/page"],
  "/nested/[foo]/[bar]": ["/nested/[foo]/[bar]/@slot/page"],
  "/nested/[foo]/[bar]/[baz]": ["/nested/[foo]/[bar]/@slot/[baz]/page"],
}

(Where there's a /nested/[foo]/[bar]/default.tsx)

The route normalization logic would produce:

{
  "/": ["/page"],
  "/[[...catchAll]]": ["/[[...catchAll]]/page"],
  "/nested/[foo]/[bar]": [
    "/nested/[foo]/[bar]/@slot/page",
    "/[[...catchAll]]/page",
  ],
  "/nested/[foo]/[bar]/[baz]": [
    "/nested/[foo]/[bar]/@slot/[baz]/page",
    "/[[...catchAll]]/page",
  ],
}

This means that when building the LoaderTree, it won't ever try to find the default for that segment. This solution operates under the assumption that if you defined a default at a particular layout segment, you intend for that to render in place of a greedier catch-all. (Let me know if this is an incorrect assumption)

How?

We can't safely normalize catch-all parallel routes without having context about where the default segments are, so this updates appPaths to be inclusive of default segments and then filters them when doing anything relating to build/export to maintain existing behavior. We use this information to check if an existing default exists at the same segment level that we'd push the catch-all to. If one exists, we don't push the catch-all. Otherwise we proceed as normal.

Closes NEXT-1987

Copy link
Member Author

ztanner commented Jan 4, 2024

Current dependencies on/for this PR:

This stack of pull requests is managed by Graphite.

@ijjk
Copy link
Member

ijjk commented Jan 4, 2024

Stats from current PR

Default Build
General Overall increase ⚠️
vercel/next.js canary vercel/next.js 01-04-ensure_more_specific_segments_match_parallel_routes_before_greedier_catch-all_segments Change
buildDuration 12.9s 12.9s N/A
buildDurationCached 7.3s 6.3s N/A
nodeModulesSize 200 MB 200 MB ⚠️ +6.45 kB
nextStartRea..uration (ms) 410ms 407ms N/A
Client Bundles (main, webpack)
vercel/next.js canary vercel/next.js 01-04-ensure_more_specific_segments_match_parallel_routes_before_greedier_catch-all_segments Change
193.HASH.js gzip 181 B 182 B N/A
3f784ff6-HASH.js gzip 53.3 kB 53.3 kB
433-HASH.js gzip 28.6 kB 28.6 kB N/A
framework-HASH.js gzip 45.2 kB 45.2 kB
main-app-HASH.js gzip 239 B 242 B N/A
main-HASH.js gzip 31.7 kB 31.8 kB N/A
webpack-HASH.js gzip 1.7 kB 1.7 kB N/A
Overall change 98.5 kB 98.5 kB
Legacy Client Bundles (polyfills)
vercel/next.js canary vercel/next.js 01-04-ensure_more_specific_segments_match_parallel_routes_before_greedier_catch-all_segments Change
polyfills-HASH.js gzip 31 kB 31 kB
Overall change 31 kB 31 kB
Client Pages
vercel/next.js canary vercel/next.js 01-04-ensure_more_specific_segments_match_parallel_routes_before_greedier_catch-all_segments Change
_app-HASH.js gzip 194 B 195 B N/A
_error-HASH.js gzip 183 B 181 B N/A
amp-HASH.js gzip 504 B 502 B N/A
css-HASH.js gzip 321 B 321 B
dynamic-HASH.js gzip 2.5 kB 2.5 kB N/A
edge-ssr-HASH.js gzip 255 B 253 B N/A
head-HASH.js gzip 350 B 349 B N/A
hooks-HASH.js gzip 369 B 369 B
image-HASH.js gzip 4.28 kB 4.28 kB N/A
index-HASH.js gzip 255 B 256 B N/A
link-HASH.js gzip 2.61 kB 2.61 kB
routerDirect..HASH.js gzip 312 B 311 B N/A
script-HASH.js gzip 385 B 383 B N/A
withRouter-HASH.js gzip 307 B 308 B N/A
1afbb74e6ecf..834.css gzip 106 B 106 B
Overall change 3.4 kB 3.4 kB
Client Build Manifests
vercel/next.js canary vercel/next.js 01-04-ensure_more_specific_segments_match_parallel_routes_before_greedier_catch-all_segments Change
_buildManifest.js gzip 483 B 484 B N/A
Overall change 0 B 0 B
Rendered Page Sizes
vercel/next.js canary vercel/next.js 01-04-ensure_more_specific_segments_match_parallel_routes_before_greedier_catch-all_segments Change
index.html gzip 527 B 529 B N/A
link.html gzip 540 B 541 B N/A
withRouter.html gzip 523 B 524 B N/A
Overall change 0 B 0 B
Edge SSR bundle Size
vercel/next.js canary vercel/next.js 01-04-ensure_more_specific_segments_match_parallel_routes_before_greedier_catch-all_segments Change
edge-ssr.js gzip 93.8 kB 93.8 kB N/A
page.js gzip 147 kB 147 kB N/A
Overall change 0 B 0 B
Middleware size
vercel/next.js canary vercel/next.js 01-04-ensure_more_specific_segments_match_parallel_routes_before_greedier_catch-all_segments Change
middleware-b..fest.js gzip 626 B 625 B N/A
middleware-r..fest.js gzip 151 B 151 B
middleware.js gzip 37.4 kB 37.4 kB N/A
edge-runtime..pack.js gzip 1.92 kB 1.92 kB
Overall change 2.07 kB 2.07 kB
Next Runtimes
vercel/next.js canary vercel/next.js 01-04-ensure_more_specific_segments_match_parallel_routes_before_greedier_catch-all_segments Change
app-page-exp...dev.js gzip 169 kB 169 kB
app-page-exp..prod.js gzip 94.2 kB 94.2 kB
app-page-tur..prod.js gzip 94.9 kB 94.9 kB
app-page-tur..prod.js gzip 89.5 kB 89.5 kB
app-page.run...dev.js gzip 139 kB 139 kB
app-page.run..prod.js gzip 88.8 kB 88.8 kB
app-route-ex...dev.js gzip 24.1 kB 24.1 kB
app-route-ex..prod.js gzip 16.7 kB 16.7 kB
app-route-tu..prod.js gzip 16.7 kB 16.7 kB
app-route-tu..prod.js gzip 16.3 kB 16.3 kB
app-route.ru...dev.js gzip 23.5 kB 23.5 kB
app-route.ru..prod.js gzip 16.3 kB 16.3 kB
pages-api-tu..prod.js gzip 9.38 kB 9.38 kB
pages-api.ru...dev.js gzip 9.65 kB 9.65 kB
pages-api.ru..prod.js gzip 9.37 kB 9.37 kB
pages-turbo...prod.js gzip 21.9 kB 21.9 kB
pages.runtim...dev.js gzip 22.6 kB 22.6 kB
pages.runtim..prod.js gzip 21.9 kB 21.9 kB
server.runti..prod.js gzip 49.5 kB 49.5 kB
Overall change 932 kB 932 kB
Commit: 83fb9eb

@ijjk
Copy link
Member

ijjk commented Jan 4, 2024

Tests Passed

@ztanner ztanner force-pushed the 01-04-ensure_more_specific_segments_match_parallel_routes_before_greedier_catch-all_segments branch from df109cf to b321561 Compare January 4, 2024 21:14
@ztanner ztanner changed the title fix catch-all route normalization for default routes fix catch-all route normalization for default parallle routes Jan 4, 2024
@ztanner ztanner changed the title fix catch-all route normalization for default parallle routes fix catch-all route normalization for default parallel routes Jan 4, 2024
@ztanner ztanner changed the title fix catch-all route normalization for default parallel routes (wip) fix catch-all route normalization for default parallel routes Jan 5, 2024
@ztanner ztanner force-pushed the 01-04-ensure_more_specific_segments_match_parallel_routes_before_greedier_catch-all_segments branch from b321561 to 73f0cdb Compare January 5, 2024 01:02
@ztanner ztanner force-pushed the 01-04-ensure_more_specific_segments_match_parallel_routes_before_greedier_catch-all_segments branch from 73f0cdb to 83fb9eb Compare January 5, 2024 01:05
@ztanner ztanner changed the title (wip) fix catch-all route normalization for default parallel routes fix catch-all route normalization for default parallel routes Jan 5, 2024
@ztanner ztanner marked this pull request as ready for review January 5, 2024 14:38
@ztanner ztanner merged commit 90f9539 into canary Jan 5, 2024
75 checks passed
@ztanner ztanner deleted the 01-04-ensure_more_specific_segments_match_parallel_routes_before_greedier_catch-all_segments branch January 5, 2024 22:20
agustints pushed a commit to agustints/next.js that referenced this pull request Jan 6, 2024
…#60240)

### What?
When relying on a `default` route as a fallback, greedier catch-all
segments in the application hierarchy would take precedence, causing
unexpected errors/matching behavior.

### Why?
When performing parallel route catch-all normalization, we push
potential catch-all matches to paths without considering that a path
might instead be matched by a `default` page. Because of this, the
catch-all take precedence and the app will not try and load the default.

For example, given this structure:

```
{
  "/": ["/page"],
  "/[[...catchAll]]": ["/[[...catchAll]]/page"],
  "/nested/[foo]/[bar]": ["/nested/[foo]/[bar]/@slot/page"],
  "/nested/[foo]/[bar]/[baz]": ["/nested/[foo]/[bar]/@slot/[baz]/page"],
}
```

(Where there's a `/nested/[foo]/[bar]/default.tsx`)

The route normalization logic would produce:

```
{
  "/": ["/page"],
  "/[[...catchAll]]": ["/[[...catchAll]]/page"],
  "/nested/[foo]/[bar]": [
    "/nested/[foo]/[bar]/@slot/page",
    "/[[...catchAll]]/page",
  ],
  "/nested/[foo]/[bar]/[baz]": [
    "/nested/[foo]/[bar]/@slot/[baz]/page",
    "/[[...catchAll]]/page",
  ],
}
```
This means that when building the `LoaderTree`, it won't ever try to
find the default for that segment. **This solution operates under the
assumption that if you defined a `default` at a particular layout
segment, you intend for that to render in place of a greedier
catch-all.** (Let me know if this is an incorrect assumption)

### How?
We can't safely normalize catch-all parallel routes without having
context about where the `default` segments are, so this updates
`appPaths` to be inclusive of default segments and then filters them
when doing anything relating to build/export to maintain existing
behavior. We use this information to check if an existing default exists
at the same segment level that we'd push the catch-all to. If one
exists, we don't push the catch-all. Otherwise we proceed as normal.

Closes NEXT-1987
ztanner added a commit that referenced this pull request Jan 11, 2024
### What
`/default` segments were considered valid page outputs to handle
catch-all route normalization (see #60240) but they shouldn't leak into
the prerender manifest. This filters them out at build time.

Closes NEXT-2053
ztanner added a commit that referenced this pull request Jan 23, 2024
… nested explicit (non-catchall) slot routes (#60776)

Fix NEXT-2165

### What?

Addresses the limitation of #60240, where a dummy `default` file is
required in parallel route child slot to prevent errors in dev server
rendering (`TypeError: Cannot read properties of undefined (reading
'clientModules')`) as well as errors in build and deploy (`Error:
ENOENT: no such file or directory, lstat
‘/vercel/path0/.next/server/app/parallel-route/[section]/@part/[partSlug]/page_client-reference-manifest.js’`)

Without the `default.tsx`, builds and deployments will fail with:

<img width="956" alt="CleanShot 2024-01-18 at 02 12 36@2x"
src="https://github.com/vercel/next.js/assets/179761/80ba61bd-6ec0-4b16-a393-dc9375227e19">

local dev server will also crash with:

<img width="986" alt="CleanShot 2024-01-18 at 02 13 19@2x"
src="https://github.com/vercel/next.js/assets/179761/cc500a32-b2f8-47b4-999e-e57cf5141b2f">

> TypeError: Cannot read properties of undefined (reading
'clientModules')


### Why?

Since `default.tsx` is not a compulsory when you have slot that are
specific and ends with a dynamic route segment, this PR extends support
so that it is possible mixing catch-all routes with specific
non-catchall routes without requiring an additional `default.tsx` .

This PR will allow the following test cases to pass:

```
it('should not add the catch-all route to segments that have a more specific [dynamicRoute]', () => {
    const appPaths = {
      '/': ['/page'],
      '/[[...catchAll]]': ['/[[...catchAll]]/page'],
      '/nested/[foo]/[bar]/default': [
        '/nested/[foo]/[bar]/default',
        '/nested/[foo]/[bar]/@slot0/default',
        '/nested/[foo]/[bar]/@slot2/default',
      ],
      '/nested/[foo]/[bar]': [
        '/nested/[foo]/[bar]/@slot0/page',
        '/nested/[foo]/[bar]/@slot1/page',
      ],
      '/nested/[foo]/[bar]/[baz]': [
        '/nested/[foo]/[bar]/@slot0/[baz]/page',
        '/nested/[foo]/[bar]/@slot1/[baz]/page',
      ],
      '/[locale]/nested/[foo]/[bar]/[baz]/[qux]': [
        '/[locale]/nested/[foo]/[bar]/@slot1/[baz]/[qux]/page',
      ],
    }

    const initialAppPaths = JSON.parse(JSON.stringify(appPaths))
    normalizeCatchAllRoutes(appPaths)
    expect(appPaths).toMatchObject(initialAppPaths)
  })
...
```

```it('should not add the catch-all route to segments that have a more specific [dynamicRoute]', () => {
    const appPaths = {
      '/': ['/page'],
      '/[[...catchAll]]': ['/[[...catchAll]]/page'],
      '/nested/[foo]/[bar]/default': [
        '/nested/[foo]/[bar]/default',
        '/nested/[foo]/[bar]/@slot0/default',
        '/nested/[foo]/[bar]/@slot2/default',
      ],
      '/nested/[foo]/[bar]': [
        '/nested/[foo]/[bar]/@slot0/page',
        '/nested/[foo]/[bar]/@slot1/page',
      ],
      '/nested/[foo]/[bar]/[baz]': [
        '/nested/[foo]/[bar]/@slot0/[baz]/page',
        '/nested/[foo]/[bar]/@slot1/[baz]/page',
      ],
      '/[locale]/nested/[foo]/[bar]/[baz]/[qux]': [
        '/[locale]/nested/[foo]/[bar]/@slot1/[baz]/[qux]/page',
      ],
    }
...
```

and allow parallel routes defined in this [code
repro](https://github.com/williamli/nextjs-NEXT-2165) to build.


![image](https://github.com/vercel/next.js/assets/179761/030f4fe1-3a27-41e5-bbd9-bc511f95e5d7)


### How?

`packages/next/src/build/normalize-catchall-routes.ts` is extended to
check `appPath` to see if it is:
1. the route is not a catchall
2. `isMoreSpecific` than the closest `catchAllRoute`.


where `isMoreSpecific` is defined as:

```

function isMoreSpecific(pathname: string, catchAllRoute: string): boolean {
  const pathnameDepth = pathname.split('/').length
  const catchAllRouteDepth = catchAllRoute.split('/').length - 1
  return pathnameDepth > catchAllRouteDepth
}

```

---------

Co-authored-by: Zack Tanner <zacktanner@gmail.com>
ztanner added a commit that referenced this pull request Jan 27, 2024
Reverts changes from #61173 & #60240 (while leaving the tests that were
added).

There are too many spots where considering `/default` routes as pages
needs to be carefully considered in different runtimes, and it turns out
that it's not actually needed to handle the case that it was originally
added for. I confirmed that the test that added the case it was intended
to fix (`parallel-routes-catchall-default`, along with the unit tests in
`normalize-catchall-routes`) are still passing as expected.
@github-actions github-actions bot added the locked label Feb 1, 2024
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 1, 2024
@vercel vercel deleted a comment from williamli Feb 28, 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.

3 participants