-
Notifications
You must be signed in to change notification settings - Fork 28.8k
feat(turbopack): externalType support global #80542
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
Conversation
Allow CI Workflow Run
Note: this should only be enabled once the PR is ready to go and can only be enabled by a maintainer |
CachedExternalType::Global => { | ||
if self.request.is_empty() { | ||
writeln!(code, "const mod = {{}};")?; | ||
} else if self.request.contains('/') { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(asking because I did a quick search and couldn't easily find an answer) How does webpack do this? Does it use '/'
or '.'
, or does it just not support nested objects in this string?
We could just make you import/require the top-level object, and then it's up to the import to do the property access. This is an external, so concerns like fine-grained dependency tracking and tree-shaking don't apply.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this seems invalid here, webpack can deal the case like:
externals: {
'antd/es/button': ['global', 'antd', 'es', 'button']
}
it can generate global['antd']['es']['button']
, this is becase the request
of webpack can accept both string
and string[]
: https://github.com/webpack/webpack/blob/main/lib/ExternalModule.js#L727
For turbopack case, the request now only accept a RcStr
:
I think i can remove my branch here, and later i will send a pr to turbopack for the external.request
to accept both RcStr
and Vec<RcStr>
, it will align with webpack
@@ -109,4 +111,5 @@ pub const TUBROPACK_RUNTIME_FUNCTION_SHORTCUTS: [(&str, &TurbopackRuntimeFunctio | |||
("__turbopack_refresh__", TURBOPACK_REFRESH), | |||
("__turbopack_require_stub__", TURBOPACK_REQUIRE_STUB), | |||
("__turbopack_require_real__", TURBOPACK_REQUIRE_REAL), | |||
("__turbopack_global_access__", TURBOPACK_GLOBAL_ACCESS), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you need to implement this function?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it seems the function has been removed, from pr: https://github.com/vercel/next.js/pull/78889/files
i will use globalThis
directly instead of using __turbopack_context__.g
here, they are equal
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This has CI failures:
error[E0432]: unresolved import `crate::runtime_functions::TUBROPACK_RUNTIME_FUNCTION_SHORTCUTS`
Error: --> turbopack/crates/turbopack-ecmascript/src/references/mod.rs:160:9
|
160 | TUBROPACK_RUNTIME_FUNCTION_SHORTCUTS, TURBOPACK_EXPORT_NAMESPACE, TURBOPACK_EXPORT_VALUE,
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
| |
| no `TUBROPACK_RUNTIME_FUNCTION_SHORTCUTS` in `runtime_functions`
| help: a similar name exists in the module: `TURBOPACK_RUNTIME_FUNCTION_SHORTCUTS`
This is a typo from a merged pr, i will update 😂 |
e88474c
to
3679dff
Compare
3679dff
to
0c0747f
Compare
CodSpeed Performance ReportMerging #80542 will not alter performanceComparing Summary
Benchmarks breakdown
|
updated~ |
)?; | ||
} | ||
} | ||
_ => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: At this point, it's a bit better for this match to be exhaustive. If somebody adds a new enum variant, it'd be good for the compiler to warn them here and require they explicitly handle it.
_ => { | |
CachedExternalType::CommonJs | CachedExternalType::EcmaScriptViaRequire => { |
Update: I pushed a commit to your PR with this change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thx~
Failing test suitesCommit: 7be3ce8
Expand output● app-dir-hmr › filesystem changes › should update server components pages when env files is changed (node-module-var)
Read more about building and testing Next.js in contributing.md.
Expand output● app-dir action handling › "use server" export values › should error when exporting non async functions at build time
Read more about building and testing Next.js in contributing.md. |
This pr introduces a new external type for turbopack(maybe not for next.js), it can help to generate external like: https://webpack.js.org/configuration/externals/#externalstypeglobal.
global
external_module.rs