★ wanayoo — archive 1999 https://github.com/nodejs/node/pull/28950Nouvelle recherche | Portail wanayoo
Skip to content

/node

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
wants to merge 1 commit into from
+195 −18

Conversation

@gntem
Copy link
Contributor

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

This comment has been minimized.

Sign in to view
Copy link
Contributor

commented Aug 4, 2019 •
edited

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

This comment has been minimized.

Sign in to view
Copy link
Contributor

commented Aug 4, 2019

//cc @nodejs/modules-active-members
@bmeck

bmeck approved these changes Aug 4, 2019
View changes

Copy link
Member

left a comment

nits, but nothing blocking
Show resolved Hide resolved lib/internal/modules/cjs/loader.js
Show resolved Hide resolved lib/internal/modules/cjs/loader.js Outdated
@ljharb

ljharb approved these changes Aug 4, 2019
View changes

Copy link
Member

left a comment

Pending @bmeck’s comments

@gntem gntem force-pushed the gntem:esm-modules-note-for-invalid-imports branch from c829d54 to dc3792d Aug 5, 2019

@Trott

Trott reviewed Aug 5, 2019
View changes

Show resolved Hide resolved lib/internal/modules/cjs/loader.js Outdated
@jkrems

jkrems reviewed Aug 5, 2019
View changes

Show resolved Hide resolved lib/internal/modules/cjs/loader.js Outdated
Show resolved Hide resolved test/fixtures/es-modules/es-note-promiserej-import-2.cjs Outdated

@gntem gntem force-pushed the gntem:esm-modules-note-for-invalid-imports branch 2 times, most recently from 610067e to d2ede7d Aug 6, 2019

@nodejs-github-bot

This comment has been minimized.

Sign in to view
Copy link

commented Aug 7, 2019

CI: https://ci.nodejs.org/job/node-test-pull-request/24874/
@Trott

Trott reviewed Aug 7, 2019
View changes

Show resolved Hide resolved test/es-module/test-esm-cjs-load-error-note.mjs Outdated

@gntem gntem force-pushed the gntem:esm-modules-note-for-invalid-imports branch from d2ede7d to 3d5741c Aug 7, 2019

@targos targos closed this Aug 7, 2019

@bmeck

This comment has been minimized.

Sign in to view
Copy link
Member

commented Aug 7, 2019

Why was this closed?
@targos

This comment has been minimized.

Sign in to view
Copy link
Member

commented Aug 7, 2019

I don't know, probably a missclick, sorry

@targos targos reopened this Aug 7, 2019

@nodejs-github-bot

This comment has been minimized.

Sign in to view
Copy link

commented Aug 9, 2019

CI: https://ci.nodejs.org/job/node-test-pull-request/24906/
@nodejs-github-bot

This comment has been minimized.

Sign in to view
Copy link

commented Aug 9, 2019

CI: https://ci.nodejs.org/job/node-test-pull-request/24911/
@nodejs-github-bot

This comment has been minimized.

Sign in to view
Copy link

commented Aug 11, 2019

CI: https://ci.nodejs.org/job/node-test-pull-request/24934/
@guybedford

This comment has been minimized.

Sign in to view
Copy link
Contributor

commented Aug 11, 2019

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

This comment has been minimized.

Sign in to view
Copy link

commented Aug 11, 2019

CI: https://ci.nodejs.org/job/node-test-pull-request/24937/
@Trott

Trott reviewed Aug 11, 2019
View changes

