esm: port loader code to JS by addaleax · Pull Request #32201 · nodejs/node · GitHub

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.

@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?

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

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

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

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

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

@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.)

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

@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

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>

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

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

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

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!

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>

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

hybrist reviewed Mar 11, 2020

View reviewed changes

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

ljharb reviewed Mar 11, 2020

View reviewed changes

fixup! esm: port loader code to JS

1f20296

bmeck reviewed Mar 11, 2020

View reviewed changes

Comment thread lib/internal/modules/esm/resolve.js

GeoffreyBooth reviewed Mar 11, 2020

View reviewed changes

Comment thread lib/internal/modules/esm/resolve.js Outdated

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

Comment thread lib/internal/modules/esm/get_format.js Outdated

Comment thread lib/internal/modules/esm/resolve.js Outdated

Comment thread lib/internal/modules/esm/resolve.js Outdated

Comment thread lib/internal/modules/esm/resolve.js Outdated

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

Comment thread lib/internal/modules/esm/resolve.js Outdated

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 outstanding review comments, 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 outstanding review comments, 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

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

esm: port loader code to JS by addaleax · Pull Request #32201 · nodejs/node · GitHub

Uh oh!

Uh oh!

esm: port loader code to JS - #32201

esm: port loader code to JS#32201addaleax wants to merge 7 commits intonodejs:masternodejs/node:masterfrom addaleax:modulewrap-jsaddaleax/node:modulewrap-jsCopy head branch name to clipboard

Conversation

addaleax commented Mar 11, 2020

Checklist

Uh oh!

MylesBorins commented Mar 11, 2020

Uh oh!

bmeck commented Mar 11, 2020

Uh oh!

hybrist left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

bmeck commented Mar 11, 2020

Uh oh!

ljharb left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

addaleax commented Mar 11, 2020

Uh oh!

addaleax commented Mar 11, 2020

Uh oh!

Uh oh!

Uh oh!

bmeck left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

MylesBorins commented Mar 11, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

addaleax commented Mar 12, 2020

Uh oh!

Uh oh!

nodejs-github-bot commented Mar 12, 2020

Uh oh!

nodejs-github-bot commented Mar 12, 2020 • edited by addaleax Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

guybedford Mar 12, 2020

Choose a reason for hiding this comment

Uh oh!

hybrist Mar 12, 2020

Choose a reason for hiding this comment

Uh oh!

addaleax commented Mar 13, 2020

Uh oh!

MylesBorins commented Apr 2, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

esm: port loader code to JS#32201
addaleax wants to merge 7 commits into
nodejs:masterfrom
addaleax:modulewrap-js

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