module: fix crash on multiline named cjs imports#35275
Closed
+31 −8
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
nodejs-github-bot added the esm 
Collaborator
nodejs-github-bot commented Sep 20, 2020
Review requested:
|
addaleax approved these changes Sep 20, 2020
addaleax added author ready 
github-actionsbot removed the request-ci Collaborator
nodejs-github-bot commented Sep 20, 2020
MylesBorins approved these changes Sep 20, 2020
Contributor
MylesBorins left a comment
MylesBorins left a comment There was a problem hiding this comment.
LGTM
tiny nit with the test to keep with the homestar runner theme, no need to land it 🤣
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
The node process crashes when trying to parse a multiline import statement for named exports of a CommonJS module: 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), The reason is that the regexp that is currently used to decorate the original error fails for multi line import statements. Unfortunately the undecorated error stack only contains the single line which causes the import to fail: 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) Hence, for multiline import statements we cannot create an equivalent piece of code that uses default import followed by an object destructuring assignment. In any case the node process should definitely not crash. So until we have a more sophisticated way of extracting the entire problematic multiline import statement, show the code example only for single-line imports where the current regexp approach works well. Refs: nodejs#35259
Co-authored-by: Myles Borins <[email protected]>
Co-authored-by: Myles Borins <[email protected]>
ctavanforce-pushed the fix-multiline-named-imports-of-cjs branch from e311be4 to af3b991Compare
Trott approved these changes Sep 22, 2020
Trott added request-ci github-actionsbot removed the request-ci Collaborator
nodejs-github-bot commented Sep 22, 2020
MylesBorins pushed a commit that referenced this pull request Sep 22, 2020
The node process crashes when trying to parse a multiline import statement for named exports of a CommonJS module: 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), The reason is that the regexp that is currently used to decorate the original error fails for multi line import statements. Unfortunately the undecorated error stack only contains the single line which causes the import to fail: 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) Hence, for multiline import statements we cannot create an equivalent piece of code that uses default import followed by an object destructuring assignment. In any case the node process should definitely not crash. So until we have a more sophisticated way of extracting the entire problematic multiline import statement, show the code example only for single-line imports where the current regexp approach works well. Refs: #35259 PR-URL: #35275 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Myles Borins <[email protected]> Reviewed-By: Rich Trott <[email protected]>
Contributor
MylesBorins commented Sep 22, 2020
Landed in 219e9fe |
MylesBorins pushed a commit that referenced this pull request Sep 24, 2020
The node process crashes when trying to parse a multiline import statement for named exports of a CommonJS module: 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), The reason is that the regexp that is currently used to decorate the original error fails for multi line import statements. Unfortunately the undecorated error stack only contains the single line which causes the import to fail: 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) Hence, for multiline import statements we cannot create an equivalent piece of code that uses default import followed by an object destructuring assignment. In any case the node process should definitely not crash. So until we have a more sophisticated way of extracting the entire problematic multiline import statement, show the code example only for single-line imports where the current regexp approach works well. Refs: #35259 PR-URL: #35275 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Myles Borins <[email protected]> Reviewed-By: Rich Trott <[email protected]>
guybedford pushed a commit to guybedford/node that referenced this pull request Sep 28, 2020
The node process crashes when trying to parse a multiline import statement for named exports of a CommonJS module: 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), The reason is that the regexp that is currently used to decorate the original error fails for multi line import statements. Unfortunately the undecorated error stack only contains the single line which causes the import to fail: 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) Hence, for multiline import statements we cannot create an equivalent piece of code that uses default import followed by an object destructuring assignment. In any case the node process should definitely not crash. So until we have a more sophisticated way of extracting the entire problematic multiline import statement, show the code example only for single-line imports where the current regexp approach works well. Refs: nodejs#35259 PR-URL: nodejs#35275 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Myles Borins <[email protected]> Reviewed-By: Rich Trott <[email protected]>
Merged
codebytere pushed a commit that referenced this pull request Oct 1, 2020
The node process crashes when trying to parse a multiline import statement for named exports of a CommonJS module: 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), The reason is that the regexp that is currently used to decorate the original error fails for multi line import statements. Unfortunately the undecorated error stack only contains the single line which causes the import to fail: 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) Hence, for multiline import statements we cannot create an equivalent piece of code that uses default import followed by an object destructuring assignment. In any case the node process should definitely not crash. So until we have a more sophisticated way of extracting the entire problematic multiline import statement, show the code example only for single-line imports where the current regexp approach works well. Refs: #35259 PR-URL: #35275 Backport-PR-URL: #35385 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Myles Borins <[email protected]> Reviewed-By: Rich Trott <[email protected]>
3 tasks
Merged
ctavan added a commit to ctavan/node that referenced this pull request Oct 16, 2020
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
joesepi pushed a commit to joesepi/node that referenced this pull request Jan 8, 2021
The node process crashes when trying to parse a multiline import statement for named exports of a CommonJS module: 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), The reason is that the regexp that is currently used to decorate the original error fails for multi line import statements. Unfortunately the undecorated error stack only contains the single line which causes the import to fail: 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) Hence, for multiline import statements we cannot create an equivalent piece of code that uses default import followed by an object destructuring assignment. In any case the node process should definitely not crash. So until we have a more sophisticated way of extracting the entire problematic multiline import statement, show the code example only for single-line imports where the current regexp approach works well. Refs: nodejs#35259 PR-URL: nodejs#35275 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Myles Borins <[email protected]> Reviewed-By: Rich Trott <[email protected]>
ctavan added a commit to ctavan/node that referenced this pull request Jan 29, 2021
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
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment
6 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The node process crashes when trying to parse a multiline import
statement for named exports of a CommonJS module:
The reason is that the regexp that is currently used to decorate the
original error fails for multi line import statements.
Unfortunately the undecorated error stack only contains the single line
which causes the import to fail:
Hence, for multiline import statements we cannot create an equivalent
piece of code that uses default import followed by an object
destructuring assignment.
In any case the node process should definitely not crash. So until we
have a more sophisticated way of extracting the entire problematic
multiline import statement, show the code example only for single-line
imports where the current regexp approach works well.
Refs: #35259
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passes