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.

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

module: add note when cjs loader throws error #28950

Closed

Conversation

gntem
Copy link
Contributor

@gntem gntem commented Aug 3, 2019

subsystem: module: add note to error when import,export is detected.

These will allow users to know how to change their project to support
es modules when the message is displayed.

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

@guybedford
Copy link
Contributor

guybedford commented Aug 4, 2019

I've been working with @gntem as part of the mentorship program and one of the projects we discussed was seeing if we could improve the error messages for using ES modules in Node.js to provide easier onboarding.

Currently if you write a Node.js module say x.js containing import/export syntax you get a syntax error message when running node --experimental-modules x.js, looking like:

(node:7720) ExperimentalWarning: The ESM module loader is experimental.
C:\Users\Guy\x.js:1
export var p = 5;
^^^^^^

SyntaxError: Unexpected token export
    at Module._compile (internal/modules/cjs/loader.js:720:23)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:787:10)
    at Module.load (internal/modules/cjs/loader.js:643:32)
    at Function.Module._load (internal/modules/cjs/loader.js:556:12)
    at internal/modules/esm/translators.js:87:15
    at Object.meta.done (internal/modules/esm/create_dynamic_module.js:48:9)
    at file:///C:/Users/Guy/x.js:9:13
    at ModuleJob.run (internal/modules/esm/module_job.js:111:37)
    at processTicksAndRejections (internal/process/task_queues.js:85:5)
    at async Loader.import (internal/modules/esm/loader.js:134:24)

With this PR, the above error message instead becomes:

(node:7720) ExperimentalWarning: The ESM module loader is experimental.
Note: To load an ES module using --experimental-modules set "type": "module" in
the package.json or use the .mjs extension.

C:\Users\Guy\x.js:1
export var p = 5;
^^^^^^

SyntaxError: Unexpected token export
    at Module._compile (internal/modules/cjs/loader.js:720:23)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:787:10)
    at Module.load (internal/modules/cjs/loader.js:643:32)
    at Function.Module._load (internal/modules/cjs/loader.js:556:12)
    at internal/modules/esm/translators.js:87:15
    at Object.meta.done (internal/modules/esm/create_dynamic_module.js:48:9)
    at file:///C:/Users/Guy/x.js:9:13
    at ModuleJob.run (internal/modules/esm/module_job.js:111:37)
    at processTicksAndRejections (internal/process/task_queues.js:85:5)
    at async Loader.import (internal/modules/esm/loader.js:134:24)

Providing the actionable information to the user to use modules in Node.js.

The tricky case here was catching invalid import syntax as that throws a different unexpected token message depending on the syntax as dynamic import() is supported in CommonJS. A regular expression approach is thus used to check the import cases specifically.

@guybedford
Copy link
Contributor

//cc @nodejs/modules-active-members

Copy link
Member

@bmeck bmeck left a comment

Choose a reason for hiding this comment

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

nits, but nothing blocking

lib/internal/modules/cjs/loader.js Show resolved Hide resolved
lib/internal/modules/cjs/loader.js Outdated Show resolved Hide resolved
Copy link
Member

@ljharb ljharb left a comment

Choose a reason for hiding this comment

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

Pending @bmeck’s comments

@gntem gntem force-pushed the esm-modules-note-for-invalid-imports branch from c829d54 to dc3792d Compare August 5, 2019 06:41
@gntem gntem force-pushed the esm-modules-note-for-invalid-imports branch 2 times, most recently from 610067e to d2ede7d Compare August 6, 2019 15:04
@nodejs-github-bot
Copy link
Collaborator

@gntem gntem force-pushed the esm-modules-note-for-invalid-imports branch from d2ede7d to 3d5741c Compare August 7, 2019 07:09
@targos targos closed this Aug 7, 2019
@bmeck
Copy link
Member

bmeck commented Aug 7, 2019

Why was this closed?

@targos
Copy link
Member

targos commented Aug 7, 2019

I don't know, probably a missclick, sorry

@targos targos reopened this Aug 7, 2019
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@guybedford
Copy link
Contributor

Test failures here all seem to be flakes at this point.

Any further feedback on this PR at all? Good to merge?

@nodejs-github-bot
Copy link
Collaborator

