-
-
Notifications
You must be signed in to change notification settings - Fork 34k
loader, docs, test: named exports from commonjs modules #16675
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
Changes from 2 commits
81fef20
d3647dd
e8478e2
116b470
1a53fd1
ac2b4a1
4a1c95a
49add2b
28f58f6
c6da711
1220d06
fe73e26
c4a6c27
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -36,31 +36,38 @@ loaders.set('esm', async (url) => { | |
|
|
||
| // Strategy for loading a node-style CommonJS module | ||
| loaders.set('cjs', async (url) => { | ||
| return createDynamicModule(['default'], url, (reflect) => { | ||
| debug(`Loading CJSModule ${url}`); | ||
| const CJSModule = require('module'); | ||
| const pathname = internalURLModule.getPathFromURL(new URL(url)); | ||
| CJSModule._load(pathname); | ||
| debug(`Loading CJSModule ${url}`); | ||
| const CJSModule = require('module'); | ||
| const pathname = internalURLModule.getPathFromURL(new URL(url)); | ||
| const exports = CJSModule._load(pathname); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This changes CJS to always evaluate prior to linking ESM which reorders imports in odd ways |
||
| const keys = Object.keys(exports); | ||
| return createDynamicModule(['default', ...keys], url, (reflect) => { | ||
| reflect.exports.default.set(exports); | ||
| for (const key of keys) reflect.exports[key].set(exports[key]); | ||
| }); | ||
| }); | ||
|
|
||
| // Strategy for loading a node builtin CommonJS module that isn't | ||
| // through normal resolution | ||
| loaders.set('builtin', async (url) => { | ||
| return createDynamicModule(['default'], url, (reflect) => { | ||
| debug(`Loading BuiltinModule ${url}`); | ||
| const exports = NativeModule.require(url.substr(5)); | ||
| debug(`Loading BuiltinModule ${url}`); | ||
| const exports = NativeModule.require(url.substr(5)); | ||
| const keys = Object.keys(exports); | ||
| return createDynamicModule(['default', ...keys], url, (reflect) => { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's not documented that builtin modules still have a default export.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Perhaps for these native module keys we should add a filter -
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please do not; |
||
| reflect.exports.default.set(exports); | ||
| for (const key of keys) reflect.exports[key].set(exports[key]); | ||
| }); | ||
| }); | ||
|
|
||
| loaders.set('addon', async (url) => { | ||
| const ctx = createDynamicModule(['default'], url, (reflect) => { | ||
| debug(`Loading NativeModule ${url}`); | ||
| const module = { exports: {} }; | ||
| const pathname = internalURLModule.getPathFromURL(new URL(url)); | ||
| process.dlopen(module, _makeLong(pathname)); | ||
| debug(`Loading NativeModule ${url}`); | ||
| const module = { exports: {} }; | ||
| const pathname = internalURLModule.getPathFromURL(new URL(url)); | ||
| process.dlopen(module, _makeLong(pathname)); | ||
| const keys = Object.keys(module.exports); | ||
| const ctx = createDynamicModule(['default', ...keys], url, (reflect) => { | ||
| reflect.exports.default.set(module.exports); | ||
| for (const key of keys) reflect.exports[key].set(module.exports[key]); | ||
| }); | ||
| return ctx; | ||
| }); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,7 +1,9 @@ | ||
| // Flags: --experimental-modules | ||
| /* eslint-disable required-modules */ | ||
|
|
||
| import * as fs from 'fs'; | ||
| import assert from 'assert'; | ||
| import fs, { readFile } from 'fs'; | ||
|
|
||
| assert.deepStrictEqual(Object.keys(fs), ['default']); | ||
| assert(fs); | ||
| assert(fs.readFile); | ||
| assert(readFile); | ||
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,8 @@ | ||
| // Flags: --experimental-modules | ||
| /* eslint-disable required-modules */ | ||
|
|
||
| import assert from 'assert'; | ||
| import { delete as d } from | ||
| '../fixtures/es-module-loaders/reserved-keywords.js'; | ||
|
|
||
| assert(d); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| module.exports = { | ||
| enum: 'enum', | ||
| class: 'class', | ||
| delete: 'delete', | ||
| }; |
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.
I would add a sentence explaining what will not be supported, i.e. modifying
module.exportswith an asynchronous operations will not update that property in ESM. It seems an edge case, but I do not see a way to provide a good error message for it we should be careful in how we document.