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

parallel routes: fix @children slots #60288

Merged

Conversation

ztanner
Copy link
Member

@ztanner ztanner commented Jan 5, 2024

What?

Our docs point out that app/page.js is equivalent to app/@children/page.js, however in practice this is not the case, and causes type errors when using @children slots as well as incorrect behavior when matching catch-all routes.

Why?

  • When typechecking, @children slots would be added to the typeguard file for the associated layout, resulting in duplicate identifiers for the children prop
  • When determining where to insert catchall slots, the hasMatchedSlots check wasn't considering that the @children slot corresponds with the page component, so matching another page would clobber the previous one.

How?

  • Filters out the @children slot when collecting slots for typechecking
  • Filters out the @children slot when running the hasMatchedSlots function in the catch-all normalizer

Closes NEXT-1984

Copy link
Member Author

ztanner commented Jan 5, 2024

Current dependencies on/for this PR:

This stack of pull requests is managed by Graphite.

@ijjk
Copy link
Member

ijjk commented Jan 5, 2024

Stats from current PR

Default Build
General Overall increase ⚠️
vercel/next.js canary vercel/next.js 01-05-ensure_children_slots_work_with_catch-all_routes Change
buildDuration 13s 12.8s N/A
buildDurationCached 7.3s 6.3s N/A
nodeModulesSize 200 MB 200 MB ⚠️ +3.12 kB
nextStartRea..uration (ms) 434ms 423ms N/A
Client Bundles (main, webpack)
vercel/next.js canary vercel/next.js 01-05-ensure_children_slots_work_with_catch-all_routes 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-05-ensure_children_slots_work_with_catch-all_routes 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-05-ensure_children_slots_work_with_catch-all_routes 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-05-ensure_children_slots_work_with_catch-all_routes 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-05-ensure_children_slots_work_with_catch-all_routes Change
index.html gzip 528 B 525 B N/A
link.html gzip 540 B 540 B
withRouter.html gzip 523 B 523 B
Overall change 1.06 kB 1.06 kB
Edge SSR bundle Size
vercel/next.js canary vercel/next.js 01-05-ensure_children_slots_work_with_catch-all_routes 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-05-ensure_children_slots_work_with_catch-all_routes Change
middleware-b..fest.js gzip 625 B 624 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-05-ensure_children_slots_work_with_catch-all_routes 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: e4662b9

@ztanner ztanner force-pushed the 01-05-ensure_children_slots_work_with_catch-all_routes branch 3 times, most recently from 010defb to 2b250d1 Compare January 5, 2024 22:03
@ztanner ztanner changed the title ensure @children slots work with catch-all routes ensure @children slots work Jan 5, 2024
@ijjk
Copy link
Member

ijjk commented Jan 5, 2024

Tests Passed

@ztanner ztanner force-pushed the 01-05-ensure_children_slots_work_with_catch-all_routes branch from 2b250d1 to e4662b9 Compare January 5, 2024 22:22
@ztanner ztanner marked this pull request as ready for review January 5, 2024 23:03
@ztanner ztanner changed the title ensure @children slots work parallel routes: fix @children slots work Jan 6, 2024
@ztanner ztanner changed the title parallel routes: fix @children slots work parallel routes: fix @children slots Jan 6, 2024
@ztanner ztanner merged commit efebba8 into canary Jan 6, 2024
73 checks passed
@ztanner ztanner deleted the 01-05-ensure_children_slots_work_with_catch-all_routes branch January 6, 2024 15:24
agustints pushed a commit to agustints/next.js that referenced this pull request Jan 6, 2024
### What?
Our
[docs](https://nextjs.org/docs/app/building-your-application/routing/parallel-routes#convention)
point out that `app/page.js` is equivalent to `app/@children/page.js`,
however in practice this is not the case, and causes type errors when
using `@children` slots as well as incorrect behavior when matching
catch-all routes.

### Why?
- When typechecking, `@children` slots would be added to the typeguard
file for the associated layout, resulting in duplicate identifiers for
the `children` prop
- When determining where to insert catchall slots, the `hasMatchedSlots`
check wasn't considering that the `@children` slot corresponds with the
page component, so matching another page would clobber the previous one.

### How?
- Filters out the `@children` slot when collecting slots for
typechecking
- Filters out the `@children` slot when running the `hasMatchedSlots`
function in the catch-all normalizer

Closes NEXT-1984
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jan 21, 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.

2 participants