Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

module: print nicer error message on syntax error #17281

Merged
merged 1 commit into from
Nov 29, 2017
Merged

module: print nicer error message on syntax error #17281

merged 1 commit into from
Nov 29, 2017

Conversation

bnoordhuis
Copy link
Member

@bnoordhuis bnoordhuis commented Nov 24, 2017 •
edited
Loading

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. module Issues and PRs related to the module subsystem. tools Issues and PRs related to the tools directory. labels Nov 24, 2017
bnoordhuis
bnoordhuis commented Nov 24, 2017
View reviewed changes
src/module_wrap.cc
CHECK(!try_catch.Message().IsEmpty());
CHECK(!try_catch.Exception().IsEmpty());
AppendExceptionLine(env, try_catch.Exception(), try_catch.Message(),
ErrorHandlingMode::MODULE_ERROR);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I could just use FATAL_ERROR here and undo the change to node_internals.h but I figured it might be nice to distinguish between "normal" fatal errors and ones from the esm module loader.

@bnoordhuis bnoordhuis mentioned this pull request Nov 24, 2017
Closed
@bnoordhuis
module: print better message on esm syntax error
597b3d1 
Include the offending line in the output and underline the bad token.

Before this commit, it printed "SyntaxError: Unexpected reserved word"
without indicating where the syntax error is.

Now it prints the line and underlines the offending token, like it does
for syntax errors in CJS scripts.

Minor changes are made to the test runner in order to support `*.mjs`
files in test/message.

Fixes: nodejs#17277
PR-URL: nodejs#17281
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
@bnoordhuis bnoordhuis closed this Nov 29, 2017
@bnoordhuis bnoordhuis deleted the fix17277 branch November 29, 2017 11:29
@bnoordhuis bnoordhuis merged commit 597b3d1 into nodejs:master Nov 29, 2017
MylesBorins pushed a commit that referenced this pull request Dec 12, 2017
@bnoordhuis @MylesBorins
module: print better message on esm syntax error
8002fbb 
Include the offending line in the output and underline the bad token.

Before this commit, it printed "SyntaxError: Unexpected reserved word"
without indicating where the syntax error is.

Now it prints the line and underlines the offending token, like it does
for syntax errors in CJS scripts.

Minor changes are made to the test runner in order to support `*.mjs`
files in test/message.

Fixes: #17277
PR-URL: #17281
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
MylesBorins pushed a commit that referenced this pull request Dec 12, 2017
@bnoordhuis @MylesBorins
module: print better message on esm syntax error
b719b77 
Include the offending line in the output and underline the bad token.

Before this commit, it printed "SyntaxError: Unexpected reserved word"
without indicating where the syntax error is.

Now it prints the line and underlines the offending token, like it does
for syntax errors in CJS scripts.

Minor changes are made to the test runner in order to support `*.mjs`
files in test/message.

Fixes: #17277
PR-URL: #17281
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
@MylesBorins MylesBorins mentioned this pull request Dec 12, 2017
Merged
@gibfahn
Copy link
Member

gibfahn commented Dec 19, 2017

Should this be backported to 8.x? If so could someone raise a backport PR, as this fails with:

[01:39|%  98|+ 2017|-   0]: release esm_display_syntax_error.mjs match failed  
line=5
expect=^\ \ \ \ at\ loaders\.set\ \(internal\/loader\/ModuleRequest\.js\:.*\:.*\)$
actual=    at ModuleJob.loaders.set [as moduleProvider] (internal/loader/ModuleRequest.js:33:13)
=== release esm_display_syntax_error.mjs ===                    
Path: message/esm_display_syntax_error.mjs
(node:64175) ExperimentalWarning: The ESM module loader is experimental.
file:///build/gib/node/test/message/esm_display_syntax_error.mjs:3
await async () => 0;
^^^^^
SyntaxError: Unexpected reserved word
    at ModuleJob.loaders.set [as moduleProvider] (internal/loader/ModuleRequest.js:33:13)
    at <anonymous>
Command: out/Release/node --experimental-modules /build/gib/node/test/message/esm_display_syntax_error.mjs
[01:39|%  98|+ 2017|-   1]: release esm_display_syntax_error_module.mjs match failed
line=5
expect=^\ \ \ \ at\ loaders\.set\ \(internal\/loader\/ModuleRequest\.js\:.*\:.*\)$
actual=    at ModuleJob.loaders.set [as moduleProvider] (internal/loader/ModuleRequest.js:33:13)
=== release esm_display_syntax_error_module.mjs ===                    
Path: message/esm_display_syntax_error_module.mjs
(node:64185) ExperimentalWarning: The ESM module loader is experimental.
file:///build/gib/node/test/fixtures/es-module-loaders/syntax-error.mjs:2
await async () => 0;
^^^^^
SyntaxError: Unexpected reserved word
    at ModuleJob.loaders.set [as moduleProvider] (internal/loader/ModuleRequest.js:33:13)
    at <anonymous>
Command: out/Release/node --experimental-modules /build/gib/node/test/message/esm_display_syntax_error_module.mjs

targos added a commit to targos/node that referenced this pull request Dec 20, 2017
@targos
module: print better message on esm import error
f4dac9d 
Use the same approach as a previous PR to include the offending line in
the output and underline imports of inexistent exports.

Fixes: nodejs#17785
Refs: nodejs#17281
@targos targos mentioned this pull request Dec 20, 2017
Closed
3 tasks
targos added a commit to targos/node that referenced this pull request Dec 24, 2017
@targos
module: print better message on esm import error
359819e 
Use the same approach as a previous PR to include the offending line in
the output and underline imports of inexistent exports.

PR-URL: nodejs#17786
Fixes: nodejs#17785
Refs: nodejs#17281
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Guy Bedford <guybedford@gmail.com>
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this pull request Jan 8, 2018
@targos @MylesBorins
module: print better message on esm import error
83afd13 
Use the same approach as a previous PR to include the offending line in
the output and underline imports of inexistent exports.

PR-URL: #17786
Fixes: #17785
Refs: #17281
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Guy Bedford <guybedford@gmail.com>
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this pull request Jan 9, 2018
@targos @MylesBorins
module: print better message on esm import error
d4d3ff4 
Use the same approach as a previous PR to include the offending line in
the output and underline imports of inexistent exports.

PR-URL: #17786
Fixes: #17785
Refs: #17281
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Guy Bedford <guybedford@gmail.com>
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this pull request Jan 9, 2018
@targos @MylesBorins
module: print better message on esm import error
3b2d8cb 
Use the same approach as a previous PR to include the offending line in
the output and underline imports of inexistent exports.

PR-URL: #17786
Fixes: #17785
Refs: #17281
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Guy Bedford <guybedford@gmail.com>
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@bnoordhuis bnoordhuis mentioned this pull request Jun 4, 2018
Closed
@jasnell
Copy link
Member

jasnell commented Aug 17, 2018

Not convinced that this needs to be backported to 8.x

@danfuzz danfuzz mentioned this pull request Dec 14, 2022
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@evanlucas evanlucas evanlucas approved these changes

@addaleax addaleax addaleax approved these changes

@targos targos targos approved these changes

@cjihrig cjihrig cjihrig approved these changes

Assignees
No one assigned
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. module Issues and PRs related to the module subsystem. tools Issues and PRs related to the tools directory.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
9 participants
@bnoordhuis @gibfahn @jasnell @evanlucas @addaleax @targos @cjihrig @MylesBorins @nodejs-github-bot