esm: port loader code to JS #32201

addaleax · 2020-03-11T15:37:43Z

There is no reason for this to be in C++. Using JavaScript means that
the code is more accessible to more developers, which is important
for any Node.js feature. This also simplifies the code significantly
in some areas. On the technical side, this potentially also enables
making some of the file system operations that are involved
asynchronous.

@nodejs/modules-active-members

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
commit message follows commit guidelines

There is no reason for this to be in C++. Using JavaScript means that the code is more accessible to more developers, which is important for any Node.js feature. This also simplifies the code significantly in some areas. On the technical side, this potentially also enables making some of the file system operations that are involved asynchronous.

MylesBorins · 2020-03-11T15:46:34Z

@addaleax thanks much for this awesome work.

It seems like the work might overlap a bit with some error improvements I was trying to do in #32052 (which ended up being complicated due to the split logic between js + c++)

Would it make sense to fold the logic from my PR into here?

bmeck · 2020-03-11T15:52:58Z

Note: last time we benchmarked this, sync fs ops were significantly faster during resolution. We should benchmark before jumping onto async.

jkrems

Thanks so much for taking this on! I did an initial pass and it LGTM overall.

test/es-module/test-esm-exports.mjs

bmeck · 2020-03-11T15:56:25Z

@addaleax This should use primordials and cached builtins. If desired, I can make the changes and push them onto this PR.

ljharb

Sorry only some of these are suggestions; it’s really hard to type them out on mobile :-)

lib/internal/modules/esm/resolve.js

addaleax · 2020-03-11T16:12:09Z

@MylesBorins If you want, I can try to integrate your changes – at the moment, I was trying to go for a 1:1 conversion from ESM, but if there’s consensus for further changes, I’m okay with that.

addaleax · 2020-03-11T16:26:08Z

@bmeck I’ve added primordials usage where I could find it – let me know if there’s anything else. (You can feel free to push changes if you think that they make sense, too.)

lib/internal/modules/esm/resolve.js

bmeck

some nits, will need to re-review due to size

lib/internal/modules/esm/resolve.js

MylesBorins · 2020-03-11T17:05:30Z

@addaleax there was consensus on the behavior in the PR I mentioned, just needed work to ensure it covered both CJS + ESM loader. LMK if you prefer I try and add it to this PR and push a commit, since it seems like you have your work cut out for you here

lib/internal/modules/esm/get_format.js

lib/internal/modules/esm/resolve.js

addaleax · 2020-03-12T11:30:32Z

LMK if you prefer I try and add it to this PR and push a commit, since it seems like you have your work cut out for you here

Yeah, that would be great.

To be honest, I’m not clear on the exact semantics here everywhere, although it’s easy enough to translate the code into JS.

Co-Authored-By: Jordan Harband <ljharb@gmail.com>

lib/internal/modules/esm/resolve.js

Co-Authored-By: Bradley Farias <bradley.meck@gmail.com>

nodejs-github-bot · 2020-03-12T18:47:27Z

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

nodejs-github-bot · 2020-03-12T18:57:26Z

CI: https://ci.nodejs.org/job/node-test-pull-request/29786/ (:white_check_mark:)

guybedford · 2020-03-12T21:48:31Z

lib/internal/modules/esm/resolve.js

