-
-
Notifications
You must be signed in to change notification settings - Fork 32.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 5 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 | ||
---|---|---|---|---|
|
@@ -19,6 +19,8 @@ const search = require('internal/loader/search'); | |||
const asyncReadFile = require('util').promisify(require('fs').readFile); | ||||
const debug = require('util').debuglog('esm'); | ||||
|
||||
const esModuleSymbol = exports.esModuleSymbol = Symbol('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 would prefer 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. that was my first approach but it failed the tests. i speculate that symbols aren't shared between v8 contexts or something, if you have a solution please lemme know 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. > vm.runInNewContext('Symbol.for("foo")') === Symbol.for('foo')
true I think this should work? |
||||
|
||||
const realpathCache = new Map(); | ||||
|
||||
const loaders = new Map(); | ||||
|
@@ -36,21 +38,31 @@ 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); | ||||
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 = Boolean(exports[esModuleSymbol] || exports.__esModule); | ||||
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,6 @@ | ||
const { esModule } = require('module'); | ||
|
||
exports.named = true; | ||
exports.default = 1; | ||
|
||
exports[esModule] = true; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,8 @@ | ||
const { esModule } = require('module'); | ||
|
||
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.
What’s the motivation for keeping
__esModule
when a symbol is available? You can alwaysimport { esModule } from 'module';
, right?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.
Or, just a thought: What if this was an alternative to
module.exports
, e.g.module.esmExports
? We could check for the presence of that property when the script finished.The upsides of this would be that you don’t need a symbol, and you don’t need to modify the actual exports object