Skip to content

module: fix regression in main ESM loading - #15736

Closed
targos wants to merge 2 commits into
nodejs:masterfrom
targos:fix-esm-load-error
Closed

module: fix regression in main ESM loading#15736
targos wants to merge 2 commits into
nodejs:masterfrom
targos:fix-esm-load-error

Conversation

@targos

Copy link
Copy Markdown
Member

When the requested module cannot be resolved to a file, loading should
always fail, regardless of wether ESM is enabled or not.

Fixes: #15732

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

@targostargos added esm Issues and PRs related to the ECMAScript Modules implementation. module Issues and PRs related to the module subsystem. labels Oct 2, 2017
@targos
targosforce-pushed the fix-esm-load-error branch from cce78f8 to 80b98a5CompareOctober 2, 2017 14:36

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.

assert.fail() here instead of the throw?

@cjihrigcjihrig left a comment

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.

LGTM with a comment/suggestion.

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.

Could the test be simplified by using execFileSync() instead?

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.

But isn't using async-await cooler 😁 ?

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Haha I totally forgot there was a sync version! I was only thinking about how complicated it would be with callbacks xD.

When the requested module cannot be resolved to a file, loading should
always fail, regardless of wether ESM is enabled or not.
Fixes: nodejs#15732
@targos
targosforce-pushed the fix-esm-load-error branch from bfa4db3 to 1d9d6edCompareOctober 6, 2017 04:35
@targos

Copy link
Copy Markdown
MemberAuthor

There is something weird. I changed the test to use execFileSync and it fails as expected with node 8.6.0. However, I cannot make it work with current master because the error I'm catching does not have a stdout or stderr property.

@targos

Copy link
Copy Markdown
MemberAuthor

I think #13601 might have gone too far and removed properties that are supposed to be there according to the docs.

@targos

Copy link
Copy Markdown
MemberAuthor

/cc @mscdex

@gibfahn

Copy link
Copy Markdown
Member

Confirmed that 448c4c6 is the commit that introduces the change, using script provided by @targos:

t.js

const{ execFileSync }=require('child_process');try{execFileSync(process.argv[0],['xxx'],{stdio: 'ignore'});}catch(e){console.log(Object.keys(e));}

Commit before 448c4c6

$ ./node t.js[ 'error', 'file', 'args', 'options', 'envPairs', 'stderr', 'stdout', 'pid', 'output', 'signal', 'status' ]

Commit 448c4c6

$ ./node t.js[ 'status', 'signal' ]

@targos

Copy link
Copy Markdown
MemberAuthor

Quick fix:

diff --git a/lib/child_process.js b/lib/child_process.js
index a3cdadd..f041a86 100644
--- a/lib/child_process.js+++ b/lib/child_process.js@@ -566,8 +566,8 @@ function checkExecSyncError(ret, args, cmd) {
err = new Error(msg);
}
if (err) {
- err.status = ret.status < 0 ? errname(ret.status) : ret.status;- err.signal = ret.signal;+ Object.assign(err, ret);
}
return err;
}

@targostargos added this to the 9.0.0 milestone Oct 7, 2017
@cjihrig

Copy link
Copy Markdown
Contributor

@targos do you plan to work on this, or will it be included in #16147?

@targos

Copy link
Copy Markdown
MemberAuthor

I will see when I'm back from vacation (Oct 23)

guybedford added a commit to guybedford/node that referenced this pull request Oct 18, 2017
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
@targos

Copy link
Copy Markdown
MemberAuthor

I'm going to close this one because #16147 takes care of it.

@targostargos closed this Oct 21, 2017
targos pushed a commit that referenced this pull request Oct 21, 2017
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 deleted the fix-esm-load-error branch October 23, 2017 12:43
MylesBorins pushed a commit that referenced this pull request Oct 23, 2017
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
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
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

Labels

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

process exits silently with --experimental-modules and inexistent entry point

4 participants

@targos@gibfahn@cjihrig@jasnell