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.

Sign up for GitHub

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

Jump to bottom

fix: force module format for virtual client-proxy file #74162

Merged
merged 2 commits into from
Jan 7, 2025
Merged

fix: force module format for virtual client-proxy file #74162

merged 2 commits into from
Jan 7, 2025
+131 −1

Conversation

lubieowoce
Copy link
Member

@lubieowoce lubieowoce commented Dec 19, 2024 •
edited
Loading

Fixes #74062 (jotai ran into this error when they added "type": "commonjs" to their package.json)

this is a bug in the transform we do when a 'use client' directive is encountered. I think what's happening is that we're creating a virtual file that uses ESM import/export syntax, but it's called proxy.js (not proxy.mjs), so the "type": "commonjs" makes turbopack "correctly" upset at the ESM syntax.
#74062 (comment)

The (slightly kludgy) solution is to use proxy.mjs or proxy.cjs to force the module format, bypassing the issue where proxy.js changes meaning depending on package.json#type.

@ijjk ijjk added created-by: Next.js team PRs by the Next.js team. Turbopack Related to Turbopack with Next.js. labels Dec 19, 2024
@lubieowoce
Copy link
Member Author

lubieowoce commented Dec 19, 2024 •
edited
Loading

Honestly, this feels like a bit of a workaround for the fact that we're creating this proxy.js module "beside" the original code, and thus are affected by its package.json. maybe we should just stop doing that, and place it somewhere else? not sure where's a good location for that. suggestions welcome

@lubieowoce lubieowoce changed the title fix(turbopack): use explicit mjs/cjs extension for virtual proxy file fix: use explicit mjs/cjs extension for virtual proxy file Dec 19, 2024
@lubieowoce lubieowoce force-pushed the lubieowoce/fix-client-transform-mjs branch from 6f3c648 to 103c65d Compare December 19, 2024 23:46
@ijjk
Copy link
Member

ijjk commented Dec 19, 2024 •
edited
Loading

Stats from current PR

Default Build (Increase detected ⚠️)
General
vercel/next.js canary vercel/next.js lubieowoce/fix-client-transform-mjs Change
buildDuration 30.9s 30.3s N/A
buildDurationCached 27.3s 22.1s N/A
nodeModulesSize 417 MB 417 MB ✓
nextStartRea..uration (ms) 848ms 687ms N/A
Client Bundles (main, webpack)
vercel/next.js canary vercel/next.js lubieowoce/fix-client-transform-mjs Change
1187-HASH.js gzip 52.6 kB 52.6 kB N/A
8276.HASH.js gzip 169 B 168 B N/A
8377-HASH.js gzip 5.44 kB 5.44 kB N/A
bccd1874-HASH.js gzip 52.9 kB 52.9 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 lubieowoce/fix-client-transform-mjs 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 lubieowoce/fix-client-transform-mjs 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.57 kB 4.57 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 lubieowoce/fix-client-transform-mjs Change
_buildManifest.js gzip 749 B 747 B N/A
Overall change 0 B 0 B ✓
Rendered Page Sizes
vercel/next.js canary vercel/next.js lubieowoce/fix-client-transform-mjs Change
index.html gzip 523 B 523 B ✓
link.html gzip 538 B 535 B N/A
withRouter.html gzip 519 B 519 B ✓
Overall change 1.04 kB 1.04 kB ✓
Edge SSR bundle Size
vercel/next.js canary vercel/next.js lubieowoce/fix-client-transform-mjs Change
edge-ssr.js gzip 128 kB 128 kB N/A
page.js gzip 206 kB 206 kB N/A
Overall change 0 B 0 B ✓
Middleware size
vercel/next.js canary vercel/next.js lubieowoce/fix-client-transform-mjs Change
middleware-b..fest.js gzip 672 B 667 B N/A
middleware-r..fest.js gzip 155 B 156 B N/A
middleware.js gzip 31.2 kB 31.2 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 lubieowoce/fix-client-transform-mjs 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 363 kB 363 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 138 kB 138 kB ✓
app-page.run...dev.js gzip 352 kB 352 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.4 kB 25.4 kB ✓
app-route.ru...dev.js gzip 39.2 kB 39.2 kB ✓
app-route.ru..prod.js gzip 25.4 kB 25.4 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 ✓
Overall change 2.44 MB 2.44 MB ✓
build cache Overall increase ⚠️
vercel/next.js canary vercel/next.js lubieowoce/fix-client-transform-mjs Change
0.pack gzip 2.08 MB 2.09 MB ⚠️ +3.92 kB
index.pack gzip 75.5 kB 75 kB N/A
Overall change 2.08 MB 2.09 MB ⚠️ +3.92 kB
Diff details
Diff for main-HASH.js

Diff too large to display

Commit: 520d4e1

@ijjk ijjk added the tests label Dec 20, 2024
@lubieowoce lubieowoce changed the title fix: use explicit mjs/cjs extension for virtual proxy file fix: force module format in virtual client proxy file Dec 20, 2024
@lubieowoce lubieowoce changed the title fix: force module format in virtual client proxy file fix: force module format in virtual client-proxy file Dec 20, 2024
@lubieowoce lubieowoce requested a review from kdy1 December 20, 2024 01:02
@lubieowoce lubieowoce marked this pull request as ready for review December 20, 2024 01:02
@lubieowoce lubieowoce requested a review from huozhi December 20, 2024 01:03
@lubieowoce lubieowoce changed the title fix: force module format in virtual client-proxy file fix: force module format for virtual client-proxy file Dec 20, 2024
@lubieowoce lubieowoce force-pushed the lubieowoce/fix-client-transform-mjs branch from ae6b69b to 0c8a5e8 Compare December 20, 2024 01:33
lubieowoce
lubieowoce commented Dec 20, 2024
View reviewed changes
test/e2e/app-dir/client-module-with-package-type/app/import-cjs/page.tsx
Copy link
Member Author

