module: enable existing machinery for deferred imports#63712
Conversation
|
Review requested:
|
0968bc3 to
6bb2db9
Compare
joyeecheung
left a comment
There was a problem hiding this comment.
I think the commit message needs to clarify that this only makes it work for static import, but not dynamic import yet?
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #63712 +/- ##
========================================
Coverage 90.34% 90.35%
========================================
Files 732 732
Lines 236580 236709 +129
Branches 44545 44585 +40
========================================
+ Hits 213748 213870 +122
- Misses 14536 14565 +29
+ Partials 8296 8274 -22
🚀 New features to boost your workflow:
|
6bb2db9 to
5e52c9a
Compare
|
Thank you, I moved the test, but should I add more extensive test coverage, even for static imports? |
|
I think a smoke test is enough. The rest should be covered by test262 in the upstream. Can you rewrap the commit message to 72 chars to make the linter happy? Also this needs adding a eslint plugin
|
2915765 to
bd9968c
Compare
This commit enables deferred import for statically imported modules, by using the corresponding functionality in V8. It adds two tests: - smoke test exercising the basic syntax - test for deferred import of module with its own imports More test coverage should be added for mixing ESM and CJS modules, as well as TypeScript modules. Refs: https://github.com/tc39/proposal-defer-import-eval Signed-off-by: Maya Lekova <maya@igalia.com>
bd9968c to
569d71c
Compare
guybedford
left a comment
There was a problem hiding this comment.
Works perfectly thank you!
|
|
||
| // Attempts to define a property on the deferred module. This should | ||
| // trigger its execution, similar to accessing the `foo` property. | ||
| assert.throws(() => Object.defineProperty(deferred, 'newProp', { value: 15 }), TypeError); |
There was a problem hiding this comment.
| assert.throws(() => Object.defineProperty(deferred, 'newProp', { value: 15 }), TypeError); | |
| assert.throws(() => Object.getOwnPropertyDescriptor(deferred, 'newProp'), TypeError); |
Linter is complaining that it's read-only, I think using the getter does what we want too anyway.
|
|
||
| import '../common/index.mjs'; | ||
|
|
||
| import defer * as deferred from '../fixtures/es-modules/module-with-module-tree.mjs' |
There was a problem hiding this comment.
| import defer * as deferred from '../fixtures/es-modules/module-with-module-tree.mjs' | |
| import defer * as deferred from '../fixtures/es-modules/module-with-module-tree.mjs' | |
| console.log(defer.bar); |
This commit enables deferred module imports by using the corresponding functionality in V8. It also adds a single test, but more test coverage should be added.
Refs: https://github.com/tc39/proposal-defer-import-eval