Skip to content

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.

Sign up for GitHub

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

Closed
devsnek wants to merge 13 commits into from
Closed

loader, docs, test: named exports from commonjs modules #16675

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
81fef20
loader,docs,test: set named exports based on keys from `module.exports`
devsnek Nov 1, 2017
d3647dd
add test for exporting reserved keywords from cjs from esm
devsnek Nov 2, 2017
e8478e2
Slight change of plan with loading (use well-defined __esModule)
devsnek Nov 2, 2017
116b470
Symbol('esModule') exported from `module` builtin
devsnek Nov 2, 2017
1a53fd1
remove comment i forgot to remove
devsnek Nov 2, 2017
ac2b4a1
change up symbol usage, use for(;;), add tests
devsnek Nov 2, 2017
4a1c95a
symbol overrides __esModule
devsnek Nov 2, 2017
49add2b
add back docs for json/c++ addons
devsnek Nov 3, 2017
28f58f6
apparently this is much faster
devsnek Nov 3, 2017
c6da711
this is why you don't commit from github web
devsnek Nov 3, 2017
1220d06
make tests a bit more verbose
devsnek Nov 4, 2017
fe73e26
fix lint
devsnek Nov 4, 2017
c4a6c27
document builtin default export
devsnek Nov 4, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
fix lint
  • Loading branch information
@devsnek
devsnek committed Nov 4, 2017
commit fe73e26878c94bfd8acfd94c9915bd79fa3b6dcc
4 changes: 2 additions & 2 deletions lib/internal/loader/ModuleRequest.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@ loaders.set('cjs', async (url) => {
const keys = es ? Object.keys(exports) : ['default'];
Copy link
Copy Markdown
Member

@addaleax addaleax Nov 2, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would Object.getOwnPropertyNames(exports) make more sense?

Copy link
Copy Markdown
Member Author

@devsnek devsnek Nov 2, 2017

Choose a reason for hiding this comment

The 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) => {
Copy link
Copy Markdown
Contributor

@loganfsmyth loganfsmyth Nov 7, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like createDynamicModule would throw if one of the exports was named executor because there would be duplicated exports? Seems like ideally it would generate an executor name that wouldn't conflict, or at least make it less likely to conflict.

Copy link
Copy Markdown
Member

@bmeck bmeck Nov 7, 2017

Choose a reason for hiding this comment

The 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

${ArrayJoin(ArrayMap(names, (name) => `export let $${name};`), '\n')}

Copy link
Copy Markdown
Contributor

@loganfsmyth loganfsmyth Nov 7, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah so it does, my mistake. Missed the $ on there.

if (es) {
Copy link
Copy Markdown
Contributor

@loganfsmyth loganfsmyth Nov 7, 2017

Choose a reason for hiding this comment

The 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 if branch does, just more generally.

Copy link
Copy Markdown
Member Author

@devsnek devsnek Nov 7, 2017 •
edited
Loading

Choose a reason for hiding this comment

The 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

Copy link
Copy Markdown
Contributor

@loganfsmyth loganfsmyth Nov 7, 2017

Choose a reason for hiding this comment

The 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++)
for (var i = 0; i < keys.length; i++)
reflect.exports[keys[i]].set(exports[keys[i]]);
} else {
reflect.exports.default.set(exports);
Expand All @@ -63,7 +63,7 @@ loaders.set('builtin', async (url) => {
const keys = Object.keys(exports);
return createDynamicModule(['default', ...keys], url, (reflect) => {
Copy link
Copy Markdown
Member

@targos targos Nov 4, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not documented that builtin modules still have a default export.

Copy link
Copy Markdown
Contributor

@guybedford guybedford Nov 6, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps for these native module keys we should add a filter - Object.keys(exports).filter(name => name.startsWith('_') === false)? This would avoid private keys such as import { _makeLong } from 'path' being on the public API that will end up in the list for type hinting systems such as TypeScript.

Copy link
Copy Markdown
Member

@ljharb ljharb Nov 6, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please do not; _ may be used to convey hopeful privacy, but it’s always fully public.

reflect.exports.default.set(exports);
for (let i = 0; i < keys.length; i++)
for (var i = 0; i < keys.length; i++)
reflect.exports[keys[i]].set(exports[keys[i]]);
Copy link
Copy Markdown
Contributor

@guybedford guybedford Nov 2, 2017

Choose a reason for hiding this comment

The 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.

});
});
Expand Down
1 change: 1 addition & 0 deletions test/es-module/test-esm-esmoduleinterop-override.mjs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
// Flags: --experimental-modules
/* eslint-disable required-modules */

// eslint-disable-next-line no-unused-vars
import eightyfour, { fourtytwo }
from '../fixtures/es-module-loaders/babel-to-esm-override.js';