@lubieowoce lubieowoce Dec 20, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

this is the case that matches what happened with jotai. other permutations are just for completeness.

@lubieowoce lubieowoce force-pushed the lubieowoce/fix-client-transform-mjs branch from 0c8a5e8 to 2e01e20 Compare December 20, 2024 01:36
@ijjk
Copy link
Member

ijjk commented Dec 20, 2024 •
edited
Loading

Tests Passed

@lubieowoce lubieowoce force-pushed the lubieowoce/fix-client-transform-mjs branch from 2e01e20 to 520d4e1 Compare January 6, 2025 15:37
@lubieowoce lubieowoce merged commit 15bb8ce into canary Jan 7, 2025
130 checks passed
@lubieowoce lubieowoce deleted the lubieowoce/fix-client-transform-mjs branch January 7, 2025 13:50
lubieowoce added a commit that referenced this pull request Jan 7, 2025
@lubieowoce
fix: force module format for virtual client-proxy file (#74162)
b80fdd2 
Fixes #74062 (`jotai` ran into this error [when they added `"type":
"commonjs"` to their
package.json](pmndrs/jotai#2579 (reply in thread)))

> this is a bug in the transform we do when a `'use client'` directive
is encountered. I think what's happening is that we're creating a
virtual file that uses ESM import/export syntax, but it's called
proxy.js (not proxy.mjs), so the `"type": "commonjs" `makes turbopack
"correctly" upset at the ESM syntax.
#74062 (comment)

The (slightly kludgy) solution is to use `proxy.mjs` or `proxy.cjs` to
force the module format, bypassing the issue where `proxy.js` changes
meaning depending on `package.json#type`.
lubieowoce added a commit that referenced this pull request Jan 7, 2025
@lubieowoce
fix: force module format for virtual client-proxy file (#74162)
0aa8020 
Fixes #74062 (`jotai` ran into this error [when they added `"type":
"commonjs"` to their
package.json](pmndrs/jotai#2579 (reply in thread)))

> this is a bug in the transform we do when a `'use client'` directive
is encountered. I think what's happening is that we're creating a
virtual file that uses ESM import/export syntax, but it's called
proxy.js (not proxy.mjs), so the `"type": "commonjs" `makes turbopack
"correctly" upset at the ESM syntax.
#74062 (comment)

The (slightly kludgy) solution is to use `proxy.mjs` or `proxy.cjs` to
force the module format, bypassing the issue where `proxy.js` changes
meaning depending on `package.json#type`.
lubieowoce added a commit that referenced this pull request Jan 7, 2025
@lubieowoce
backport: force module format for virtual client-proxy (#74162) (#74590)
c4bf4ac 
Fixes #74062 (`jotai` ran into this error [when they added `"type":
"commonjs"` to their

package.json](pmndrs/jotai#2579 (reply in thread)))

> this is a bug in the transform we do when a `'use client'` directive
is encountered. I think what's happening is that we're creating a
virtual file that uses ESM import/export syntax, but it's called
proxy.js (not proxy.mjs), so the `"type": "commonjs" `makes turbopack
"correctly" upset at the ESM syntax.
#74062 (comment)

The (slightly kludgy) solution is to use `proxy.mjs` or `proxy.cjs` to
force the module format, bypassing the issue where `proxy.js` changes
meaning depending on `package.json#type`.
lubieowoce added a commit that referenced this pull request Jan 7, 2025
@lubieowoce
fix: force module format for virtual client-proxy file (#74162)
8a76b0c 
Fixes #74062 (`jotai` ran into this error [when they added `"type":
"commonjs"` to their
package.json](pmndrs/jotai#2579 (reply in thread)))

> this is a bug in the transform we do when a `'use client'` directive
is encountered. I think what's happening is that we're creating a
virtual file that uses ESM import/export syntax, but it's called
proxy.js (not proxy.mjs), so the `"type": "commonjs" `makes turbopack
"correctly" upset at the ESM syntax.
#74062 (comment)

The (slightly kludgy) solution is to use `proxy.mjs` or `proxy.cjs` to
force the module format, bypassing the issue where `proxy.js` changes
meaning depending on `package.json#type`.
lubieowoce added a commit that referenced this pull request Jan 7, 2025
@lubieowoce
backport: force module format for virtual client-proxy (#74162) (#74608)
dcb208e 
Fixes #74062 (`jotai` ran into this error [when they added `"type":
"commonjs"` to their
package.json](pmndrs/jotai#2579 (reply in thread)))

> this is a bug in the transform we do when a `'use client'` directive
is encountered. I think what's happening is that we're creating a
virtual file that uses ESM import/export syntax, but it's called
proxy.js (not proxy.mjs), so the `"type": "commonjs" `makes turbopack
"correctly" upset at the ESM syntax.
#74062 (comment)

The (slightly kludgy) solution is to use `proxy.mjs` or `proxy.cjs` to
force the module format, bypassing the issue where `proxy.js` changes
meaning depending on `package.json#type`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@eps1lon eps1lon eps1lon approved these changes

@kdy1 kdy1 Awaiting requested review from kdy1

@huozhi huozhi Awaiting requested review from huozhi

@unstubbable unstubbable Awaiting requested review from unstubbable

Assignees
No one assigned
Labels
created-by: Next.js team PRs by the Next.js team. tests Turbopack Related to Turbopack with Next.js.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Turbopack does not handle CommonJS modules properly when "type": "commonjs" is specified in package.json
3 participants
@lubieowoce @ijjk @eps1lon