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

[ctavan]

• Version: v14.11.0
• Platform: Darwin * 19.6.0 Darwin Kernel Version 19.6.0: Thu Jun 18 20:49:00 PDT 2020; root:xnu-6153.141.1~1/RELEASE_X86_64 x86_64
• Subsystem: modules

What steps will reproduce the bug?

Failing test can be found here: https://github.com/ctavan/node/commit/ba9e73414eb5181873332da917defc25d1034afa

How often does it reproduce? Is there a required condition?

Always.

What is the expected behavior?

When trying to import named exports from a CommonJS module, there's usually a helpful error message which is assembled here: https://github.com/nodejs/node/blob/ed8af4e93831d3cf21d5562e900371d796b5fa20/lib/internal/modules/esm/module_job.js#L102-L124

What do you see instead?

When using multi-line import statements like:

import {
  comeOn,
  rightNow,
} from './fail.cjs';

the following regex does not match:

https://github.com/nodejs/node/blob/ed8af4e93831d3cf21d5562e900371d796b5fa20/lib/internal/modules/esm/module_job.js#L112

and the following error is produced:

TypeError: Cannot read property '0' of null
      at ModuleJob._instantiate (internal/modules/esm/module_job.js:112:77)
      at async ModuleJob.run (internal/modules/esm/module_job.js:137:5)
      at async Loader.import (internal/modules/esm/loader.js:165:24)
      at async rejects.name (file:///***/node/test/es-module/test-esm-cjs-named-error.mjs:56:3)
      at async waitForActual (assert.js:721:5)
      at async rejects (assert.js:830:25),

Additional information

I would love to contribute a fix for this issue, however I need some guidance on how to proceed. The problem I see is that the full error stack only contains the first line of the multi-line import statement:

[
  'file:///***/node/test/fixtures/es-modules/package-cjs-named-error/multi-line.mjs:2',
  '  comeOn,',
  '  ^^^^^^',
  "SyntaxError: The requested module './fail.cjs' does not provide an export named 'comeOn'",
  '    at ModuleJob._instantiate (internal/modules/esm/module_job.js:98:21)',
  '    at async ModuleJob.run (internal/modules/esm/module_job.js:141:5)',
  '    at async Loader.import (internal/modules/esm/loader.js:165:24)',
  '    at async rejects.name (file:///***/node/test/es-module/test-esm-cjs-named-error.mjs:56:3)',
  '    at async waitForActual (assert.js:721:5)',
  '    at async rejects (assert.js:830:25)'
]

So while the goal of the additional error decoration which was added in #33256 seems to be to provide a copy & pastable solution, I don't see how this could be achieved with the error information at hand when the error comes from a multi-line import statement.

Options that come to my mind:

• Just resort to the original error?
• Skip the example if it cannot be generated from the error stack, see: https://github.com/ctavan/node/commit/248eadadaf218088624f4c237a29befcc9dae66c
• Produce something like the following, even though it's incomplete:
  js import pkg from './fail.cjs'; const { comeOn } = pkg;
• Produce the correct example, which would probably involve using an actual parser to read the full failing import statement.

/cc @MylesBorins

[ljharb]

cc @nodejs/modules-active-members

[MylesBorins]

This is definitely less than ideal. I like the solution in https://github.com/ctavan/node/commit/248eadadaf218088624f4c237a29befcc9dae66c as a place to start. We can definitely improve the regex and support multi-line in the future.

[ctavan]

Great, I‘ll provide a PR tomorrow.

[MylesBorins]

I've changed the title of this issue to track having to improve the existing message which is not giving the hint for a multiline import

[ctavan]

@MylesBorins if you have a rough idea of how to extract the multiline import in that place then let me know and I'm happy to work on a patch.

[MylesBorins]

@ctavan honestly the best idea I can come up with is using acorn to parse the entire file, but that seems like it might be overkill

[ctavan]

@MylesBorins hmm, I was hoping for a simpler solution.

While digging through the error decoration code I realized that @bcoe you have touched this recently with the sourcemap patch (https://github.com/nodejs/node/commit/458677f5ef2). I didn't check the sourcemap implementation in detail, but maybe it also involves rewriting error messages that show the offending (mapped) lines of code? Any ideas from that perspective?

[GeoffreyBooth]

@ctavan honestly the best idea I can come up with is using acorn to parse the entire file, but that seems like it might be overkill

Is this really overkill? The process is exiting, so the few milliseconds it takes for Acorn to parse the file shouldn’t matter too much I’d think. And this would be a solid solution without the brittleness of a regex.

[ctavan]

@GeoffreyBooth I’ll explore this over the next few days to see how much complexity this will add.

[ctavan]

@GeoffreyBooth @MylesBorins @ljharb @guybedford I have prepared a pull request over at https://github.com/nodejs/node/pull/35453 which uses acorn to parse the offending file to always produce an equivalent code example.

Since I'm still new to the Node.js codebase I have a ton of questions w.r.t. my code which I have added as inline comments on the PR. Would be super happy to receive your feedback over there.