GeezerVison

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

@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

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;

@GeoffreyBooth … jinx ;)

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.

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.

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.

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

@MylesBorins thanks for this! much appreciated

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.

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

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.

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

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.

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

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

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

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

Landed in 2c3c9f5