esm: refactor dynamic modules #9

devsnek · 2018-10-08T18:35:16Z

Use one module wrap instead of two for the dynamic modules implementation.

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
commit message follows commit guidelines

jdalton · 2018-10-08T19:06:20Z

What's the gist of the refactor? A sentence or two as a description would do.

bmeck · 2018-10-08T19:58:29Z

lib/internal/modules/esm/create_dynamic_module.js

 const debug = require('util').debuglog('esm');
 const ArrayJoin = Function.call.bind(Array.prototype.join);
 const ArrayMap = Function.call.bind(Array.prototype.map);

+const defaultName = '\u5343\u4e07\u4e0d\u8981\u7528\u8fd9\u4e2a\u53d8\u91cf' +


what is up with the default name here?

A local name has to be bound to the default export, and "do not use this variable or you will be fired" in chinese was suggested to me by @TimothyGu.

@devsnek

A local name has to be bound to the default export, and "do not use this variable or you will be fired" in chinese was suggested to me by @TimothyGu.

Would you please add code comments about it and why it's needed. This seems kind of hacky. Are there any other possible ways to tackle this (or APIs we could introduce to better address this)?

I'll add a comment. the API we can introduce is what @guybedford is already working on.

It seems like we could just prefix all names with $ like the old code instead, which I would prefer rather than treating some names differently from others.

bmeck · 2018-10-08T20:00:03Z

@jdalton the original esm loader was using 2 module records for any dynamic module due to a lack of import.meta being available when it was originally written. This moves the dynamic module facade to a single module record by exposing the reflection API off import.meta instead of a completion with a 2nd module record.

bmeck

Don't use special variable names.

bmeck · 2018-10-10T13:53:26Z

lib/internal/modules/esm/create_dynamic_module.js

 const debug = require('util').debuglog('esm');
 const ArrayJoin = Function.call.bind(Array.prototype.join);
 const ArrayMap = Function.call.bind(Array.prototype.map);

+const defaultName = '\u5343\u4e07\u4e0d\u8981\u7528\u8fd9\u4e2a\u53d8\u91cf' +


It seems like we could just prefix all names with $ like the old code instead, which I would prefer rather than treating some names differently from others.

lib/internal/modules/esm/create_dynamic_module.js

MylesBorins

LGTM

I would also like to see this upstreamed

guybedford · 2018-11-08T16:22:16Z

@devsnek were you planning to merge this after sorting out the rebase approach then? There don't seem to be any conflicts though so could be worth merging already too.

devsnek · 2018-11-08T17:17:07Z

@guybedford we shouldn't land anything when our HEAD is this far behind nodejs/node~HEAD. once we're more up to date it should be safe to start landing things again.

MylesBorins · 2018-11-08T18:12:57Z

@devsnek didn't we have consensus to do a manual rebase?

edit: also since we had consensus to upstream, would it make more sense to simply open this PR on the main repo?

devsnek · 2018-11-08T18:19:38Z

@MylesBorins oh i wasn't aware we had agreed to do that. i guess i can try to do the rebase this weekend then unless someone else is feeling more masochistic...

MylesBorins · 2018-11-08T20:26:55Z

rebase opened in #13

I think this could potentially just be moved to nodejs/node and picked up in a future rebase... thoughts?

devsnek · 2018-11-08T20:34:31Z

I don't quite understand why we're maintaining the upstream if we're just going to replace it

guybedford · 2018-11-08T20:35:15Z

It may indeed make merging easier if we don't upstream.

MylesBorins · 2018-11-08T20:35:43Z

@devsnek because people are still using it. Unless we plan to completely remove it anything we can do to keep the delta smaller will make a future merge easier imho

MylesBorins · 2018-11-09T04:21:04Z

@ljharb I was suggesting that this specific PR could land directly on master.

I've retargeting this against modules-lkgr and rebased. Will land if CI is green

CI: https://ci.nodejs.org/job/node-test-pull-request/18433/

MylesBorins · 2018-11-10T02:47:34Z

Failure on arm

06:05:25 not ok 311 known_issues/test-vm-timeout-escape-nexttick
06:05:25   ---
06:05:25   duration_ms: 1.235
06:05:25   severity: fail
06:05:25   stack: |-

Rerunning https://ci.nodejs.org/job/node-test-commit-arm-fanned/4210/

MylesBorins · 2018-11-21T05:49:26Z

rebased against modules-lkgr

ci: https://ci.nodejs.org/job/node-test-pull-request/18832/

MylesBorins · 2018-11-21T07:38:09Z

CI is green 🎉

landed in cf533af

@devsnek do you object to me opening this upstream? I think it is worth minimizing the delta if it doesn't change any behavior

This is a change from the ecmascript-modules fork. There is no change to behavior and we would like to upstream to reduce the delta between our repos. Refs: https://github.com/nodejs/ecmascript-modules#9 PR-URL: nodejs/node#24560 Refs: #9 Reviewed-By: Guy Bedford <[email protected]> Reviewed-By: Gus Caplan <[email protected]>

MylesBorins · 2018-11-30T21:42:24Z

This landed upstream in 2931c50

This is a change from the ecmascript-modules fork. There is no change to behavior and we would like to upstream to reduce the delta between our repos. Refs: https://github.com/nodejs/ecmascript-modules#9 PR-URL: #24560 Refs: nodejs/ecmascript-modules#9 Reviewed-By: Guy Bedford <[email protected]> Reviewed-By: Gus Caplan <[email protected]>

