Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

module: fix regression in main ESM loading #15736

Closed
wants to merge 2 commits into from
Closed

module: fix regression in main ESM loading #15736

wants to merge 2 commits into from

Conversation

targos
Copy link
Member

@targos targos commented Oct 2, 2017

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

@targos targos 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 targos force-pushed the fix-esm-load-error branch from cce78f8 to 80b98a5 Compare October 2, 2017 14:36
jasnell
jasnell reviewed Oct 2, 2017
View reviewed changes
test/parallel/test-module-main-fail.js Outdated
continue;
}
throw new Error('Executing node with inexistent entry point should ' +
`fail. Entry point: ${entryPoint}, Flags: [${args}]`);
Copy link
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?

cjihrig
cjihrig approved these changes Oct 2, 2017
View reviewed changes
Copy link
Contributor

@cjihrig cjihrig left a comment

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.

test/parallel/test-module-main-fail.js Outdated
for (const args of flags) {
for (const entryPoint of entryPoints) {
try {
await execFile(node, args.concat(entryPoint));
Copy link
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
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
Member Author

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.

targos added 2 commits October 5, 2017 21:35
@targos
module: fix regression in main ESM loading
527c727 
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
squash: use assert.fail, execFileSync
1d9d6ed
@targos targos force-pushed the fix-esm-load-error branch from bfa4db3 to 1d9d6ed Compare October 6, 2017 04:35
@targos
Copy link
Member Author

targos commented Oct 6, 2017

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
Member Author

targos commented Oct 6, 2017

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

@targos
Copy link
Member Author

targos commented Oct 6, 2017

/cc @mscdex

@gibfahn
Copy link
Member

gibfahn commented Oct 6, 2017

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
Member Author

targos commented Oct 6, 2017

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;
 }

@targos targos added this to the 9.0.0 milestone Oct 7, 2017
@guybedford guybedford mentioned this pull request Oct 11, 2017
Closed
3 tasks
@cjihrig
Copy link
Contributor

cjihrig commented Oct 15, 2017

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

@targos
Copy link
Member Author

targos commented Oct 16, 2017

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
@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
@targos
Copy link
Member Author

targos commented Oct 21, 2017

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

@targos targos closed this Oct 21, 2017
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 <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
@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
@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 <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
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 <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
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 <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gibfahn gibfahn gibfahn left review comments

@cjihrig cjihrig cjihrig approved these changes

@jasnell jasnell jasnell approved these changes

Assignees
No one assigned
Labels
esm Issues and PRs related to the ECMAScript Modules implementation. module Issues and PRs related to the module subsystem.
Projects
None yet
Milestone
9.0.0
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