lib/internal/modules/cjs/loader.js Outdated
*/
if (err.message.startsWith('Unexpected token export') ||
(/^\s*import(?=[ {'"*])\s*(?![ (])/).test(lineWithErr)) {
err.stack = 'Note: To load an ES module set "type": "module" ' +

This comment has been minimized.

Sign in to view
Copy link
@Trott

Trott Aug 11, 2019

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

Trott reviewed Aug 11, 2019
View changes

test/es-module/test-esm-cjs-load-error-note.mjs Outdated
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 ' +

This comment has been minimized.

Sign in to view
Copy link
@Trott

Trott Aug 11, 2019

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

@Trott Trott force-pushed the gntem:esm-modules-note-for-invalid-imports branch from 53c4037 to 1bc60b0 Aug 11, 2019

@nodejs-github-bot

This comment has been minimized.

Sign in to view
Copy link

commented Aug 11, 2019

CI: https://ci.nodejs.org/job/node-test-pull-request/24941/
@Trott

Trott reviewed Aug 11, 2019
View changes

@Trott Trott dismissed their stale review Aug 11, 2019

rescinding my approval as I have a comment/change request

@Trott

Trott reviewed Aug 11, 2019
View changes

lib/internal/modules/cjs/loader.js Outdated
*/
if (err.message.startsWith('Unexpected token export') ||
(/^\s*import(?=[ {'"*])\s*(?![ (])/).test(lineWithErr)) {
err.stack = 'To load an ES module, set "type": "module" ' +

This comment has been minimized.

Sign in to view
Copy link
@Trott

Trott Aug 11, 2019

Member

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?

This comment has been minimized.

Sign in to view
Copy link
@ljharb

ljharb Aug 11, 2019

Member

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

This comment has been minimized.

Sign in to view
Copy link
@guybedford

guybedford Aug 12, 2019 •
edited

Contributor

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.

This comment has been minimized.

Sign in to view
Copy link
@guybedford

guybedford Aug 12, 2019

Contributor

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

This comment has been minimized.

Sign in to view
Copy link
@Trott

Trott Aug 12, 2019

Member

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

This comment has been minimized.

Sign in to view
Copy link
@ljharb

ljharb Aug 12, 2019

Member

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

This comment has been minimized.

Sign in to view
Copy link
@gntem

gntem Aug 12, 2019

Author Contributor

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

This comment has been minimized.

Sign in to view
Copy link

commented Aug 12, 2019

CI: https://ci.nodejs.org/job/node-test-pull-request/24944/
@Trott

Trott reviewed Aug 12, 2019
View changes

lib/internal/modules/cjs/loader.js Outdated
*/
if (err.message.startsWith('Unexpected token export') ||
(/^\s*import(?=[ {'"*])\s*(?![ (])/).test(lineWithErr)) {
console.warn('To load an ES module, set "type": "module" ' +

This comment has been minimized.

Sign in to view
Copy link
@Trott

Trott Aug 12, 2019

Member

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

This comment has been minimized.

Sign in to view
Copy link
@gntem

gntem Aug 12, 2019 •
edited

Author Contributor

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

This comment has been minimized.

Sign in to view
Copy link
@Trott

Trott Aug 12, 2019 •
edited

Member

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):

This comment has been minimized.

Sign in to view
Copy link
@Trott

Trott Aug 12, 2019

Member

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

This comment has been minimized.

Sign in to view
Copy link
@Trott

Trott Aug 12, 2019

Member

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....
@gntem @Trott
module: add warning when import,export is detected in CJS context 
This will allow users to know how to change their project to support
ES modules.
Loading status checks…
3372860

@Trott Trott force-pushed the gntem:esm-modules-note-for-invalid-imports branch from d25077d to 3372860 Aug 12, 2019

@Trott

This comment has been minimized.

Sign in to view
Copy link
Member

commented Aug 12, 2019

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

Trott approved these changes Aug 12, 2019
View changes

@nodejs-github-bot

This comment has been minimized.

Sign in to view
Copy link

commented Aug 12, 2019

CI: https://ci.nodejs.org/job/node-test-pull-request/24955/
@guybedford

guybedford approved these changes Aug 13, 2019
View changes

Copy link
Contributor

left a comment

LGTM!
@Trott

This comment has been minimized.

Sign in to view
Copy link
Member

commented Aug 13, 2019

Landed in 427e534

@Trott Trott closed this Aug 13, 2019

Trott added a commit to Trott/io.js that referenced this pull request Aug 13, 2019

@gntem @Trott
module: add warning when import,export is detected in CJS context 
This will allow users to know how to change their project to support
ES modules.

PR-URL: nodejs#28950
Reviewed-By: Bradley Farias <bradley.meck@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Guy Bedford <guybedford@gmail.com>
427e534
@Trott

This comment has been minimized.

Sign in to view
Copy link
Member

commented Aug 13, 2019

Thanks for the contribution! 🎉

@gntem gntem deleted the gntem:esm-modules-note-for-invalid-imports branch Aug 14, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ljharb ljharb

@bmeck bmeck

@jkrems jkrems

@guybedford guybedford

@Trott Trott

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
8 participants
@gntem @guybedford @nodejs-github-bot @bmeck @targos @Trott @ljharb @jkrems
You can’t perform that action at this time.