+  }
+}
+
+function getPackageConfig(path, base) {


It should be possible to update this to be shared with https://github.com/nodejs/node/blob/master/lib/internal/modules/cjs/loader.js#L245 now. That can be a further refactoring certainly, but it's ripe to share the package.json caches and C++ JSON parser code optimized for package.json reading.

I think I'd like to move that refactoring to a later point. In a similar vein: We could also share the exports logic now!

addaleax · 2020-03-13T16:35:09Z

Landed in 605615e

There is no reason for this to be in C++. Using JavaScript means that the code is more accessible to more developers, which is important for any Node.js feature. This also simplifies the code significantly in some areas. On the technical side, this potentially also enables making some of the file system operations that are involved asynchronous. PR-URL: #32201 Reviewed-By: Bradley Farias <bradley.meck@gmail.com> Reviewed-By: Guy Bedford <guybedford@gmail.com>

MylesBorins · 2020-04-02T04:42:53Z

hey @addaleax do you think we should backport this to 12.x? Seems to me like if we don't future module updates will be hard to land.

There is no reason for this to be in C++. Using JavaScript means that the code is more accessible to more developers, which is important for any Node.js feature. This also simplifies the code significantly in some areas. On the technical side, this potentially also enables making some of the file system operations that are involved asynchronous. PR-URL: nodejs#32201 Reviewed-By: Bradley Farias <bradley.meck@gmail.com> Reviewed-By: Guy Bedford <guybedford@gmail.com>

There is no reason for this to be in C++. Using JavaScript means that the code is more accessible to more developers, which is important for any Node.js feature. This also simplifies the code significantly in some areas. On the technical side, this potentially also enables making some of the file system operations that are involved asynchronous. PR-URL: #32201 Reviewed-By: Bradley Farias <bradley.meck@gmail.com> Reviewed-By: Guy Bedford <guybedford@gmail.com>

addaleax added the esm Issues and PRs related to the ECMAScript Modules implementation. label Mar 11, 2020

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. labels Mar 11, 2020

jkrems reviewed Mar 11, 2020

View reviewed changes

test/es-module/test-esm-exports.mjs Show resolved Hide resolved

ljharb reviewed Mar 11, 2020

View reviewed changes

fixup! esm: port loader code to JS

1f20296

bmeck reviewed Mar 11, 2020

View reviewed changes

lib/internal/modules/esm/resolve.js Show resolved Hide resolved

GeoffreyBooth reviewed Mar 11, 2020

View reviewed changes

lib/internal/modules/esm/resolve.js Outdated Show resolved Hide resolved

bmeck suggested changes Mar 11, 2020

View reviewed changes

fixup! esm: port loader code to JS

49c4f53

ljharb reviewed Mar 11, 2020

View reviewed changes

lib/internal/modules/esm/get_format.js Outdated Show resolved Hide resolved

lib/internal/modules/esm/resolve.js Outdated Show resolved Hide resolved

lib/internal/modules/esm/resolve.js Outdated Show resolved Hide resolved

lib/internal/modules/esm/resolve.js Outdated Show resolved Hide resolved

addaleax and others added 2 commits March 12, 2020 12:31

fixup! esm: port loader code to JS

1f93185

Co-Authored-By: Jordan Harband <ljharb@gmail.com>

fixup! fixup! esm: port loader code to JS

2038dda

bmeck reviewed Mar 12, 2020

View reviewed changes

lib/internal/modules/esm/resolve.js Outdated Show resolved Hide resolved

fixup! esm: port loader code to JS

826c63b

Co-Authored-By: Bradley Farias <bradley.meck@gmail.com>

bmeck approved these changes Mar 12, 2020

View reviewed changes

fixup! fixup! esm: port loader code to JS

bab3482

guybedford approved these changes Mar 12, 2020

View reviewed changes

addaleax added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Mar 13, 2020

addaleax closed this Mar 13, 2020

addaleax deleted the modulewrap-js branch March 13, 2020 16:35

MylesBorins mentioned this pull request Mar 19, 2020

v13.12.0 proposal #32376

Merged

MylesBorins added the backport-requested-v12.x label Apr 2, 2020

targos removed author ready PRs that have at least one approval, no pending requests for changes, and a CI started. backport-requested-v12.x labels Apr 25, 2020

targos mentioned this pull request May 2, 2020

v12.17.0 release proposal #33197

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

esm: port loader code to JS #32201

esm: port loader code to JS #32201

addaleax commented Mar 11, 2020

MylesBorins commented Mar 11, 2020

bmeck commented Mar 11, 2020

jkrems left a comment

bmeck commented Mar 11, 2020

ljharb left a comment

addaleax commented Mar 11, 2020

addaleax commented Mar 11, 2020

bmeck left a comment

MylesBorins commented Mar 11, 2020

addaleax commented Mar 12, 2020

nodejs-github-bot commented Mar 12, 2020

nodejs-github-bot commented Mar 12, 2020 •

edited by addaleax

Loading

guybedford Mar 12, 2020

jkrems Mar 12, 2020

addaleax commented Mar 13, 2020

MylesBorins commented Apr 2, 2020

esm: port loader code to JS #32201

esm: port loader code to JS #32201

Conversation

addaleax commented Mar 11, 2020

Checklist

MylesBorins commented Mar 11, 2020

bmeck commented Mar 11, 2020

jkrems left a comment

Choose a reason for hiding this comment

bmeck commented Mar 11, 2020

ljharb left a comment

Choose a reason for hiding this comment

addaleax commented Mar 11, 2020

addaleax commented Mar 11, 2020

bmeck left a comment

Choose a reason for hiding this comment

MylesBorins commented Mar 11, 2020

addaleax commented Mar 12, 2020

nodejs-github-bot commented Mar 12, 2020

nodejs-github-bot commented Mar 12, 2020 • edited by addaleax Loading

guybedford Mar 12, 2020

Choose a reason for hiding this comment

jkrems Mar 12, 2020

Choose a reason for hiding this comment

addaleax commented Mar 13, 2020

MylesBorins commented Apr 2, 2020

nodejs-github-bot commented Mar 12, 2020 •

edited by addaleax

Loading