-
Notifications
You must be signed in to change notification settings - Fork 31.1k
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
loader, docs, test: named exports from commonjs modules #16675
Changes from 6 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 |
---|---|---|
|
@@ -83,12 +83,19 @@ All CommonJS, JSON, and C++ modules can be used with `import`. | |
Modules loaded this way will only be loaded once, even if their query | ||
or fragment string differs between `import` statements. | ||
|
||
When loaded via `import` these modules will provide a single `default` export | ||
representing the value of `module.exports` at the time they finished evaluating. | ||
CommonJS modules, when imported, will be handled in one of two ways. By default | ||
they will provide a single `default` export representing the value of | ||
`module.exports` at the time they finish evaluating. However, they may also | ||
provide `@@esModuleInterop` or `__esModule` to use named exports, representing | ||
each enumerable key of `module.exports` at the time they finish evaluating. | ||
In both cases, this should be thought of like a "snapshot" of the exports at | ||
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 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 commentThe reason will be displayed to describe this comment to others. Learn more. I thought just saying 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. 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 commentThe 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 commentThe 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) |
||
the time of importing; asynchronously modifying `module.exports` will not | ||
affect the values of the exports. Builtin libraries are provided with named | ||
exports as if they were using `@@esModuleInterop`. | ||
|
||
```js | ||
import fs from 'fs'; | ||
fs.readFile('./foo.txt', (err, body) => { | ||
import { readFile } from 'fs'; | ||
readFile('./foo.txt', (err, body) => { | ||
if (err) { | ||
console.error(err); | ||
} else { | ||
|
@@ -97,6 +104,15 @@ fs.readFile('./foo.txt', (err, body) => { | |
}); | ||
``` | ||
|
||
```js | ||
// main.mjs | ||
import { part } from './other.js'; | ||
|
||
// other.js | ||
exports.part = () => {}; | ||
exports[Symbol.for('esModuleInterop')] = true; | ||
``` | ||
|
||
## Loader hooks | ||
|
||
<!-- type=misc --> | ||
|
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 esModuleInterop = Symbol.for('esModuleInterop'); | ||||
|
||||
const realpathCache = new Map(); | ||||
|
||||
const loaders = new Map(); | ||||
|
@@ -36,21 +38,32 @@ 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[esModuleInterop] || 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 wonder if it might be useful to allow |
||||
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 (let i = 0; i < keys.length; i++) | ||||
reflect.exports[keys[i]].set(exports[keys[i]]); | ||||
} 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 (let i = 0; i < keys.length; i++) | ||||
reflect.exports[keys[i]].set(exports[keys[i]]); | ||||
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. One of the worries with doing this for native modules was that any updates to native module exports won't be seen. Specifically I think this was an issue when considering APM use cases that monkey patch any native internals. That said, if such issues do come up, setter detection on native modules could possibly be configured to update named exports. |
||||
}); | ||||
}); | ||||
|
||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,9 @@ | ||
// Flags: --experimental-modules | ||
/* eslint-disable required-modules */ | ||
|
||
import assert from 'assert'; | ||
import eightyfour, { named as fourtytwo } from | ||
'../fixtures/es-module-loaders/babel-to-esm.js'; | ||
|
||
assert.strictEqual(eightyfour, 84); | ||
assert.strictEqual(fourtytwo, 42); |
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,15 @@ | ||
"use strict"; | ||
|
||
/* | ||
created by babel with es2015 preset | ||
``` | ||
export const named = 42; | ||
export default 84; | ||
``` | ||
*/ | ||
|
||
Object.defineProperty(exports, "__esModule", { | ||
value: true | ||
}); | ||
var named = exports.named = 42; | ||
exports.default = 84; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,4 @@ | ||
exports.named = true; | ||
exports.default = 1; | ||
|
||
exports[Symbol.for('esModuleInterop')] = true; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,6 @@ | ||
module.exports = { | ||
enum: 'enum', | ||
class: 'class', | ||
delete: 'delete', | ||
[Symbol.for('esModuleInterop')]: true, | ||
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. Babel folks: Any opinions on the symbol name 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's no need to force camelCase. 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. most symbols i see throughout the js world use camelCase or PascalCase, and 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. Variable names do; Symbols haven't been around long enough to have an established convention around that. The string in 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 did not see the camel case debate coming... was thinking more along the lines of whether it should be Node is currently a bit all over the place with its symbol labels. Examples:
I'm curious if Babel is transitioning to a symbol key as well and if they already have a name. 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. Definitely appreciate the ping. I personally don't see that it'd be feasible for us to use a Symbol because the code could be running on a platform that doesn't support Symbols, assuming I'm following all this properly, but it probably does make sense to use a Symbol in the long run for loaders, assuming Symbol support is known to exist. |
||
}; |
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.
Perhaps clarify exactly what these properties are - "provide a
@esModuleInterop
or__esModule
boolean export indicating that a snapshot should be taken of each enumerable key..."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.
sounds good, and is it
@@
or@
to doc a symbol, i thought it was twoThere 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.
Yeah it's still two, that was a typo.