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 parallel route top-level catch-all normalization logic to support nested explicit (non-catchall) slot routes #60776

Conversation

williamli
Copy link
Contributor

@williamli williamli commented Jan 17, 2024

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:

CleanShot 2024-01-18 at 02 12 36@2x

local dev server will also crash with:

CleanShot 2024-01-18 at 02 13 19@2x

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)
  })
...
    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 to build.

image

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
}

@ijjk
Copy link
Member

ijjk commented Jan 17, 2024

Failing test suites

Commit: 27d096d

pnpm test-dev test/e2e/app-dir/not-found/basic/index.test.ts (PPR)

  • app dir - not-found - basic > with runtime = edge > should render the 404 page when the file is removed, and restore the page when re-added
Expand output

● app dir - not-found - basic › with runtime = edge › should render the 404 page when the file is removed, and restore the page when re-added

TIMED OUT: Root Not Found

My page

TimeoutError: page.waitForSelector: Timeout 60000ms exceeded.
=========================== logs ===========================
waiting for locator('h1')
============================================================

  636 |
  637 |   if (hardError) {
> 638 |     throw new Error('TIMED OUT: ' + regex + '\n\n' + content + '\n\n' + lastErr)
      |           ^
  639 |   }
  640 |   return false
  641 | }

  at check (lib/next-test-utils.ts:638:11)
  at Object.<anonymous> (e2e/app-dir/not-found/basic/index.test.ts:123:11)

Read more about building and testing Next.js in contributing.md.

@ijjk
Copy link
Member

ijjk commented Jan 17, 2024

Stats from current PR

Default Build
General Overall increase ⚠️
vercel/next.js canary williamli/next.js fix-parallel-routes-top-level-catchall-but-no-default Change
buildDuration 11.7s 11.8s N/A
buildDurationCached 6s 5s N/A
nodeModulesSize 200 MB 200 MB ⚠️ +3.04 kB
nextStartRea..uration (ms) 424ms 433ms N/A
Client Bundles (main, webpack)
vercel/next.js canary williamli/next.js fix-parallel-routes-top-level-catchall-but-no-default Change
193.HASH.js gzip 181 B 182 B N/A
3f784ff6-HASH.js gzip 53.4 kB 53.4 kB
433-HASH.js gzip 29 kB 29 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.8 kB 31.8 kB N/A
webpack-HASH.js gzip 1.7 kB 1.7 kB N/A
Overall change 98.6 kB 98.6 kB
Legacy Client Bundles (polyfills)
vercel/next.js canary williamli/next.js fix-parallel-routes-top-level-catchall-but-no-default Change
polyfills-HASH.js gzip 31 kB 31 kB
Overall change 31 kB 31 kB
Client Pages
vercel/next.js canary williamli/next.js fix-parallel-routes-top-level-catchall-but-no-default 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.18 kB 4.18 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 williamli/next.js fix-parallel-routes-top-level-catchall-but-no-default Change
_buildManifest.js gzip 484 B 485 B N/A
Overall change 0 B 0 B
Rendered Page Sizes
vercel/next.js canary williamli/next.js fix-parallel-routes-top-level-catchall-but-no-default Change
index.html gzip 528 B 525 B N/A
link.html gzip 540 B 538 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 williamli/next.js fix-parallel-routes-top-level-catchall-but-no-default Change
edge-ssr.js gzip 94 kB 94 kB N/A
page.js gzip 149 kB 149 kB N/A
Overall change 0 B 0 B
Middleware size
vercel/next.js canary williamli/next.js fix-parallel-routes-top-level-catchall-but-no-default Change
middleware-b..fest.js gzip 626 B 627 B N/A
middleware-r..fest.js gzip 151 B 151 B
middleware.js gzip 37.5 kB 37.5 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 williamli/next.js fix-parallel-routes-top-level-catchall-but-no-default Change
app-page-exp...dev.js gzip 169 kB 169 kB
app-page-exp..prod.js gzip 95.6 kB 95.6 kB
app-page-tur..prod.js gzip 96.3 kB 96.3 kB
app-page-tur..prod.js gzip 90.9 kB 90.9 kB
app-page.run...dev.js gzip 142 kB 142 kB
app-page.run..prod.js gzip 90.2 kB 90.2 kB
app-route-ex...dev.js gzip 24.2 kB 24.2 kB
app-route-ex..prod.js gzip 16.9 kB 16.9 kB
app-route-tu..prod.js gzip 16.9 kB 16.9 kB
app-route-tu..prod.js gzip 16.4 kB 16.4 kB
app-route.ru...dev.js gzip 23.6 kB 23.6 kB
app-route.ru..prod.js gzip 16.4 kB 16.4 kB
pages-api-tu..prod.js gzip 9.41 kB 9.41 kB
pages-api.ru...dev.js gzip 9.68 kB 9.68 kB
pages-api.ru..prod.js gzip 9.4 kB 9.4 kB
pages-turbo...prod.js gzip 22 kB 22 kB
pages.runtim...dev.js gzip 22.6 kB 22.6 kB
pages.runtim..prod.js gzip 22 kB 22 kB
server.runti..prod.js gzip 49.7 kB 49.7 kB
Overall change 944 kB 944 kB
Commit: 21ffa68

@williamli williamli force-pushed the fix-parallel-routes-top-level-catchall-but-no-default branch from c4c91a9 to 4a865c5 Compare January 17, 2024 18:01
@williamli williamli changed the title prevent catchall from taking control if appPath ends in a dynamic route fix catch-all route normalization to support slot specific [dynamicPageRoute] Jan 17, 2024
@williamli williamli changed the title fix catch-all route normalization to support slot specific [dynamicPageRoute] fix catch-all route normalization to support slot specific [dynamicRouteSegment] Jan 17, 2024
@williamli williamli marked this pull request as ready for review January 17, 2024 18:20
@williamli williamli changed the title fix catch-all route normalization to support slot specific [dynamicRouteSegment] fix parallel route catch-all normalization logic to support slot specific [dynamicRouteSegment] Jan 18, 2024
@williamli williamli changed the title fix parallel route catch-all normalization logic to support slot specific [dynamicRouteSegment] fix parallel route top-level catch-all normalization logic to support nested slot specific non-catchall routes Jan 19, 2024
@williamli williamli marked this pull request as draft January 19, 2024 07:02
@williamli williamli marked this pull request as ready for review January 19, 2024 07:40
@williamli williamli marked this pull request as draft January 19, 2024 08:05
@williamli williamli marked this pull request as ready for review January 19, 2024 08:12
@williamli williamli changed the title fix parallel route top-level catch-all normalization logic to support nested slot specific non-catchall routes fix parallel route top-level catch-all normalization logic to support nested explicit (non-catchall) slot routes Jan 19, 2024
williamli and others added 5 commits January 23, 2024 10:34
…llel-routes-catchall-dynamic-segment.test.ts

Co-authored-by: Zack Tanner <zacktanner@gmail.com>
…llel-routes-catchall-dynamic-segment.test.ts

Co-authored-by: Zack Tanner <zacktanner@gmail.com>
…s/app/[locale]/nested/layout.tsx

Co-authored-by: Zack Tanner <zacktanner@gmail.com>
Copy link
Member

@ztanner ztanner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@ztanner ztanner enabled auto-merge (squash) January 23, 2024 22:04
@ztanner ztanner merged commit 71335a9 into vercel:canary Jan 23, 2024
61 of 66 checks passed
@github-actions github-actions bot added the locked label Feb 7, 2024
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 7, 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.

None yet

3 participants