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: ctavan@ba9e734

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:

node/lib/internal/modules/esm/module_job.js

Lines 102 to 124 in ed8af4e

	if (StringPrototypeIncludes(e.message,
	' does not provide an export named')) {
	const splitStack = StringPrototypeSplit(e.stack, '\n');
	const parentFileUrl = splitStack[0];
	const childSpecifier = StringPrototypeMatch(e.message, /module '(.*)' does/)[1];
	const childFileURL =
	await this.loader.resolve(childSpecifier, parentFileUrl);
	const format = await this.loader.getFormat(childFileURL);
	if (format === 'commonjs') {
	const importStatement = splitStack[1];
	const namedImports = StringPrototypeMatch(importStatement, /{.*}/)[0];
	const destructuringAssignment = StringPrototypeReplace(namedImports, /\s+as\s+/g, ': ');
	e.message = `The requested module '${childSpecifier}' is expected ` +
	'to be of type CommonJS, which does not support named exports. ' +
	'CommonJS modules can be imported by importing the default ' +
	'export.\n' +
	'For example:\n' +
	`import pkg from '${childSpecifier}';\n` +
	`const ${destructuringAssignment} = pkg;`;
	const newStack = StringPrototypeSplit(e.stack, '\n');
	newStack[3] = `SyntaxError: ${e.message}`;
	e.stack = ArrayPrototypeJoin(newStack, '\n');
	}

What do you see instead?

When using multi-line import statements like:

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

the following regex does not match:

node/lib/internal/modules/esm/module_job.js

Line 112 in ed8af4e

const namedImports = StringPrototypeMatch(importStatement, /{.*}/)[0];

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: ctavan@248eada
• Produce something like the following, even though it's incomplete:

    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 ctavan@248eada 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 (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 #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.

[github-actions]

There has been no activity on this feature request for 5 months and it is unlikely to be implemented. It will be closed 6 months after the last non-automated comment.

For more information on how the project manages feature requests, please consult the feature request management document.

[github-actions]

There has been no activity on this feature request and it is being closed. If you feel closing this issue is not the right thing to do, please leave a comment.

For more information on how the project manages feature requests, please consult the feature request management document.