[experiment] implement tc39 Defer Module Evaluation #16567
Conversation
|
Yeah, we can implement this under the |
|
For maintainers only:
|
7420ab0 to
32408f7
Compare
This comment was marked as resolved.
This comment was marked as resolved.
|
Just update snaphots ( |
Is it locally? |
This comment was marked as resolved.
This comment was marked as resolved.
|
@Jack-Works Sounds like a bug in jest... do you use the same steps as in our CI file? Based on CI report you just need to update snapshot, no more |
|
Can you update |
This comment was marked as resolved.
This comment was marked as resolved.
|
@Jack-Works What is node version? You can try to change, also just check you have the same versions as in the lock file |
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
Weird, I don't have problem with jest locally... Okay, we can try to update jest (maybe new versions resolve this problem) Also can you use |
This comment was marked as resolved.
This comment was marked as resolved.
|
I tried to add some test cases (6041d87) but it seems like does not work... Did I do right? |
You can use virtual machine here 😄 |
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
9161b0f to
5250fd6
Compare
If the comment is put after |
|
Not sure why Github Actions / integration (ubuntu-latest, 10.x, b) (pull_request) and Github Actions / integration (windows-latest, 10.x, b) (pull_request) fails with |
|
Yeah feel free to inline if needed. But maybe @alexander-akait could you try re-running CI without cache first? |
de9c023 to
db2bd21
Compare
CodSpeed Performance ReportMerging #16567 will degrade performances by 69.75%Comparing Summary
Benchmarks breakdown
|
CodSpeed Performance ReportMerging #16567 will degrade performances by 23.26%Comparing Summary
Benchmarks breakdown
|
|
The same test becomes 23% slower in development and 23% faster in production? 🤔 |
|
hi @alexander-akait I think it is ready now! |
| @@ -46,6 +46,8 @@ class ImportParserPlugin { | |||
| Array.from(enumerable, e => [e]); | |||
| parser.hooks.importCall.tap("ImportParserPlugin", expr => { | |||
| const param = parser.evaluateExpression(expr.source); | |||
| // TODO support deferImport | |||
| // this requires the evaluateExpression to support Object literals. | |||
There was a problem hiding this comment.
@Jack-Works So we don't support import('something', { defer: true })? Do we have this in spec?
There was a problem hiding this comment.
@Jack-Works So we don't support
import('something', { defer: true })? Do we have this in spec?
No, the syntax becomes import.defer('something') (this comment is written in a very early stage), I'll update this soon.
| * @this {TODO} | ||
| * @param {TODO} node node | ||
| * @returns {TODO} import.meta | ||
| */ |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
There was a problem hiding this comment.
@Jack-Works Sorry for delay, there are two question, and we can merge (want to merge it this week), thank you for your work
Co-authored-by: Nicolo Ribaudo <hello@nicr.dev>
|
updated, but |
|
@Jack-Works Just for information - any blockers for |
Not yet, I didn't do any research on this because I'm focusing on other things right now. I emit this error to make the PR merge-ready (without a breaking |
|
Fyi, TypeScript just merged support for this :) |
| ])},`, | ||
| `defineProperty: ${runtimeTemplate.basicFunction("_, name", [ | ||
| init, | ||
| // TODO: |
There was a problem hiding this comment.
What we should do here in future?
There was a problem hiding this comment.
A Module Namespace object's defineProperty checks the new property descriptor. It only returns true when the new property descriptor is the same as the current one.
This behaviour is skipped since webpack's implementation (lib/runtime/MakeNamespaceObjectRuntimeModule.js) for normal namespace object is skipping this too.
| environment: { | ||
| optionalChaining: false | ||
| } | ||
| }, |
There was a problem hiding this comment.
Very weird, I will resolve it before merge, no need action from you
| ]), | ||
| "}", | ||
| // we don't fully emulate ES Module semantics in this Proxy to align with normal webpack esm namespace object. | ||
| "return new Proxy(ns, handler);" |
There was a problem hiding this comment.
Do we need to be compatible with es5 for Proxy?
There was a problem hiding this comment.
It is possible to support ES5 with known exported names as a getter, but I'd like to leave it to the future if anyone asks for it.
| `ownKeys: ${runtimeTemplate.basicFunction("", [ | ||
| init, | ||
| `var keys = Reflect.ownKeys(ns).filter(${runtimeTemplate.expressionFunction('x !== "then"', "x")}).concat([Symbol.toStringTag]);`, | ||
| "return Reflect.ownKeys(keys);" |
There was a problem hiding this comment.
Is there a typo here? maybe want to return keys?
There was a problem hiding this comment.
@Jack-Works Can you look at this and we can merge, thank you for your work and help
There was a problem hiding this comment.
Thanks! This is indeed an error, will fix soon.
| @@ -1162,6 +1162,12 @@ class JavascriptModulesPlugin { | |||
| buf.push(""); | |||
| } | |||
|
|
|||
| if (runtimeRequirements.has(RuntimeGlobals.makeDeferredNamespaceObject)) { | |||
| buf.push("// The deferred module cache"); | |||
| buf.push("var __webpack_module_deferred_exports__ = {};"); | |||
There was a problem hiding this comment.
I apologize, could you please clarify why this is necessary?
There was a problem hiding this comment.
When the deferred module is an ES Module, to get rid of all proxy handlers after the module initialization (search delete handler.get), the underlying object of the Proxy (ns in return new Proxy(ns, handler);) must be the export object of the module, but this object is not created until the module is evaluated (search exports: {} in JavascriptModulesPlugin).
This extra store __webpack_module_deferred_exports__ allows the exports object to be created and shared with all deferred importers (__webpack_module_deferred_exports__[moduleId] = { __proto__: null }).
This extra store is used when the module initializes:
// Create a new module (and put it into the cache)
var module = __webpack_module_cache__[moduleId] = {
id: moduleId,
// no module.loaded
exports: __webpack_module_deferred_exports__[moduleId] || {}
};There was a problem hiding this comment.
Thank you for your detailed clarification for all comment. ❤️
To be honest, if I understand correctly, I believe we don’t need it. We init module.exports first and then evaluate the module code. If I’ve misunderstood, could you provide an example of a scenario where removing it would cause issues?
Additionally, we already have some helper runtime functions to ensure CJS module compatibility(i.e. RuntimeGlobals.createFakeNamespaceObject for exportType: default and RuntimeGlobals.compatGetDefaultExport for exportType: dynamic), which should be used instead.
Would you mind if I co-authored this PR? It would be great to land it in v5.100.0.
There was a problem hiding this comment.
We init
module.exportsfirst and then evaluate the module code.
module.exports is initialized when the module first time imported, but this object (__webpack_module_deferred_exports__[moduleId]) is required internally (explained above, as the first argument of the Proxy and module.exports of that module in the future) when a deferred namespace object of this module is created, at that time, this module may not be imported yet.
If I’ve misunderstood, could you provide an example of a scenario where removing it would cause issues?
It's possible to implement this feature without __webpack_module_deferred_exports__, but that will have a runtime cost that every access to the deferred module namespace object will trigger the Proxy handler. I cannot give an example because it is a runtime optimization.
Would you mind if I co-authored this PR?
No problem, but I wonder what changes you're proposing.
There was a problem hiding this comment.
but that will have a runtime cost that every access to the deferred module namespace object will trigger the Proxy handler
I might get you now, so __webpack_module_deferred_exports__[moduleId] is used to preserve the original ns reference, enabling safe removal of the proxy handler in ESM namespace modules. Without it, reassigning to ns would not work.
but I wonder what changes you're proposing
I think we should use RuntimeGlobals.createFakeNamespaceObject and RuntimeGlobals.compatGetDefaultExport for CJS compatibility.
There was a problem hiding this comment.
@Jack-Works Does the following change make sense to you?
diff --git a/lib/RuntimePlugin.js b/lib/RuntimePlugin.js
index 5d4d94d47..09eaf1e49 100644
--- a/lib/RuntimePlugin.js
+++ b/lib/RuntimePlugin.js
@@ -102,6 +102,7 @@ const TREE_DEPENDENCIES = {
[RuntimeGlobals.makeDeferredNamespaceObject]: [
RuntimeGlobals.definePropertyGetters,
RuntimeGlobals.makeNamespaceObject,
+ RuntimeGlobals.compatGetDefaultExport,
RuntimeGlobals.createFakeNamespaceObject,
RuntimeGlobals.hasOwnProperty,
RuntimeGlobals.require
diff --git a/lib/runtime/MakeDeferredNamespaceObjectRuntime.js b/lib/runtime/MakeDeferredNamespaceObjectRuntime.js
index 0dba55e28..7c3b0e282 100644
--- a/lib/runtime/MakeDeferredNamespaceObjectRuntime.js
+++ b/lib/runtime/MakeDeferredNamespaceObjectRuntime.js
@@ -106,44 +106,47 @@ class MakeDeferredNamespaceObjectRuntimeModule extends HelperRuntimeModule {
"var cachedModule = __webpack_module_cache__[moduleId];",
...(strictError ? strictModuleCache : nonStrictModuleCache),
"",
- "if (mode == 1) {",
- Template.indent([
- // if this var is changed to let or const, please define a new ns in the outer scope.
- "var ns = { __proto__: null };",
- `${RuntimeGlobals.makeNamespaceObject}(ns);`,
- `${RuntimeGlobals.definePropertyGetters}(ns, {`,
- Template.indent([
- `"default": ${runtimeTemplate.returningFunction(
- `${RuntimeGlobals.require}(moduleId)`
- )}`
- ]),
- "});",
- "return ns;"
- ]),
- "}",
- "",
`var init = ${runtimeTemplate.basicFunction("", [
`ns = ${RuntimeGlobals.require}(moduleId);`,
hasAsync
? `if (${RuntimeGlobals.asyncModuleExportSymbol} in ns) ns = ns[${RuntimeGlobals.asyncModuleExportSymbol}];`
: "",
"init = null;",
- "if (mode == 3) {",
- Template.indent(["if (ns.__esModule) mode = 0;", "else mode = 2;"]),
- "}",
- "if (mode) return ns;",
+ "if (mode == 0) {",
// this optimization only applies to esm because it's .exports cannot be assigned to another object.
- "delete handler.defineProperty;",
- "delete handler.deleteProperty;",
- "delete handler.set;",
- "delete handler.get;",
- "delete handler.has;",
- "delete handler.ownKeys;",
- "delete handler.getOwnPropertyDescriptor;",
- "return ns;"
+ Template.indent([
+ "delete handler.defineProperty;",
+ "delete handler.deleteProperty;",
+ "delete handler.set;",
+ "delete handler.get;",
+ "delete handler.has;",
+ "delete handler.ownKeys;",
+ "delete handler.getOwnPropertyDescriptor;",
+ ]),
+ "} else if (mode == 1) {",
+ Template.indent([
+ `ns = ${RuntimeGlobals.createFakeNamespaceObject}(ns);`,
+ ]),
+ "} else if (mode == 2) {",
+ Template.indent([
+ `ns = ${RuntimeGlobals.createFakeNamespaceObject}(ns, 2);`,
+ ]),
+ "} else if (mode == 3) {",
+ Template.indent([
+ `${RuntimeGlobals.definePropertyGetters}(ns, {`,
+ Template.indent([
+ `"default": ${RuntimeGlobals.compatGetDefaultExport}(ns)`
+ ]),
+ "});",
+ ]),
+ "}"
])};`,
"",
- `ns = ${
+ `var ns = ${
strictError ? "" : "cachedModule && cachedModule.exports || "
}__webpack_module_deferred_exports__[moduleId] || (__webpack_module_deferred_exports__[moduleId] = { __proto__: null });`,
"var handler = {",
@@ -158,11 +161,7 @@ class MakeDeferredNamespaceObjectRuntimeModule extends HelperRuntimeModule {
]),
"}",
init,
- "// possible mode: 0, 2",
- "// for mode 2, ns.default is re-evaluated every time to reflect the latest module.exports value.",
- "// for the rest, we access the initial namespace object to keep esm semantics (export list is static).",
- `if (mode == 0 || name !== "default" || ${RuntimeGlobals.hasOwnProperty}(ns, name)) return ns[name];`,
- `return ${RuntimeGlobals.require}(moduleId);`
+ `return ns[name];`
])},`,
`has: ${runtimeTemplate.basicFunction("_, name", [
"switch (name) {",
There was a problem hiding this comment.
Yes I guess, but a little change:
-if (mode == 0) {
+if (mode == 0 || mode == 3 && ns.__esModule) {It passes all tests so I guess this change is correct. Do you want to push a commit or let me do that?
| const props = [ | ||
| `/* ${exportsType} */ get a() {`, | ||
| exportsType === "namespace" || exportsType === "dynamic" | ||
| ? Template.indent([ |
There was a problem hiding this comment.
Could you elaborate why we need handle namespace/dynamic here?
There was a problem hiding this comment.
This part is for runtime performance.
If exportsType is "namespace", we generate the following code:
const some_module_webpack_module_123 = {
get a() {
var exports = init code;
Object.defineProperty(this, "a", { value: exports });
return exports;
}
}On the second access to this object, the getter will not be triggered again, and access the real module export directly.
For "dynamic", it's only possible do this optimization when there is a __esModule on it, so we generate this:
const some_module_webpack_module_123 = {
get a() {
var exports = init code;
if (exports.__esModule) Object.defineProperty(this, "a", { value: exports });
return exports;
}
}For the rest cases, it's impossible to do any optimization, so it will generate
const some_module_webpack_module_123 = {
get a() {
return init code;
}
}
Does this PR introduce a breaking change?
no?
What needs to be documented once your changes are merged?
Defer import
This implements the semantics of the TC39 proposal Deferring Module Evaluation.
New options:
experiments.deferImport: trueTo use the native syntax, install
acorn-import-deferas a peer dependency.Result:
Other things to notice
Alternative syntax
can be written as
I suggest putting the magic comment after the
fromkeyword, which is compatible with TypeScript, Babel, SWC, and Flow if the necessary option to preserve comments is enabled.Please do not put it after the "import" keyword (like
import /* webpackDefer: true */ * as ns from './mod.mjs'), the filesystem cache is broken, and I don't know why.ESBuild is not compatible, please read evanw/esbuild#1439 and evanw/esbuild#309. Esbuild may support this feature ad-hoc like other webpack magic comments.