This is a change from the ecmascript-modules fork. There is no change to behavior and we would like to upstream to reduce the delta between our repos. Refs: https://github.com/nodejs/ecmascript-modules#9 PR-URL: nodejs#24560 Refs: nodejs/ecmascript-modules#9 Reviewed-By: Guy Bedford <[email protected]> Reviewed-By: Gus Caplan <[email protected]>

This is a change from the ecmascript-modules fork. There is no change to behavior and we would like to upstream to reduce the delta between our repos. Refs: https://github.com/nodejs/ecmascript-modules#9 PR-URL: #24560 Refs: nodejs/ecmascript-modules#9 Reviewed-By: Guy Bedford <[email protected]> Reviewed-By: Gus Caplan <[email protected]>

devsnek requested review from bmeck and guybedford October 8, 2018 18:35

bmeck reviewed Oct 8, 2018

View reviewed changes

devsnek force-pushed the master branch from bdac05e to 47aac7c Compare October 9, 2018 00:18

devsnek force-pushed the refactor/dynamic-modules branch from 541da29 to 285493f Compare October 9, 2018 00:21

bmeck suggested changes Oct 10, 2018

View reviewed changes

devsnek force-pushed the refactor/dynamic-modules branch 2 times, most recently from c84a41b to 7bd1e1c Compare October 10, 2018 15:06

bmeck approved these changes Oct 10, 2018

View reviewed changes

guybedford approved these changes Oct 15, 2018

View reviewed changes

MylesBorins added the modules-agenda label Oct 24, 2018

MylesBorins mentioned this pull request Nov 6, 2018

Node.js Foundation Modules Team Meeting 2018-11-07 nodejs/modules#217

Closed

devsnek added the blocked label Nov 7, 2018

jdalton approved these changes Nov 7, 2018

View reviewed changes

MylesBorins approved these changes Nov 7, 2018

View reviewed changes

devsnek removed the blocked label Nov 7, 2018

devsnek removed the modules-agenda label Nov 8, 2018

MylesBorins force-pushed the master branch from 47aac7c to 1f6c4ba Compare November 8, 2018 20:37

This comment has been minimized.

Sign in to view

MylesBorins changed the base branch from master to modules-lkgr November 9, 2018 04:18

MylesBorins force-pushed the refactor/dynamic-modules branch from 7bd1e1c to 60ec064 Compare November 9, 2018 04:19

MylesBorins force-pushed the modules-lkgr branch from 2fa778b to 05ba15a Compare November 21, 2018 05:46

esm: refactor dynamic modules

cf533af

MylesBorins force-pushed the refactor/dynamic-modules branch from 60ec064 to cf533af Compare November 21, 2018 05:48

MylesBorins merged commit cf533af into modules-lkgr Nov 21, 2018

MylesBorins mentioned this pull request Nov 21, 2018

esm: refactor dynamic modules nodejs/node#24560

Closed

devsnek deleted the refactor/dynamic-modules branch November 30, 2018 21:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

esm: refactor dynamic modules #9

esm: refactor dynamic modules #9

devsnek commented Oct 8, 2018 •

edited

Loading

jdalton commented Oct 8, 2018

bmeck Oct 8, 2018

devsnek Oct 8, 2018

jdalton Oct 8, 2018

devsnek Oct 8, 2018

bmeck Oct 10, 2018

bmeck commented Oct 8, 2018

bmeck left a comment

bmeck Oct 10, 2018

MylesBorins left a comment

guybedford commented Nov 8, 2018

devsnek commented Nov 8, 2018 •

edited

Loading

MylesBorins commented Nov 8, 2018 •

edited

Loading

devsnek commented Nov 8, 2018

MylesBorins commented Nov 8, 2018

devsnek commented Nov 8, 2018

guybedford commented Nov 8, 2018

MylesBorins commented Nov 8, 2018

This comment has been minimized.

MylesBorins commented Nov 9, 2018 •

edited

Loading

MylesBorins commented Nov 10, 2018

MylesBorins commented Nov 21, 2018

MylesBorins commented Nov 21, 2018

MylesBorins commented Nov 30, 2018

esm: refactor dynamic modules #9

esm: refactor dynamic modules #9

Conversation

devsnek commented Oct 8, 2018 • edited Loading

Checklist

jdalton commented Oct 8, 2018

bmeck Oct 8, 2018

Choose a reason for hiding this comment

devsnek Oct 8, 2018

Choose a reason for hiding this comment

jdalton Oct 8, 2018

Choose a reason for hiding this comment

devsnek Oct 8, 2018

Choose a reason for hiding this comment

bmeck Oct 10, 2018

Choose a reason for hiding this comment

bmeck commented Oct 8, 2018

bmeck left a comment

Choose a reason for hiding this comment

bmeck Oct 10, 2018

Choose a reason for hiding this comment

MylesBorins left a comment

Choose a reason for hiding this comment

guybedford commented Nov 8, 2018

devsnek commented Nov 8, 2018 • edited Loading

MylesBorins commented Nov 8, 2018 • edited Loading

devsnek commented Nov 8, 2018

MylesBorins commented Nov 8, 2018

devsnek commented Nov 8, 2018

guybedford commented Nov 8, 2018

MylesBorins commented Nov 8, 2018

This comment has been minimized.

MylesBorins commented Nov 9, 2018 • edited Loading

MylesBorins commented Nov 10, 2018

MylesBorins commented Nov 21, 2018

MylesBorins commented Nov 21, 2018

MylesBorins commented Nov 30, 2018

devsnek commented Oct 8, 2018 •

edited

Loading

devsnek commented Nov 8, 2018 •

edited

Loading

MylesBorins commented Nov 8, 2018 •

edited

Loading

MylesBorins commented Nov 9, 2018 •

edited

Loading