-
-
Notifications
You must be signed in to change notification settings - Fork 31.7k
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 3 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 | ||
---|---|---|---|---|
|
@@ -35,22 +35,34 @@ loaders.set('esm', async (url) => { | |||
}); | ||||
|
||||
// Strategy for loading a node-style CommonJS module | ||||
// Uses babel and typescript style __esModule property for | ||||
// attaching named imports due to the possiblity of `module.exports.default` | ||||
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); | ||||
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 es = !!exports.__esModule; | ||||
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. I think we should promote the use of a 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. thats a fantastic idea, would attaching it to the 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 something in the name to indicate it is a symbol for CommonJS interop cases? 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. I'm fine with making it as long as we want. I would use |
||||
const keys = es ? Object.keys(exports) : ['default']; | ||||
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. Would 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. i chose to use Object.keys so that it only exports enumerable properties. (it says so in the esm doc) |
||||
return createDynamicModule(keys, url, (reflect) => { | ||||
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. Looks like 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 maps names to safeguard against this already: node/lib/internal/loader/ModuleWrap.js Line 18 in f5a7a9e
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. Ah so it does, my mistake. Missed the |
||||
if (es) { | ||||
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. Is branching like this faster? Otherwise seems like the else branch does exactly what the 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. i don't understand what you mean, those two blocks do different things 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. Sign, nevermind, just another case of misreading. |
||||
for (const key of keys) | ||||
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. There might be some edge cases where 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. I think we should use a |
||||
reflect.exports[key].set(exports[key]); | ||||
} else { | ||||
reflect.exports.default.set(exports); | ||||
} | ||||
}); | ||||
}); | ||||
|
||||
// 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) => { | ||||
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. 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 - 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]); | ||||
}); | ||||
}); | ||||
|
||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,7 +1,14 @@ | ||
// Flags: --experimental-modules | ||
/* eslint-disable required-modules */ | ||
|
||
import * as fs from 'fs'; | ||
import assert from 'assert'; | ||
import fs, { readFile } from 'fs'; | ||
import main, { named } from | ||
'../fixtures/es-module-loaders/cjs-to-es-namespace.js'; | ||
|
||
assert.deepStrictEqual(Object.keys(fs), ['default']); | ||
assert(fs); | ||
assert(fs.readFile); | ||
assert.strictEqual(fs.readFile, readFile); | ||
|
||
assert.strictEqual(main, 1); | ||
assert.strictEqual(named, true); |
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 { enum as e } from | ||
'../fixtures/es-module-loaders/reserved-keywords.js'; | ||
|
||
assert(e); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,4 @@ | ||
exports.named = true; | ||
exports.default= 1; | ||
|
||
Object.defineProperty(exports, '__esModule', { value: true }); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,6 @@ | ||
module.exports = { | ||
enum: 'enum', | ||
class: 'class', | ||
delete: 'delete', | ||
__esModule: true, | ||
}; |
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.
Is this a snapshot of the values, or just a snapshot of the names?
The latter is necessary, but the former may not be.
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 thought just saying
exports
was ok since its both the names and the valuesThere 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.
Right - I’m saying that there’s no need for the values to be snapshotted; just the names.
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.
technically it isn't a snapshot, it's just a useful term to describe how it becomes static when assigned in reflection with es imports.
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’s pretty important to be precise here :-) i think “a snapshot of the names of the exports”, and indicating that if the values are updated, the resulting imports will update as well (a requirement for APM-like use cases, i understand)