Skip to content

module: ESM main top-level load handling - #16147

Closed
guybedford wants to merge 1 commit into
nodejs:masterfrom
guybedford:esm-main-handling
Closed

module: ESM main top-level load handling#16147
guybedford wants to merge 1 commit into
nodejs:masterfrom
guybedford:esm-main-handling

Conversation

@guybedford

@guybedfordguybedford commented Oct 11, 2017•
edited
Loading

Copy link
Copy Markdown
Contributor

This is an update to the top-level loading for ES module loading based on the latest master, including the fix and test (working async version) from #15736 by @targos.

Firstly we ensure that all resolver errors of code ENOENT are reported as MODULE_NOT_FOUND errors, and secondly, we can now delegate entirely to the ES module loader for resolution as it will internally delegate to the CommonJS loader based on extension rules so we can simplify the top-level logic quite nicely.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines
Affected core subsystem(s)

esmodules

@nodejs-github-botnodejs-github-bot added the module Issues and PRs related to the module subsystem. label Oct 11, 2017
@guybedfordguybedford changed the title module: ESM main handlingmodule: ESM main top-level load handlingOct 11, 2017
hiroppy
hiroppy reviewed Oct 11, 2017
View reviewed changes
Comment threadlib/module.js Outdated

@hiroppyhiroppyOct 11, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Why do you use var?

@guybedfordguybedfordOct 11, 2017

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

To match the code beneath it goes with - this is just moving the existing line from https://github.com/nodejs/node/pull/16147/files#diff-d1234a869b3d648ebfcdce5a76747d71L424. Although it could be refactored in the process certainly.

@mscdexmscdex added the esm Issues and PRs related to the ECMAScript Modules implementation. label Oct 11, 2017
Jessidhia
Jessidhia reviewed Oct 12, 2017
View reviewed changes
Comment threadlib/internal/loader/ModuleRequest.js Outdated

@JessidhiaJessidhiaOct 12, 2017

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Why did this change to let real instead of staying const real? It's still only assigned once during its lifetime.

Comment threadlib/internal/loader/ModuleRequest.js Outdated

@JessidhiaJessidhiaOct 12, 2017

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Is Error the correct class, or is there a more precise subclass with a code?

@guybedfordguybedfordOct 12, 2017

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

I've adjusted this to amend the code to the error thrown from C++. If there is a better way to throw the error with the right code in C++ let me know, but I wasn't able to find existing examples for this pattern.

@guybedford
guybedfordforce-pushed the esm-main-handling branch 2 times, most recently from 7b40f86 to 3c36790CompareOctober 12, 2017 12:00
cjihrig
cjihrig reviewed Oct 15, 2017
View reviewed changes
Comment threadtest/parallel/test-module-main-fail.js Outdated

@cjihrigcjihrigOct 15, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Can you just use execFileSync() if it gets the job done. I think it would simplify the test.

Comment threadtest/parallel/test-module-main-preserve-symlinks-fail.js Outdated

@cjihrigcjihrigOct 15, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Same comment here.

@cjihrigcjihrig mentioned this pull request Oct 15, 2017
Closed
3 tasks
@guybedford

guybedford commented Oct 16, 2017

Copy link
Copy Markdown
ContributorAuthor

@cjihrig thanks for review - that update is just pending merge of #16060 it seems. As soon as that is in I will update this PR.

@guybedford
module: main resolution fix and not found updates
23381c7
This simplifies the top-level load when ES modules are enabled
as we can entirely delegate the module resolver, which will hand
over to CommonJS where appropriate.
All not found errors are made consistent to throw during resolve
and have the MODULE_NOT_FOUND code.
Includes the test case from nodejs#15736
@guybedford

guybedford commented Oct 18, 2017

Copy link
Copy Markdown
ContributorAuthor

I've updated this PR to use the sync tests, while working around the issue in #16060.

Would be nice to get these fixes into the current release cycle if possible, but I understand if the timeline for that has passed already.

@targos

targos commented Oct 21, 2017

Copy link
Copy Markdown
Member

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

@targos

targos commented Oct 21, 2017

Copy link
Copy Markdown
Member

I'm fixing the linting errors and landing.

targos pushed a commit that referenced this pull request Oct 21, 2017
@guybedford@targos
module: fix main resolution and not found updates
fe4675b
This simplifies the top-level load when ES modules are enabled
as we can entirely delegate the module resolver, which will hand
over to CommonJS where appropriate.
All not found errors are made consistent to throw during resolve
and have the MODULE_NOT_FOUND code.
Includes the test case from #15736.
Fixes: #15732
PR-URL: #16147
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@targos

targos commented Oct 21, 2017

Copy link
Copy Markdown
Member

Landed in fe4675b

@targostargos closed this Oct 21, 2017
MylesBorins pushed a commit that referenced this pull request Oct 23, 2017
@guybedford@MylesBorins
module: fix main resolution and not found updates
598787d
This simplifies the top-level load when ES modules are enabled
as we can entirely delegate the module resolver, which will hand
over to CommonJS where appropriate.
All not found errors are made consistent to throw during resolve
and have the MODULE_NOT_FOUND code.
Includes the test case from #15736.
Fixes: #15732
PR-URL: #16147
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
addaleax pushed a commit to ayojs/ayo that referenced this pull request Oct 26, 2017
@guybedford@addaleax
module: fix main resolution and not found updates
7e56f67
This simplifies the top-level load when ES modules are enabled
as we can entirely delegate the module resolver, which will hand
over to CommonJS where appropriate.
All not found errors are made consistent to throw during resolve
and have the MODULE_NOT_FOUND code.
Includes the test case from nodejs/node#15736.
Fixes: nodejs/node#15732
PR-URL: nodejs/node#16147
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
addaleax pushed a commit to ayojs/ayo that referenced this pull request Dec 7, 2017
@guybedford@addaleax
module: fix main resolution and not found updates
e7f1e84
This simplifies the top-level load when ES modules are enabled
as we can entirely delegate the module resolver, which will hand
over to CommonJS where appropriate.
All not found errors are made consistent to throw during resolve
and have the MODULE_NOT_FOUND code.
Includes the test case from nodejs/node#15736.
Fixes: nodejs/node#15732
PR-URL: nodejs/node#16147
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@fhinkelfhinkelfhinkel approved these changes
@jasnelljasnelljasnell approved these changes
@targostargostargos approved these changes
@cjihrigcjihrigcjihrig approved these changes
+2 more reviewers
@JessidhiaJessidhiaJessidhia left review comments
@hiroppyhiroppyhiroppy left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

esmIssues and PRs related to the ECMAScript Modules implementation.moduleIssues and PRs related to the module subsystem.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

10 participants

@guybedford@targos@Jessidhia@fhinkel@jasnell@cjihrig@hiroppy@mscdex@MylesBorins@nodejs-github-bot