module: better error for named exports from cjs #33256

MylesBorins · 2020-05-05T20:19:17Z

We do not support importing named exports from a CJS module.
This change decorates the error message for missing named exports in
the case that the module being imported is expected to be CJS by the
ESM loader.

Some high level questions

Are folks ok with how I've decorated the error? It is a bit fragile, as it relies on the format of the error in V8.
Are folks ok with the copy of the error message? I took a quick stab at it but it could maybe be better

Needs a test, but wanted to confirm folks would be open to doing this before hacking on a test.

guybedford · 2020-05-05T20:24:47Z

Definitely a +1 to this, it is much needed.

Ideally the error message could contain the exact context and solution, eg something like:

No named export named "asdf" for CommonJS module /path/to/module.js
CommonJS modules only have a "default" export, try importing this instead

lib/internal/modules/esm/module_job.js

MylesBorins · 2020-05-05T20:28:38Z

@guybedford getting the specific named export that was attempted to be imported would be a bit more work digging into the internals (is a hidden part of the error message). Happy to include the solution regarding the default but was unsure how verbose to be

ljharb

cc @nodejs/modules-active-members

lib/internal/modules/esm/module_job.js

GeoffreyBooth · 2020-05-05T21:38:20Z

Ideally the error message could contain the exact context and solution, eg something like:

The requested module 'pkg' is expected to be of type CommonJS which does not
support named exports. CommonJS modules can be imported by importing the
default, for example: import pkg from 'pkg'; const { name } = pkg;

lib/internal/modules/esm/module_job.js

addaleax · 2020-05-05T21:39:39Z

@GeoffreyBooth … jinx ;)

MylesBorins · 2020-05-05T22:16:15Z

Ok... I've accepted the proposed changes and made another stab at the text. Currently it uses a generic module name and named imports... We should be able to make the error message include the explicit named imports that were used but I'll need to dig in a bit how the message is being decorated with the arrow to do this.

lib/internal/util.js

MylesBorins · 2020-05-06T03:54:03Z

ok... it is gross but it works 🎉.

Added options argument to the decorate error function and utilized the existing code that grabs the failing import statement. Errors will now point out the error and offer alternative working code.

lib/internal/modules/esm/module_job.js

lib/internal/util.js

benjamingr · 2020-05-06T09:43:21Z

I'm not up to date enough in that space right now to approve IMO but changes LGTM.

I just want to make sure that semverness is taken into account and that if modules are stable we make sure to choose the appropriate semverness for an error message change.

MylesBorins · 2020-05-06T15:00:06Z

@benjamingr ESM is still marked experimental in the docs and changes are not subject to Semver

mafintosh · 2020-05-06T15:50:21Z

@MylesBorins thanks for this! much appreciated

lib/internal/modules/esm/module_job.js

MylesBorins · 2020-05-06T21:22:23Z

This is catching a V8 error, not an node.js error. I can double check but I don't believe these errors have error.code

…

On Wed, May 6, 2020 at 5:18 PM Jordan Harband ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In lib/internal/modules/esm/module_job.js <#33256 (comment)>: > @@ -92,7 +94,22 @@ class ModuleJob { this.module.instantiate(); } } catch (e) { - decorateErrorStack(e); + if (e.message.includes('does not provide an export named')) { + const rootURL = this.module.url; + const childSpecifier = StringPrototypeMatch( + e.message, + // Match the first string in either single or double quotes fair enough; it still seems safer to me to use error codes to pivot, and to store this information as a property on the error rather than parsing it out of the error message — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#33256 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AADZYVZE2I67XE62JGFUFMLRQHHYTANCNFSM4MZ4EM7Q> .

lib/internal/util.js

MylesBorins · 2020-05-07T06:47:04Z

PTAL I've added tests and refactored pretty significantly including documentation in the code that gets a bit funky.

I do not have tests for specifiers that have escaped quotes as my various manual tests seem to imply it is impossible to import esmodules in Node.js with escaped quotes in the specifier. If my assumption here is wrong someone please let me know.

targos · 2020-05-07T06:55:22Z

I do not have tests for specifiers that have escaped quotes as my various manual tests seem to imply it is impossible to import esmodules in Node.js with escaped quotes in the specifier. If my assumption here is wrong someone please let me know.

Not sure what you mean. This works:

import { value } from './a\'b.js';

lib/internal/modules/esm/module_job.js

ljharb

LGTM!

lib/internal/util.js

MylesBorins · 2020-05-07T08:31:25Z

OK... so it looks like escaped strings can work as specifiers... and it also looks like V8 normalizes this before printing a message... so independent of if single or double quotes have been used, if quote inside the string are single / double or escaped, the message will always include only single quote 😅

SyntaxError: The requested module './oh'no.cjs' does not provide an export named 'value'

I've gone ahead and removed the use of a regular expression and am instead manually separating the string based on knowing the shape it will be (this is hardcoded into V8). If this changes internally in V8 all the tests will blow up.

lib/internal/modules/esm/module_job.js

addaleax

LGTM

nodejs-github-bot · 2020-05-07T22:57:07Z

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

test/es-module/test-esm-cjs-named-error.mjs

MylesBorins · 2020-05-08T05:07:12Z

OK... so this was a wild ride but I've fixed all the things. I didn't write a test for the indirection sub package.json edge case I was thinking could be an issue... but all child modules, including bare imports, are 100% being resolved relative to the file that the import statement was contained in 🎉.

This should cover all cases of escape characters, although might give inaccurate results depending on the string we get from V8.

nodejs-github-bot · 2020-05-08T05:08:53Z

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

ljharb

LGTM

ljharb · 2020-05-08T05:07:51Z

test/fixtures/es-modules/package-cjs-named-error/json-hack/fail.js

@@ -0,0 +1,3 @@
+module.exports = {
+  comeOn: 'fhqwhgads'


everybody to the limit

Maybe foo would be more appropriate here?

i don't see how anything could possibly be more appropriate than this particular reference

lib/internal/modules/esm/module_job.js

We do not support importing named exports from a CJS module. This change decorates the error message for missing named exports in the case where the module being imported is expected to be CJS by the ESM loader. Signed-off-by: Myles Borins <myles.borins@gmail.com>

nodejs-github-bot · 2020-05-08T15:43:55Z

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

nodejs-github-bot · 2020-05-08T16:20:16Z

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

MylesBorins · 2020-05-08T17:08:24Z

CI is green. Going to land this EOD unless there are any blockers

We do not support importing named exports from a CJS module. This change decorates the error message for missing named exports in the case where the module being imported is expected to be CJS by the ESM loader. Signed-off-by: Myles Borins <myles.borins@gmail.com> PR-URL: #33256 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Michaël Zasso <targos@protonmail.com>

MylesBorins · 2020-05-08T19:35:38Z

Landed in 2c3c9f5

We do not support importing named exports from a CJS module. This change decorates the error message for missing named exports in the case where the module being imported is expected to be CJS by the ESM loader. Signed-off-by: Myles Borins <myles.borins@gmail.com> PR-URL: #33256 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Michaël Zasso <targos@protonmail.com>

nodejs-github-bot added the esm Issues and PRs related to the ECMAScript Modules implementation. label May 5, 2020

mathiasbynens reviewed May 5, 2020

View reviewed changes

lib/internal/modules/esm/module_job.js Outdated Show resolved Hide resolved

MylesBorins mentioned this pull request May 5, 2020

Rediscussion for out-of-order CJS exec? nodejs/modules#509

Closed

ljharb reviewed May 5, 2020

View reviewed changes

lib/internal/modules/esm/module_job.js Outdated Show resolved Hide resolved

lib/internal/modules/esm/module_job.js Outdated Show resolved Hide resolved

lib/internal/modules/esm/module_job.js Outdated Show resolved Hide resolved

addaleax reviewed May 5, 2020

View reviewed changes

lib/internal/modules/esm/module_job.js Outdated Show resolved Hide resolved

devsnek reviewed May 5, 2020

View reviewed changes

lib/internal/modules/esm/module_job.js Outdated Show resolved Hide resolved

addaleax reviewed May 6, 2020

View reviewed changes

lib/internal/util.js Outdated Show resolved Hide resolved

ljharb reviewed May 6, 2020

View reviewed changes

lib/internal/modules/esm/module_job.js Outdated Show resolved Hide resolved

lib/internal/util.js Outdated Show resolved Hide resolved

ljharb reviewed May 6, 2020

View reviewed changes

lib/internal/modules/esm/module_job.js Outdated Show resolved Hide resolved

lib/internal/modules/esm/module_job.js Outdated Show resolved Hide resolved

GeoffreyBooth reviewed May 6, 2020

View reviewed changes

lib/internal/util.js Outdated Show resolved Hide resolved

GeoffreyBooth reviewed May 7, 2020

View reviewed changes

lib/internal/util.js Outdated Show resolved Hide resolved

MylesBorins force-pushed the better-cjs-named-export-error branch from 69199b7 to 7a40821 Compare May 7, 2020 06:45

ljharb reviewed May 7, 2020

View reviewed changes

lib/internal/modules/esm/module_job.js Outdated Show resolved Hide resolved

ljharb reviewed May 7, 2020

View reviewed changes

lib/internal/util.js Outdated Show resolved Hide resolved

MylesBorins commented May 7, 2020

View reviewed changes

lib/internal/modules/esm/module_job.js Outdated Show resolved Hide resolved

MylesBorins commented May 7, 2020

View reviewed changes

lib/internal/modules/esm/module_job.js Outdated Show resolved Hide resolved

addaleax approved these changes May 7, 2020

View reviewed changes

GeoffreyBooth reviewed May 8, 2020

View reviewed changes

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

MylesBorins force-pushed the better-cjs-named-export-error branch from 603a879 to 06cdf08 Compare May 8, 2020 05:06

ljharb reviewed May 8, 2020

View reviewed changes

targos reviewed May 8, 2020

View reviewed changes

lib/internal/modules/esm/module_job.js Outdated Show resolved Hide resolved

targos approved these changes May 8, 2020

View reviewed changes

MylesBorins force-pushed the better-cjs-named-export-error branch from d0df6ab to 75f08f9 Compare May 8, 2020 15:42

MylesBorins closed this May 8, 2020

codebytere mentioned this pull request May 18, 2020

v14.3.0 proposal #33452

Merged

codebytere mentioned this pull request Jun 9, 2020

v12.18.1 proposal #33811

Merged

ctavan mentioned this pull request Sep 18, 2020

module: improve error decoration for cjs named exports for multi-line import statements #35259

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

module: better error for named exports from cjs #33256

module: better error for named exports from cjs #33256

MylesBorins commented May 5, 2020

guybedford commented May 5, 2020

MylesBorins commented May 5, 2020

ljharb left a comment

GeoffreyBooth commented May 5, 2020

addaleax commented May 5, 2020

MylesBorins commented May 5, 2020

MylesBorins commented May 6, 2020

benjamingr commented May 6, 2020

MylesBorins commented May 6, 2020

mafintosh commented May 6, 2020

MylesBorins commented May 6, 2020 via email

MylesBorins commented May 7, 2020

targos commented May 7, 2020

ljharb left a comment

MylesBorins commented May 7, 2020

addaleax left a comment

nodejs-github-bot commented May 7, 2020

MylesBorins commented May 8, 2020

nodejs-github-bot commented May 8, 2020

ljharb left a comment

ljharb May 8, 2020

DerekNonGeneric May 8, 2020

ljharb May 8, 2020 •

edited

Loading

nodejs-github-bot commented May 8, 2020

nodejs-github-bot commented May 8, 2020

MylesBorins commented May 8, 2020

MylesBorins commented May 8, 2020

module: better error for named exports from cjs #33256

module: better error for named exports from cjs #33256

Conversation

MylesBorins commented May 5, 2020

guybedford commented May 5, 2020

MylesBorins commented May 5, 2020

ljharb left a comment

Choose a reason for hiding this comment

GeoffreyBooth commented May 5, 2020

addaleax commented May 5, 2020

MylesBorins commented May 5, 2020

MylesBorins commented May 6, 2020

benjamingr commented May 6, 2020

MylesBorins commented May 6, 2020

mafintosh commented May 6, 2020

MylesBorins commented May 6, 2020 via email

MylesBorins commented May 7, 2020

targos commented May 7, 2020

ljharb left a comment

Choose a reason for hiding this comment

MylesBorins commented May 7, 2020

addaleax left a comment

Choose a reason for hiding this comment

nodejs-github-bot commented May 7, 2020

MylesBorins commented May 8, 2020

nodejs-github-bot commented May 8, 2020

ljharb left a comment

Choose a reason for hiding this comment

ljharb May 8, 2020

Choose a reason for hiding this comment

DerekNonGeneric May 8, 2020

Choose a reason for hiding this comment

ljharb May 8, 2020 • edited Loading

Choose a reason for hiding this comment

nodejs-github-bot commented May 8, 2020

nodejs-github-bot commented May 8, 2020

MylesBorins commented May 8, 2020

MylesBorins commented May 8, 2020

ljharb May 8, 2020 •

edited

Loading