module: add note when cjs loader throws error by gntem · Pull Request #28950 · nodejs/node · GitHub

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

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.

//cc @nodejs/modules-active-members

nits, but nothing blocking

Pending @bmeck’s comments

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

Why was this closed?

I don't know, probably a missclick, sorry

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

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

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

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

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

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

Suggested change

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

Suggested change

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

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

rescinding my approval as I have a comment/change request

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?

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

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.

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

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

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

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.

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

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

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

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

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

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.

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

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

LGTM!

Landed in 427e534

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>

Thanks for the contribution! 🎉

This will allow users to know how to change their project to support ES modules. PR-URL: #28950 Reviewed-By: Bradley Farias <bradley.meck@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Guy Bedford <guybedford@gmail.com>

nodejs/node#28950

bmeck approved these changes Aug 4, 2019

View reviewed changes

Comment thread lib/internal/modules/cjs/loader.js Outdated

Comment thread lib/internal/modules/cjs/loader.js Outdated

ljharb approved these changes Aug 4, 2019

View reviewed changes

gntem force-pushed the esm-modules-note-for-invalid-imports branch from c829d54 to dc3792d Compare August 5, 2019 06:41

Trott reviewed Aug 5, 2019

View reviewed changes

Comment thread lib/internal/modules/cjs/loader.js Outdated

hybrist reviewed Aug 5, 2019

View reviewed changes

Comment thread lib/internal/modules/cjs/loader.js Outdated

Comment thread test/fixtures/es-modules/es-note-promiserej-import-2.cjs Outdated

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

Trott reviewed Aug 7, 2019

View reviewed changes

Comment thread test/es-module/test-esm-cjs-load-error-note.mjs Outdated

gntem force-pushed the esm-modules-note-for-invalid-imports branch from d2ede7d to 3d5741c Compare August 7, 2019 07:09

targos closed this Aug 7, 2019

targos reopened this Aug 7, 2019

Trott reviewed Aug 11, 2019

View reviewed changes

Trott force-pushed the esm-modules-note-for-invalid-imports branch from 53c4037 to 1bc60b0 Compare August 11, 2019 21:54

Trott previously approved these changes Aug 11, 2019

View reviewed changes

Trott reviewed Aug 11, 2019

View reviewed changes

Trott reviewed Aug 12, 2019

View reviewed changes

module: add warning when import,export is detected in CJS context

3372860

This will allow users to know how to change their project to support ES modules.

Trott force-pushed the esm-modules-note-for-invalid-imports branch from d25077d to 3372860 Compare August 12, 2019 22:31

Trott approved these changes Aug 12, 2019

View reviewed changes

guybedford approved these changes Aug 13, 2019

View reviewed changes

Trott closed this Aug 13, 2019

gntem deleted the esm-modules-note-for-invalid-imports branch August 14, 2019 06:49

targos mentioned this pull request Aug 19, 2019

v12.9.0 release proposal #29210

Merged

codebytere added a commit to electron/electron that referenced this pull request Aug 21, 2019

update fixme_use_redefined_version_of_internalmodulestat.patch

35f4186

nodejs/node#28950

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

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

module: add note when cjs loader throws error by gntem · Pull Request #28950 · nodejs/node · GitHub

Uh oh!

Uh oh!

module: add note when cjs loader throws error - #28950

module: add note when cjs loader throws error#28950gntem wants to merge 1 commit intonodejs:masternodejs/node:masterfrom gntem:esm-modules-note-for-invalid-importsgntem/node:esm-modules-note-for-invalid-importsCopy head branch name to clipboard

Conversation

gntem commented Aug 3, 2019

Checklist

Uh oh!

guybedford commented Aug 4, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

guybedford commented Aug 4, 2019

Uh oh!

bmeck left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

ljharb left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

nodejs-github-bot commented Aug 7, 2019

Uh oh!

Uh oh!

bmeck commented Aug 7, 2019

Uh oh!

targos commented Aug 7, 2019

Uh oh!

nodejs-github-bot commented Aug 9, 2019

Uh oh!

nodejs-github-bot commented Aug 9, 2019

Uh oh!

nodejs-github-bot commented Aug 11, 2019

Uh oh!

guybedford commented Aug 11, 2019

Uh oh!

nodejs-github-bot commented Aug 11, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nodejs-github-bot commented Aug 11, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

guybedford Aug 12, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nodejs-github-bot commented Aug 12, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gntem Aug 12, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Trott Aug 12, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

module: add note when cjs loader throws error#28950
gntem wants to merge 1 commit into
nodejs:masterfrom
gntem:esm-modules-note-for-invalid-imports

guybedford commented Aug 4, 2019 •
edited
Loading

guybedford Aug 12, 2019 •
edited
Loading

gntem Aug 12, 2019 •
edited
Loading

Trott Aug 12, 2019 •
edited
Loading