module: improve named cjs import errors#35453

ctavan · 2020-10-01T20:54:38Z

When trying to import named exports from a CommonJS module an error is
thrown. Unfortunately the V8 error only contains the single line that
causes the error, it is therefore impossible to construct an equivalent
code consisting of default import + object descructuring assignment.

This was the reason why the example code was removed for multi line
import statements in #35275

To generate a helpful error messages for any case we can parse the file
where the error happens using acorn and construct a valid example code
from the parsed ImportDeclaration. This will work for any valid import
statement.

Since this code is only executed shortly before the node process crashes
anyways performance should not be a concern here.

Fixes: #35259
Refs: #35275

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
commit message follows commit guidelines

nodejs-github-bot · 2020-10-01T20:54:42Z

Review requested:

@nodejs/modules

ctavan · 2020-10-01T20:56:43Z

lib/internal/modules/esm/module_job.js

Is it OK to "lazy-load" inline here? My assumption is that this code is executed exactly once before the node process crashes. Is my assumption correct?
I saw a different way of lazy loading acorn in assert
node/lib/assert.js
Lines 216 to 218 in 4a6005c
// Lazy load acorn.
if(parseExpressionAt===undefined){
constacorn=require('internal/deps/acorn/acorn/dist/acorn');
.

ctavan · 2020-10-01T20:57:19Z

lib/internal/modules/esm/module_job.js

Is it OK to readFileSync?

That code would run only if the process is crashing, right? I think it's perfectly fine to use readFileSync for that kind of things.

This is precisely the question: I don't understand 100% yet of this error will always lead to termination of the node process. If so, I also think that readFileSync is the way to go.

Unless I find a way to parse the input file in a streaming fashion… Since imports typically come in the beginning of a file this would allow us to reduce the parsing work to a minimum… But maybe that's all unnecessary micro-optimizations for the crash case.

This has been benchmarked already, see nodejs/cjs-module-lexer#4 (comment). The gist of it is that streaming is not worth it for most JS files.

Thanks for the context!
Great, so the one possible optimization would be, having read the entire file, around not parsing the entire file with with acorn, see #35453 (comment)

ctavan · 2020-10-01T20:58:04Z

lib/internal/modules/esm/module_job.js

Is there any way around parsing the full file? I tried working with parseExpressionAt to only part the first few lines of a file but didn't have success (received unexpected token errors resulting from the import statements).

lib/internal/modules/esm/module_job.js

ctavan · 2020-10-01T21:04:21Z

test/fixtures/es-modules/package-cjs-named-error/default-and-renamed-import.mjs

For reasons I don't understand this import statement only errors out on line 3 (everybody). I do not understand why it doesn't error out on the renamed import statement. If I swap the order it will error on line 2 (still everybody).
Any clues why this is the case?

test/fixtures/es-modules/package-cjs-named-error/default-and-renamed-import.mjs

lib/internal/modules/esm/module_job.js

ctavan · 2020-10-01T21:19:01Z

lib/internal/modules/esm/module_job.js

My line splitting logic is pretty "simplistic"… Any idea on how to do this better?

I wonder if we do anything similar elsewhere? a nice to have would be checking the terminal width, rather than assuming 80.

codecov-commenter · 2020-10-02T03:14:17Z

Codecov Report

Merging #35453 into master will not change coverage.
The diff coverage is n/a.

@@ Coverage Diff @@## master #35453 +/- ## ======================================= Coverage 96.84% 96.84% ======================================= Files 212 212 Lines 69609 69609 ======================================= Hits 67410 67410 Misses 2199 2199

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4a6005c...6c22d54. Read the comment docs.

mcollina

+1 on the idea

ctavan · 2020-10-16T08:37:08Z

@MylesBorins @mcollina @Trott is there any interest in landing this change? I'm happy to work on it further.

lib/internal/modules/esm/module_job.js

aduh95 · 2020-10-16T09:14:35Z

@guybedford Does #35249 change something for this PR?

mcollina · 2020-10-16T10:03:29Z

Can you please make example of the error message before and after this PR?

nodejs-github-bot · 2020-10-16T10:24:55Z

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

ctavan · 2020-10-16T11:30:23Z

Can you please make example of the error message before and after this PR?

Sure. Upon encountering a multi-line named import statement like:

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

Before:

file:///***/node/test/fixtures/es-modules/package-cjs-named-error/multi-line.mjs:2 comeOn, ^^^^^^ SyntaxError: Named export 'comeOn' not found. The requested module './fail.cjs' is a CommonJS module, which may not support all module.exports as named exports. CommonJS modules can always be imported via the default export, for example using: import pkg from './fail.cjs'; at ModuleJob._instantiate (internal/modules/esm/module_job.js:98:21) at async ModuleJob.run (internal/modules/esm/module_job.js:143:5) at async Loader.import (internal/modules/esm/loader.js:165:24) at async Object.loadESM (internal/process/esm_loader.js:68:5)

After:

file:///***/node/test/fixtures/es-modules/package-cjs-named-error/multi-line.mjs:2 comeOn, ^^^^^^ SyntaxError: Named export 'comeOn' not found. The requested module './fail.cjs' is a CommonJS module, which may not support all module.exports as named exports. CommonJS modules can always be imported via the default export, for example using: import pkg from './fail.cjs'; const{comeOn, everybody } = pkg; at ModuleJob._instantiate (internal/modules/esm/module_job.js:164:21) at async ModuleJob.run (internal/modules/esm/module_job.js:205:5) at async Loader.import (internal/modules/esm/loader.js:165:24) at async Object.loadESM (internal/process/esm_loader.js:68:5)

Diff:

--- before-short 2020-10-16 12:54:45.000000000 +0200+++ after-short 2020-10-16 12:55:05.000000000 +0200@@ -5,8 +5,9 @@ CommonJS modules can always be imported via the default export, for example using: import pkg from './fail.cjs'; +const{comeOn, everybody } = pkg;- at ModuleJob._instantiate (internal/modules/esm/module_job.js:98:21)- at async ModuleJob.run (internal/modules/esm/module_job.js:143:5)+ at ModuleJob._instantiate (internal/modules/esm/module_job.js:164:21)+ at async ModuleJob.run (internal/modules/esm/module_job.js:205:5) at async Loader.import (internal/modules/esm/loader.js:165:24) at async Object.loadESM (internal/process/esm_loader.js:68:5)

For a single-line statement like:

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

nothing changes, it's the following before and after this patch:

file:///***/node/test/fixtures/es-modules/package-cjs-named-error/single-quote.mjs:1 import{comeOn } from './fail.cjs'; ^^^^^^ SyntaxError: Named export 'comeOn' not found. The requested module './fail.cjs' is a CommonJS module, which may not support all module.exports as named exports. CommonJS modules can always be imported via the default export, for example using: import pkg from './fail.cjs'; const{comeOn } = pkg; at ModuleJob._instantiate (internal/modules/esm/module_job.js:98:21) at async ModuleJob.run (internal/modules/esm/module_job.js:143:5) at async Loader.import (internal/modules/esm/loader.js:165:24) at async Object.loadESM (internal/process/esm_loader.js:68:5)

Also with this patch, very long import statements that don't fit into an 80 character line are split like this:

file:///***/node/test/fixtures/es-modules/package-cjs-named-error/long-multi-line.mjs:4 one, ^^^ SyntaxError: Named export 'one' not found. The requested module './fail.cjs' is a CommonJS module, which may not support all module.exports as named exports. CommonJS modules can always be imported via the default export, for example using: import pkg from './fail.cjs'; const{comeOn, one, two, three, four, five, six, seven, eight, nine, ten } = pkg; at ModuleJob._instantiate (internal/modules/esm/module_job.js:164:21) at async ModuleJob.run (internal/modules/esm/module_job.js:205:5) at async Loader.import (internal/modules/esm/loader.js:165:24) at async Object.loadESM (internal/process/esm_loader.js:68:5)

aduh95 · 2020-10-16T11:35:13Z

lib/internal/modules/esm/module_job.js

What if findNodeAt returns undefined?

Can you imagine a test case that would reproduce this?

I'm not sure, but I assume it may happen because it's in your while condition.
Have you tried to use findNodeAround instead of findNodeAt? It seems to fit better our use case.
Suggested change
node=findNodeAt(parsed,start);
node=findNodeAround(parsed,lineNumber,'ImportDeclaration');

mcollina

lgtm

aduh95

There are still some unresolved conversations on this thread that I'd like to be addressed before landing.

ctavan · 2020-10-16T12:26:19Z

The most serious issue that I must understand before landing is: #35453 (comment)

The same happens with the long-multiline test that I just added: Node throws on the second import (one) in that case instead of the first import (comeOn).

How is that possible?

guybedford

This gives a better message for users, so it seems good to me.

Approval conditional on the following:

If v8 changes the error output for some class of cases we don't notice, can this code ever error itself? It is very important that these decorations can never break so the JS must be written incredibly defensively. Eg what if the JS doesn't parse? What if it is a test framework and there was no JS at that location? What if the v8 output changes slightly for some case? What if it uses language features Acorn doesn't support. It otherwise gives the user a far far worse experience.
As this is the first time Acorn is being used in core proper (and not just REPL), it would be good to have some ok from others involved in this integration previously this is a path we want to go down for the project as it sets precedent.

ctavan · 2020-10-20T09:29:13Z

Thank you @guybedford for the input. I will put some more thought and effort into the parsing code to increase its defensiveness.

One of the things that immediately came to my mind: I'm currently using the line-number from the V8 error to determine the offending import statement.

@bcoe I recall you have added sourcemap support to Node.js and I believe errors are also rewritten when sourcemaps are in use. Could this interfere with this PR? Would be great if you could shed some light on this.

bcoe · 2020-10-21T15:27:01Z

@bcoe I recall you have added sourcemap support to Node.js and I believe errors are also rewritten when sourcemaps are in use. Could this interfere with this PR? Would be great if you could shed some light on this.

@ctavan where this might break with source maps would be the extra line of context Node.js adds to stack traces:

import{foo, bar} from 'blerg' ^

Perhaps we could add an additional test, with a transpiled file that has a multi-line import, and make sure it works? I find these a good opportunity to add a regression test for an additional transpiler; perhaps we could use ncc or rollup, if we don't have a fixture for either yet.

bcoe · 2020-12-01T15:35:15Z

@ctavan 👋 let me know when you'd like to dust this off, would be happy to pair.

ctavan · 2020-12-10T14:31:04Z

@bcoe thanks for chiming in and for offering your help.

Now that #35249 landed and named exports of CJS modules are pretty well supported, do we expect this error to be still relevant?

I searched open issues for the error message and – apart from my own issue #35259 – only found one issue that pre-date the named CJS import support which landed in v14.13.0/v12.20.0: #32137 (comment)

@guybedford could you summarize, under which conditions the error that this PR tries to improve would still be triggered?

ctavan · 2021-01-25T14:15:02Z

@guybedford could you summarize, under which conditions the error that this PR tries to improve would still be triggered?

@guybedford sorry for the ping. Can you answer my question off the top of your head? My assumption is that we can just close this PR and the corresponding issue, but getting confirmation from your end would be nice.

guybedford · 2021-01-29T02:36:44Z

@ctavan with cjs-module-lexer we now have detectable CJS exports being handled ok, so that this message only applies to cases where that detection fails. The rebase would still be useful to users in those specific scenarios I think. Entirely up to you if you want to pick this up again.

When trying to import named exports from a CommonJS module an error is thrown. Unfortunately the V8 error only contains the single line that causes the error, it is therefore impossible to construct an equivalent code consisting of default import + object descructuring assignment. This was the reason why the example code was removed for multi line import statements in nodejs#35275 To generate a helpful error messages for any case we can parse the file where the error happens using acorn and construct a valid example code from the parsed ImportDeclaration. This will work for _any_ valid import statement. Since this code is only executed shortly before the node process crashes anyways performance should not be a concern here. Fixes: nodejs#35259 Refs: nodejs#35275

Co-authored-by: Antoine du Hamel <[email protected]>

This reverts commit 22f700c.

ctavan · 2021-05-15T12:20:14Z

@guybedford since my priorities have shifted a little bit recently I doubt that I'll find the time to complete this PR any time soon.

I'll therefore close it and leave it to other folks to pick it up again in case there's interest.

guybedford · 2021-05-15T17:27:16Z

@ctavan thanks for the update. If you do find time again in future and come across any areas we can improve in future your contributions would always be appreciated.

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

ctavan commented Oct 1, 2020
View reviewed changes

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

ctavan commented Oct 1, 2020
View reviewed changes

test/fixtures/es-modules/package-cjs-named-error/default-and-renamed-import.mjs Outdated Show resolvedHide resolved

aduh95 reviewed Oct 1, 2020
View reviewed changes

ctavan force-pushed the improve-named-import-of-cjs-error branch from f0498e2 to 6c22d54Compare October 1, 2020 21:17

ctavan commented Oct 1, 2020
View reviewed changes

ctavan mentioned this pull request Oct 1, 2020
module: improve error decoration for cjs named exports for multi-line import statements #35259
Closed

mcollina reviewed Oct 2, 2020
View reviewed changes

ctavan force-pushed the improve-named-import-of-cjs-error branch from 68f2978 to e7a0b5cCompare October 2, 2020 10:29

aduh95 reviewed Oct 16, 2020
View reviewed changes

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

aduh95 reviewed Oct 16, 2020
View reviewed changes

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

mcollina requested a review from guybedford October 16, 2020 10:02

mcollina added the request-ci Add this label to start a Jenkins CI on a PR. label Oct 16, 2020

github-actionsbot removed the request-ci Add this label to start a Jenkins CI on a PR. label Oct 16, 2020

aduh95 reviewed Oct 16, 2020
View reviewed changes

mcollina approved these changes Oct 16, 2020
View reviewed changes

aduh95 requested changes Oct 16, 2020
View reviewed changes

ctavan force-pushed the improve-named-import-of-cjs-error branch from 809c06a to 36db7dfCompare October 16, 2020 12:23

guybedford approved these changes Oct 16, 2020
View reviewed changes

ctavanand others added 11 commits January 29, 2021 22:18

Update lib/internal/modules/esm/module_job.js
e4a018a
Co-authored-by: Antoine du Hamel <[email protected]>

Update lib/internal/modules/esm/module_job.js
75c84fd
Co-authored-by: Antoine du Hamel <[email protected]>

Apply suggestions from code review
757d643
Co-authored-by: Antoine du Hamel <[email protected]>

Fix syntax errors introduced by commit suggestions
1fcb340

Apply suggestions from code review
368305c
Co-authored-by: Antoine du Hamel <[email protected]>

Add long multiline test with comment
0677559

Fail if import example cannot be extracted
ae0a8ee

fixup! Add long multiline test with comment
4d11c44

Satisfy eslint
182245f

Revert "module: improve named cjs import errors"
533560f
This reverts commit 22f700c.

ctavan force-pushed the improve-named-import-of-cjs-error branch from c7931bd to 533560fCompare January 29, 2021 23:29

guybedford force-pushed the master branch from dc5a5da to 8e46568Compare March 29, 2021 21:33

ctavan closed this May 15, 2021

	// Lazy load acorn.
	if(parseExpressionAt===undefined){
	constacorn=require('internal/deps/acorn/acorn/dist/acorn');

	node=findNodeAt(parsed,start);
	node=findNodeAround(parsed,lineNumber,'ImportDeclaration');

Uh oh!

module: improve named cjs import errors#35453

module: improve named cjs import errors #35453

Uh oh!

Conversation

ctavan commented Oct 1, 2020

Checklist

Uh oh!

nodejs-github-bot commented Oct 1, 2020• edited by RaisinTen Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov-commenter commented Oct 2, 2020

Codecov Report

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

ctavan commented Oct 16, 2020

Uh oh!

Uh oh!

Uh oh!

aduh95 commented Oct 16, 2020

Uh oh!

mcollina commented Oct 16, 2020

Uh oh!

nodejs-github-bot commented Oct 16, 2020

Uh oh!

ctavan commented Oct 16, 2020• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aduh95Oct 16, 2020• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

aduh95 left a comment

Choose a reason for hiding this comment

Uh oh!

ctavan commented Oct 16, 2020

Uh oh!

guybedford left a comment

Choose a reason for hiding this comment

Uh oh!

ctavan commented Oct 20, 2020

Uh oh!

nodejs-github-bot commented Oct 1, 2020•
edited by RaisinTen
Loading

ctavan commented Oct 16, 2020•
edited
Loading

aduh95Oct 16, 2020•
edited
Loading