*/
if (err.message.startsWith('Unexpected token export') ||
(/^\s*import(?=[ {'"*])\s*(?![ (])/).test(lineWithErr)) {
err.stack = 'Note: To load an ES module set "type": "module" ' +
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
err.stack = 'Note: To load an ES module set "type": "module" ' +
err.stack = 'To load an ES module, set "type": "module" ' +

const Import5 = fixtures.path('/es-modules/es-note-unexpected-import-5.cjs');

// Expect note to be included in the error output
const expectedNote = 'Note: To load an ES module ' +
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
const expectedNote = 'Note: To load an ES module ' +
const expectedNote = 'To load an ES module, ' +

@Trott Trott force-pushed the esm-modules-note-for-invalid-imports branch from 53c4037 to 1bc60b0 Compare August 11, 2019 21:54
@nodejs-github-bot
Copy link
Collaborator

Trott
Trott previously approved these changes Aug 11, 2019
@Trott Trott dismissed their stale review August 11, 2019 21:56

rescinding my approval as I have a comment/change request

*/
if (err.message.startsWith('Unexpected token export') ||
(/^\s*import(?=[ {'"*])\s*(?![ (])/).test(lineWithErr)) {
err.stack = 'To load an ES module, set "type": "module" ' +
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't text like this be added to err.message or something like that? Wedging stuff that isn't a stack trace into err.stack seems like the wrong place to put it, no?

Copy link
Member

Choose a reason for hiding this comment

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

actually yeah, good call - this will make the stacks violate the spec, if i can ever get that proposal advanced.

Copy link
Contributor

@guybedford guybedford Aug 12, 2019

Choose a reason for hiding this comment

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

I believe the reasoning here was originally that if we append the message the error looks like:

(node:7720) ExperimentalWarning: The ESM module loader is experimental.

C:\Users\Guy\x.js:1
export var p = 5;
^^^^^^

To load an ES module using --experimental-modules set "type": "module" in
the package.json or use the .mjs extension.

SyntaxError: Unexpected token export
    at Module._compile (internal/modules/cjs/loader.js:720:23)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:787:10)
    at Module.load (internal/modules/cjs/loader.js:643:32)
    at Function.Module._load (internal/modules/cjs/loader.js:556:12)
    at internal/modules/esm/translators.js:87:15
    at Object.meta.done (internal/modules/esm/create_dynamic_module.js:48:9)
    at file:///C:/Users/Guy/x.js:9:13
    at ModuleJob.run (internal/modules/esm/module_job.js:111:37)
    at processTicksAndRejections (internal/process/task_queues.js:85:5)
    at async Loader.import (internal/modules/esm/loader.js:134:24)

where the note is hidden beneath the error frame.

Ideally we'd have more control over mutating these error outputs though.

Copy link
Contributor

Choose a reason for hiding this comment

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

Correction - I mean the note is hidden beneath the error frame.

Copy link
Member

Choose a reason for hiding this comment

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

Maybe an acceptable solution would be to leave the error contents alone and emit a separate warning with the note?

Copy link
Member

Choose a reason for hiding this comment

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

You could even overwrite the own data property with an accessory that console.warned.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe an acceptable solution would be to leave the error contents alone and emit a separate warning with the note?

So call console.warn after the error condition, would be enough, right? it would still print before the error message and stack.

@nodejs-github-bot
Copy link
Collaborator

*/
if (err.message.startsWith('Unexpected token export') ||
(/^\s*import(?=[ {'"*])\s*(?![ (])/).test(lineWithErr)) {
console.warn('To load an ES module, set "type": "module" ' +
Copy link
Member

Choose a reason for hiding this comment

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

I think we'd want to use process.emitWarning() instead to allow the end user to decide how to handle warnings?

Copy link
Contributor Author

@gntem gntem Aug 12, 2019

Choose a reason for hiding this comment

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

I can't seem to get the tests passing I changed to listen for .on('warning',(warning) => ..assertions..) or .on('close',...) also debugged the stderr output but nothing is showing up, the arguments --trace-warnings is provided to the spawned process, but no warning is emitted. In that line, I just replaced with the console.warn with

 process.emitWarning('To load an ES module, set "type": "module" ' +
      'in the package.json or use the .mjs ' +
      'extension.\n\n', 'ExperimentalWarning', undefined);

Copy link
Member

@Trott Trott Aug 12, 2019

Choose a reason for hiding this comment

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

I don't think we want it as an ExperimentalWarning because we'll want it even after ES modules are no longer experimental. I'd say stick with the default (which is just Warning):

Copy link
Member

Choose a reason for hiding this comment

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

I wonder if the problem you're having is that the error is thrown before the warning is emitted....

Copy link
Member

Choose a reason for hiding this comment

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

To get it to work, you have to use the undocumented now argument for process.emitWarning(). I'll add a commit to this branch in a little bit....

This will allow users to know how to change their project to support
ES modules.
@Trott Trott force-pushed the esm-modules-note-for-invalid-imports branch from d25077d to 3372860 Compare August 12, 2019 22:31
@Trott
Copy link
Member

Trott commented Aug 12, 2019

Updated, rebased, squashed, force-pushed. I think this is ready to go?

@nodejs-github-bot
Copy link
Collaborator

Copy link
Contributor

@guybedford guybedford left a comment

Choose a reason for hiding this comment

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

LGTM!

@Trott
Copy link
Member

Trott commented Aug 13, 2019

Landed in 427e534

@Trott Trott closed this Aug 13, 2019
Trott pushed a commit to Trott/io.js that referenced this pull request Aug 13, 2019
This will allow users to know how to change their project to support
ES modules.

PR-URL: nodejs#28950
Reviewed-By: Bradley Farias <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Guy Bedford <[email protected]>
@Trott
Copy link
Member

Trott commented Aug 13, 2019

Thanks for the contribution! 🎉

@gntem gntem deleted the esm-modules-note-for-invalid-imports branch August 14, 2019 06:49
targos pushed a commit that referenced this pull request Aug 19, 2019
This will allow users to know how to change their project to support
ES modules.

PR-URL: #28950
Reviewed-By: Bradley Farias <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Guy Bedford <[email protected]>
targos pushed a commit that referenced this pull request Aug 19, 2019
This will allow users to know how to change their project to support
ES modules.

PR-URL: #28950
Reviewed-By: Bradley Farias <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Guy Bedford <[email protected]>
codebytere added a commit to electron/electron that referenced this pull request Aug 21, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants