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

Turbopack: next/dynamic use transitions instead of AST analysis #73627

Merged
merged 3 commits into from
Dec 19, 2024

Conversation

mischnic
Copy link
Contributor

@mischnic mischnic commented Dec 6, 2024

  • Before, there was a separate AST traversal that collected next-dynamic imports
  • Now, we use transitions (which are inserted by the existing next-custom-transform) to tell Turbopack about the next-dynamic imports
    • Unlike Webpack (where the react-loadable identifiers are path/to/parent-module -> ./imported-client), Turbopack just uses the module id: [project]/path/to/imported-client.js [app-client] (ecmascript, next/dynamic entry)
    • the next-dynamic transition only inserts the marker module (i.e. for the SSR import())
    • the next-dynamic-client transition inserts the marker module and changes the module context (used to get the client module name)
  • Now, we use the correct AvailabilityInfo for the next/dynamic chunks to preload, and they are not explicitly emitted. If they are for some reason not the same chunks that are actually emitted for the dynamic import, you get a hard error. (Previously, they didn't use the same AvailabilityInfo as the real async chunks, so they usually bundled another copy of React, and also had a different chunk name hash)
  • For Turbopack, react-loadable manifests are now scoped per page instead of globally

Closes PACK-3670

@mischnic mischnic changed the title WIP Turbopack: use transitions instead of separate AST analysis Dec 6, 2024
@mischnic mischnic force-pushed the mischnic/single-traversal-client-references branch from 9689752 to 112167d Compare December 9, 2024 14:59
@mischnic mischnic force-pushed the mischnic/next-dynamic-transition branch 2 times, most recently from af84449 to b9aa680 Compare December 9, 2024 15:14
@ijjk
Copy link
Member

ijjk commented Dec 9, 2024

Tests Passed

@ijjk
Copy link
Member

ijjk commented Dec 9, 2024

Stats from current PR

Default Build (Increase detected ⚠️)
General
vercel/next.js canary vercel/next.js mischnic/next-dynamic-transition Change
buildDuration 22.1s 20.6s N/A
buildDurationCached 19.3s 16.6s N/A
nodeModulesSize 416 MB 416 MB N/A
nextStartRea..uration (ms) 539ms 557ms N/A
Client Bundles (main, webpack)
vercel/next.js canary vercel/next.js mischnic/next-dynamic-transition Change
1187-HASH.js gzip 52.4 kB 52.4 kB N/A
8276.HASH.js gzip 169 B 168 B N/A
8377-HASH.js gzip 5.36 kB 5.36 kB N/A
bccd1874-HASH.js gzip 53 kB 53 kB N/A
framework-HASH.js gzip 57.5 kB 57.5 kB N/A
main-app-HASH.js gzip 232 B 235 B N/A
main-HASH.js gzip 34.1 kB 34.1 kB N/A
webpack-HASH.js gzip 1.71 kB 1.71 kB N/A
Overall change 0 B 0 B
Legacy Client Bundles (polyfills)
vercel/next.js canary vercel/next.js mischnic/next-dynamic-transition Change
polyfills-HASH.js gzip 39.4 kB 39.4 kB
Overall change 39.4 kB 39.4 kB
Client Pages
vercel/next.js canary vercel/next.js mischnic/next-dynamic-transition Change
_app-HASH.js gzip 193 B 193 B
_error-HASH.js gzip 193 B 193 B
amp-HASH.js gzip 512 B 510 B N/A
css-HASH.js gzip 343 B 342 B N/A
dynamic-HASH.js gzip 1.84 kB 1.84 kB
edge-ssr-HASH.js gzip 265 B 265 B
head-HASH.js gzip 363 B 362 B N/A
hooks-HASH.js gzip 393 B 392 B N/A
image-HASH.js gzip 4.49 kB 4.49 kB N/A
index-HASH.js gzip 268 B 268 B
link-HASH.js gzip 2.35 kB 2.34 kB N/A
routerDirect..HASH.js gzip 328 B 328 B
script-HASH.js gzip 397 B 397 B
withRouter-HASH.js gzip 323 B 326 B N/A
1afbb74e6ecf..834.css gzip 106 B 106 B
Overall change 3.59 kB 3.59 kB
Client Build Manifests
vercel/next.js canary vercel/next.js mischnic/next-dynamic-transition Change
_buildManifest.js gzip 749 B 746 B N/A
Overall change 0 B 0 B
Rendered Page Sizes
vercel/next.js canary vercel/next.js mischnic/next-dynamic-transition Change
index.html gzip 524 B 524 B
link.html gzip 539 B 537 B N/A
withRouter.html gzip 520 B 521 B N/A
Overall change 524 B 524 B
Edge SSR bundle Size
vercel/next.js canary vercel/next.js mischnic/next-dynamic-transition Change
edge-ssr.js gzip 129 kB 129 kB N/A
page.js gzip 205 kB 205 kB N/A
Overall change 0 B 0 B
Middleware size
vercel/next.js canary vercel/next.js mischnic/next-dynamic-transition Change
middleware-b..fest.js gzip 671 B 669 B N/A
middleware-r..fest.js gzip 155 B 156 B N/A
middleware.js gzip 31.3 kB 31.3 kB N/A
edge-runtime..pack.js gzip 844 B 844 B
Overall change 844 B 844 B
Next Runtimes
vercel/next.js canary vercel/next.js mischnic/next-dynamic-transition Change
274-experime...dev.js gzip 322 B 322 B
274.runtime.dev.js gzip 314 B 314 B
app-page-exp...dev.js gzip 356 kB 356 kB
app-page-exp..prod.js gzip 129 kB 129 kB
app-page-tur..prod.js gzip 142 kB 142 kB
app-page-tur..prod.js gzip 137 kB 137 kB
app-page.run...dev.js gzip 347 kB 347 kB
app-page.run..prod.js gzip 125 kB 125 kB
app-route-ex...dev.js gzip 37.5 kB 37.5 kB
app-route-ex..prod.js gzip 25.5 kB 25.5 kB
app-route-tu..prod.js gzip 25.5 kB 25.5 kB
app-route-tu..prod.js gzip 25.3 kB 25.3 kB
app-route.ru...dev.js gzip 39.1 kB 39.1 kB
app-route.ru..prod.js gzip 25.3 kB 25.3 kB
pages-api-tu..prod.js gzip 9.69 kB 9.69 kB
pages-api.ru...dev.js gzip 11.6 kB 11.6 kB
pages-api.ru..prod.js gzip 9.68 kB 9.68 kB
pages-turbo...prod.js gzip 21.7 kB 21.7 kB
pages.runtim...dev.js gzip 27.5 kB 27.5 kB
pages.runtim..prod.js gzip 21.7 kB 21.7 kB
server.runti..prod.js gzip 916 kB 916 kB N/A
Overall change 1.51 MB 1.51 MB
build cache Overall increase ⚠️
vercel/next.js canary vercel/next.js mischnic/next-dynamic-transition Change
0.pack gzip 2.08 MB 2.08 MB ⚠️ +2.79 kB
index.pack gzip 74.1 kB 73.6 kB N/A
Overall change 2.08 MB 2.08 MB ⚠️ +2.79 kB
Diff details
Diff for main-HASH.js

Diff too large to display

Diff for server.runtime.prod.js

Diff too large to display

Commit: 39b6afb

@mischnic mischnic force-pushed the mischnic/single-traversal-client-references branch from 112167d to 3f52131 Compare December 9, 2024 21:36
@mischnic mischnic force-pushed the mischnic/next-dynamic-transition branch from 2adb01d to ee5d4da Compare December 9, 2024 21:37
@mischnic mischnic force-pushed the mischnic/single-traversal-client-references branch from 3f52131 to bacf500 Compare December 10, 2024 12:14
@mischnic mischnic force-pushed the mischnic/next-dynamic-transition branch from ee5d4da to 23f58e0 Compare December 10, 2024 12:14
@mischnic mischnic force-pushed the mischnic/single-traversal-client-references branch from bacf500 to 9ad8cd2 Compare December 10, 2024 13:24
@mischnic mischnic force-pushed the mischnic/next-dynamic-transition branch from 0042451 to 1316e1a Compare December 10, 2024 13:25
@mischnic mischnic force-pushed the mischnic/single-traversal-client-references branch 2 times, most recently from b36b605 to fc04007 Compare December 10, 2024 14:56
@mischnic mischnic force-pushed the mischnic/next-dynamic-transition branch from 1316e1a to 6d6f0f6 Compare December 10, 2024 14:56
@mischnic mischnic force-pushed the mischnic/single-traversal-client-references branch from fc04007 to 2ee338c Compare December 10, 2024 15:14
@mischnic mischnic force-pushed the mischnic/next-dynamic-transition branch from 6d6f0f6 to 32feeb3 Compare December 10, 2024 15:14
@mischnic mischnic force-pushed the mischnic/single-traversal-client-references branch from 2ee338c to 155b5ed Compare December 10, 2024 15:40
@mischnic mischnic force-pushed the mischnic/next-dynamic-transition branch from 72cf773 to a76215f Compare December 10, 2024 15:41
@mischnic mischnic force-pushed the mischnic/single-traversal-client-references branch from 155b5ed to d3f5611 Compare December 10, 2024 15:43
@mischnic mischnic force-pushed the mischnic/next-dynamic-transition branch 2 times, most recently from 17c44a0 to aa18202 Compare December 10, 2024 16:26
@mischnic mischnic force-pushed the mischnic/single-traversal-client-references branch from 3960bda to 3f5ca85 Compare December 11, 2024 11:30
@mischnic mischnic force-pushed the mischnic/next-dynamic-transition branch from 37d5190 to da2b3ff Compare December 11, 2024 11:31
@mischnic mischnic changed the base branch from mischnic/single-traversal-client-references to graphite-base/73627 December 11, 2024 12:13
@mischnic mischnic force-pushed the mischnic/next-dynamic-transition branch from da2b3ff to eedd94d Compare December 11, 2024 12:16
@mischnic mischnic force-pushed the graphite-base/73627 branch from 3f5ca85 to 8c66f84 Compare December 11, 2024 12:16
@mischnic mischnic changed the base branch from graphite-base/73627 to canary December 11, 2024 12:16
@mischnic mischnic force-pushed the mischnic/next-dynamic-transition branch 2 times, most recently from 5594d28 to cff55cf Compare December 11, 2024 13:27
Comment on lines +877 to +891
// The SSR and Client Graphs are not connected in Pages Router.
// We are only interested in get_next_dynamic_imports_for_endpoint at the
// moment, which only needs the client graph anyway.
//
// If we do want to change this to have both included. We'd need to create a
// `IncludeModulesModule` that includes both SSR and Client (and use that both
// there and in Project::get_all_entries):
// let client_module = self.client_module().to_resolved().await?;
// let ssr_module = self.internal_ssr_chunk_module().await?.ssr_module;
// Ok(Vc::upcast(IncludeModulesModule::new(
// self.source()
// .ident()
// .with_modifier(Vc::cell("unified entrypoint".into())),
// vec![*client_module, *ssr_module],
// )))
Copy link
Contributor Author

@mischnic mischnic Dec 11, 2024

Choose a reason for hiding this comment

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

@wbinnssmith You might also run into this at some point.
And, well, turns out I'll might this for the global module-ids anyway...

@mischnic mischnic marked this pull request as ready for review December 11, 2024 13:43
@mischnic mischnic requested a review from sokra December 11, 2024 13:47
@mischnic mischnic force-pushed the mischnic/next-dynamic-transition branch from 2524b3f to 2cbee9e Compare December 13, 2024 13:34
@mischnic mischnic changed the title Turbopack: use transitions instead of separate AST analysis Turbopack: next/dynamic use transitions instead of AST analysis Dec 16, 2024
@mischnic mischnic force-pushed the mischnic/next-dynamic-transition branch 3 times, most recently from 0fe6367 to 3cf7824 Compare December 18, 2024 10:31
Comment on lines +971 to +972
let loadable_manifest_output =
self.react_loadable_manifest(*dynamic_import_entries, NextRuntime::NodeJs);
Copy link
Member

Choose a reason for hiding this comment

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

Is the next dynamic manifest not using in development?

Copy link
Contributor Author

@mischnic mischnic Dec 18, 2024

Choose a reason for hiding this comment

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

It is. This is only to get the filename to add to the NFT list

Further down in fn output() it is called again and added to the list of output assets (also in dev).

@@ -0,0 +1,17 @@
import { __turbopack_module_id__ as id } from "../text-dynamic-no-ssr-server" with {
"turbopack-transition": "next-client-dynamic",
"turbopack-chunking-type": "none"
Copy link
Member

Choose a reason for hiding this comment

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

Does it work in the single graph when it doesn't have a chunking type?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. This subgraph is still visited when the EcmascriptClientReference reference with ChunkingType::Isolated is walked.

@mischnic mischnic force-pushed the mischnic/next-dynamic-transition branch 2 times, most recently from ac6fdf4 to 533d4b1 Compare December 18, 2024 15:38
@mischnic mischnic force-pushed the mischnic/next-dynamic-transition branch from 533d4b1 to 39b6afb Compare December 19, 2024 09:04
@mischnic mischnic merged commit d3529ba into canary Dec 19, 2024
127 of 132 checks passed
@mischnic mischnic deleted the mischnic/next-dynamic-transition branch December 19, 2024 11:34
@github-actions github-actions bot added the locked label Jan 3, 2025
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jan 3, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
created-by: Turbopack team PRs by the Turbopack team. locked tests Turbopack Related to Turbopack with Next.js. type: next
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants