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: better error for named exports from cjs #33256

Closed

Conversation

MylesBorins
Copy link
Contributor

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.

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

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

@MylesBorins
Copy link
Contributor Author

@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

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.

cc @nodejs/modules-active-members

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

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;

@addaleax
Copy link
Member

addaleax commented May 5, 2020

@GeoffreyBooth … jinx ;)

@MylesBorins
Copy link
Contributor Author

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 Outdated Show resolved Hide resolved
@MylesBorins
Copy link
Contributor Author

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 Outdated Show resolved Hide resolved
lib/internal/util.js Outdated Show resolved Hide resolved
@benjamingr
Copy link
Member

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
Copy link
Contributor Author

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

@mafintosh
Copy link
Member

@MylesBorins thanks for this! much appreciated

lib/internal/modules/esm/module_job.js Outdated Show resolved Hide resolved
lib/internal/modules/esm/module_job.js Outdated Show resolved Hide resolved
@MylesBorins
Copy link
Contributor Author

MylesBorins commented May 6, 2020 via email

lib/internal/util.js Outdated Show resolved Hide resolved
lib/internal/util.js Outdated Show resolved Hide resolved
@MylesBorins MylesBorins force-pushed the better-cjs-named-export-error branch from 69199b7 to 7a40821 Compare May 7, 2020 06:45
@MylesBorins
Copy link
Contributor Author

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
Copy link
Member

targos commented May 7, 2020

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

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.

LGTM!

lib/internal/util.js Outdated Show resolved Hide resolved
@MylesBorins
Copy link
Contributor Author

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.

Copy link
Member

@addaleax addaleax left a comment

Choose a reason for hiding this comment

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

LGTM

@nodejs-github-bot
Copy link
Collaborator

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

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
Copy link
Collaborator

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.

LGTM

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

Choose a reason for hiding this comment

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

everybody to the limit

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe foo would be more appropriate here?

Copy link
Member

@ljharb ljharb May 8, 2020

Choose a reason for hiding this comment

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

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

lib/internal/modules/esm/module_job.js Outdated Show resolved Hide resolved
lib/internal/modules/esm/module_job.js Outdated Show resolved Hide resolved
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 <[email protected]>
@MylesBorins MylesBorins force-pushed the better-cjs-named-export-error branch from d0df6ab to 75f08f9 Compare May 8, 2020 15:42
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@MylesBorins
Copy link
Contributor Author

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

MylesBorins added a commit that referenced this pull request May 8, 2020
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 <[email protected]>

PR-URL: #33256
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
@MylesBorins
Copy link
Contributor Author

Landed in 2c3c9f5

@MylesBorins MylesBorins closed this May 8, 2020
codebytere pushed a commit that referenced this pull request May 11, 2020
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 <[email protected]>

PR-URL: #33256
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
@codebytere codebytere mentioned this pull request May 18, 2020
codebytere pushed a commit that referenced this pull request Jun 7, 2020
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 <[email protected]>

PR-URL: #33256
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
@codebytere codebytere mentioned this pull request Jun 9, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
esm Issues and PRs related to the ECMAScript Modules implementation.
Projects
None yet
Development

Successfully merging this pull request may close these issues.