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

feat(turbopack): support basic next/dynamic #56389

Merged
merged 19 commits into from Oct 16, 2023
Merged

Conversation

kwonoj
Copy link
Contributor

@kwonoj kwonoj commented Oct 4, 2023

Closes WEB-1702

This PR implements initial support for the next/dynamic in Turbopack, more specifically resolving some hydration errors and other components boot up cases.

Previously, turbopack had partial next/dynamic support via its own mode (https://github.com/vercel/next.js/pull/56389/files#diff-e1af4f79cb88a73f819a25443d15ed4b1ffabcbb879256caa59b751fad46d7c4L68), which does a transform against next/dynamic wrapped import to embed dynamically resolvable chunk ids like (https://github.com/vercel/next.js/blob/ad42b610c25b72561ad367b82b1c7383fd2a5dd2/packages/next-swc/crates/next-transform-dynamic/tests/fixture/wrapped-import/output-turbo-dev-server.js).

However, since next.js relies on static path to the chunks to the dynamic import and passing those ids in between client-server to ensure component load (and avoid hydration errors), it doesn't work out of the box. This PR changes turbopack's behavior to closely mimic what current next.js's webpack plugin does, by

  1. Traverse the module graph, find out dynamic(import())
  2. Generate chunks for those imports, creates a partial LoadableManifest per each imports
  3. Merge partial manifest into a single react-loadable-manifest.json
  4. For the id, use static (Webpack mode) instead of dynamic so we can embed it in react-loadable-manifest as well as next.js can use it to pass it between server-client context.

I left a small comment to the implementation (https://github.com/vercel/next.js/pull/56389/files#diff-bf12ed2c69d0bc89a06884779da4ae44967eb8becada031dea12bedef28e2622R155) for the lifecycle of this feature in case to fix further.

This makes to pass most of the basic next-dynamic related integration tests, except if the import have webpack specific features like

import(/* webpackChunkName: 'hello1' */ '../../components/hello3')
.

@ijjk ijjk added Turbopack Related to Turbopack with Next.js. created-by: Turbopack team PRs by the turbopack team type: next labels Oct 4, 2023
@kwonoj kwonoj changed the title feat(turbopack): support basic next/dynamic [DONOTMERGE] [WIP] feat(turbopack): support basic next/dynamic Oct 4, 2023
@ijjk
Copy link
Member

ijjk commented Oct 4, 2023

Tests Passed

@ijjk
Copy link
Member

ijjk commented Oct 4, 2023

Stats from current PR

Default Build
General Overall increase ⚠️
vercel/next.js canary vercel/next.js feat-dynamic-id Change
buildDuration 10.2s 10.3s N/A
buildDurationCached 6s 6.6s ⚠️ +579ms
nodeModulesSize 172 MB 172 MB ⚠️ +5.69 kB
nextStartRea..uration (ms) 531ms 525ms N/A
Client Bundles (main, webpack)
vercel/next.js canary vercel/next.js feat-dynamic-id Change
199-HASH.js gzip 27.5 kB 27.5 kB N/A
3f784ff6-HASH.js gzip 50.9 kB 50.9 kB
99.HASH.js gzip 182 B 182 B
framework-HASH.js gzip 45.3 kB 45.3 kB
main-app-HASH.js gzip 254 B 252 B N/A
main-HASH.js gzip 32.9 kB 32.9 kB N/A
webpack-HASH.js gzip 1.75 kB 1.75 kB N/A
Overall change 96.4 kB 96.4 kB
Legacy Client Bundles (polyfills)
vercel/next.js canary vercel/next.js feat-dynamic-id Change
polyfills-HASH.js gzip 31 kB 31 kB
Overall change 31 kB 31 kB
Client Pages
vercel/next.js canary vercel/next.js feat-dynamic-id Change
_app-HASH.js gzip 206 B 205 B N/A
_error-HASH.js gzip 182 B 180 B N/A
amp-HASH.js gzip 506 B 505 B N/A
css-HASH.js gzip 322 B 323 B N/A
dynamic-HASH.js gzip 2.57 kB 2.57 kB N/A
edge-ssr-HASH.js gzip 260 B 259 B N/A
head-HASH.js gzip 350 B 350 B
hooks-HASH.js gzip 369 B 369 B
image-HASH.js gzip 4.35 kB 4.35 kB N/A
index-HASH.js gzip 256 B 256 B
link-HASH.js gzip 2.64 kB 2.63 kB N/A
routerDirect..HASH.js gzip 312 B 311 B N/A
script-HASH.js gzip 385 B 384 B N/A
withRouter-HASH.js gzip 307 B 308 B N/A
1afbb74e6ecf..834.css gzip 106 B 106 B
Overall change 1.08 kB 1.08 kB
Client Build Manifests
vercel/next.js canary vercel/next.js feat-dynamic-id Change
_buildManifest.js gzip 485 B 482 B N/A
Overall change 0 B 0 B
Rendered Page Sizes
vercel/next.js canary vercel/next.js feat-dynamic-id Change
index.html gzip 529 B 530 B N/A
link.html gzip 542 B 542 B
withRouter.html gzip 524 B 525 B N/A
Overall change 542 B 542 B
Edge SSR bundle Size
vercel/next.js canary vercel/next.js feat-dynamic-id Change
edge-ssr.js gzip 93.7 kB 93.7 kB N/A
page.js gzip 152 kB 152 kB N/A
Overall change 0 B 0 B
Middleware size
vercel/next.js canary vercel/next.js feat-dynamic-id Change
middleware-b..fest.js gzip 624 B 623 B N/A
middleware-r..fest.js gzip 150 B 151 B N/A
middleware.js gzip 22.5 kB 22.5 kB N/A
edge-runtime..pack.js gzip 1.92 kB 1.92 kB
Overall change 1.92 kB 1.92 kB
Commit: bca73ec

@kwonoj kwonoj force-pushed the feat-dynamic-id branch 10 times, most recently from 183c42a to 8c24187 Compare October 4, 2023 22:09
@kwonoj kwonoj changed the title [DONOTMERGE] [WIP] feat(turbopack): support basic next/dynamic feat(turbopack): support basic next/dynamic Oct 4, 2023
@kwonoj kwonoj marked this pull request as ready for review October 4, 2023 22:10
huozhi
huozhi previously approved these changes Oct 4, 2023
Copy link
Contributor

@jridgewell jridgewell left a comment

Choose a reason for hiding this comment

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

Unfortunately this will have some conflicts with the chunking refactor that have landed this week.

Copy link
Member

@timneutkens timneutkens left a comment

Choose a reason for hiding this comment

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

Great work OJ!

@timneutkens timneutkens merged commit 5b52e77 into canary Oct 16, 2023
53 of 58 checks passed
@timneutkens timneutkens deleted the feat-dynamic-id branch October 16, 2023 08:24
Comment on lines -717 to +785
Vc::cell(evaluatable_assets),
Vc::cell(evaluatable_assets.clone()),
Copy link
Member

Choose a reason for hiding this comment

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

You can avoid the clone, by creating only one cell and use that for both calls:

let evaluatable_assets = Vc::cell(evaluatable_assets)

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 Turbopack Related to Turbopack with Next.js. type: next
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants