Skip to content

repl: improve static import error message in repl - #33588

Closed
MylesBorins wants to merge 1 commit into
nodejs:masterfrom
MylesBorins:better-repl-error
Closed

repl: improve static import error message in repl#33588
MylesBorins wants to merge 1 commit into
nodejs:masterfrom
MylesBorins:better-repl-error

Conversation

@MylesBorins

@MylesBorinsMylesBorins commented May 27, 2020

Copy link
Copy Markdown
Contributor

Currently the error is rather ambiguous and does not inform folks that
static import is not supported in the repl. This overrides the default
error message with one that is more informative.

Closes: #33576

@nodejs-github-botnodejs-github-bot added the repl Issues and PRs related to the REPL subsystem. label May 27, 2020
@nodejs-github-bot

nodejs-github-bot commented May 27, 2020

Copy link
Copy Markdown
Collaborator

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

@MylesBorinsMylesBorins added the esm Issues and PRs related to the ECMAScript Modules implementation. label May 27, 2020
@MylesBorins
MylesBorins requested a review from BridgeARMay 27, 2020 16:32
BridgeAR
BridgeAR reviewed May 27, 2020
View reviewed changes

@BridgeARBridgeAR left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The logic should probably be moved here:

node/lib/repl.js

Line 567 in 54b36e4

// Remove stack trace.

Comment threadlib/repl.js Outdated
Comment threadlib/repl.js Outdated
Comment threadlib/repl.js Outdated
@BridgeAR
BridgeARforce-pushed the master branch 2 times, most recently from 8ae28ff to 2935f72CompareMay 31, 2020 12:18
@MylesBorins

MylesBorins commented Jul 20, 2020

Copy link
Copy Markdown
ContributorAuthor

I've updated PTAL

@BridgeAR I don't think we need to check the length on this. If someone is explicitly messing with the stack size I'm pretty happy that this feature might fail for them. Thoughts?

MylesBorins
MylesBorins commented Jul 20, 2020
View reviewed changes
Comment threadlib/repl.js Outdated

@MylesBorinsMylesBorinsJul 20, 2020

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

we could alternative find which item in the array starts with SyntaxError rather than by default using the 5th item

@BridgeARBridgeARJul 21, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

That sounds better to me. There is in fact similar code to determine that line below. It'll only be triggered in case there's no uncaughtException listener though.

@MylesBorinsMylesBorinsJul 22, 2020

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Updated to use regex PTAL

@BridgeAR

BridgeAR commented Jul 21, 2020

Copy link
Copy Markdown
Member

Is it not possible to just fix static import? That would be most user friendly and I would rather have that. I guess it might be tricky to implement it though?

@MylesBorins

MylesBorins commented Jul 21, 2020

Copy link
Copy Markdown
ContributorAuthor

@BridgeAR AFAIK there is no way to implement static import in the REPL. I could be wrong, but in lieu of a solution to that this seems like the next best thing.

@MylesBorins
repl: improve static import error message in repl
4a7fe9a
Currently the error is rather ambiguous and does not inform folks that
static import is not supported in the repl. This overrides the default
error message with one that is more informative.
Closes: nodejs#33576
@nodejs-github-bot

nodejs-github-bot commented Jul 23, 2020

Copy link
Copy Markdown
Collaborator

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

@MylesBorinsMylesBorins added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jul 25, 2020
@MylesBorins

MylesBorins commented Jul 30, 2020

Copy link
Copy Markdown
ContributorAuthor

hey @BridgeAR are you ok with this landing?

@MylesBorins

MylesBorins commented Aug 3, 2020

Copy link
Copy Markdown
ContributorAuthor

Pinging @BridgeAR one more time. I'll land in 48 hours if I don't hear anything.

MylesBorins added a commit that referenced this pull request Aug 5, 2020
@MylesBorins
repl: improve static import error message in repl
861a54c
Currently the error is rather ambiguous and does not inform folks that
static import is not supported in the repl. This overrides the default
error message with one that is more informative.
Closes: #33576
PR-URL: #33588Fixes: #33576
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Guy Bedford <guybedford@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
@MylesBorins

MylesBorins commented Aug 5, 2020

Copy link
Copy Markdown
ContributorAuthor

Landed in 861a54c

Trott
Trott reviewed Aug 6, 2020
View reviewed changes
Comment threadtest/parallel/test-repl.js
expect: '42'
},
{
send: 'import comeOn from \'fhqwhgads\'',

@TrottTrottAug 6, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
send: 'import comeOn from \'fhqwhgads\'',
send: 'import everybody from \'the limit\'',

@TrottTrottAug 6, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

On second thought...

Suggested change
send: 'import comeOn from \'fhqwhgads\'',
send: 'import theLimit from \'everybody\'',

@TrottTrottAug 6, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Long descriptive names are preferable.

Suggested change
send: 'import comeOn from \'fhqwhgads\'',
send: 'import comeOn from \'fhqwhgadshgnsdhjsdbkhsdabkfabkveybvf\'',

@MylesBorinsMylesBorinsAug 6, 2020

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

LOLOLOL

guess this will need to come in a followup

codebytere pushed a commit that referenced this pull request Aug 6, 2020
@MylesBorins@codebytere
repl: improve static import error message in repl
465c262
Currently the error is rather ambiguous and does not inform folks that
static import is not supported in the repl. This overrides the default
error message with one that is more informative.
Closes: #33576
PR-URL: #33588Fixes: #33576
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Guy Bedford <guybedford@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
@codebyterecodebytere mentioned this pull request Aug 10, 2020
Merged
codebytere pushed a commit that referenced this pull request Aug 11, 2020
@MylesBorins@codebytere
repl: improve static import error message in repl
938842e
Currently the error is rather ambiguous and does not inform folks that
static import is not supported in the repl. This overrides the default
error message with one that is more informative.
Closes: #33576
PR-URL: #33588Fixes: #33576
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Guy Bedford <guybedford@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
MylesBorins added a commit that referenced this pull request Nov 3, 2020
@MylesBorins
repl: improve static import error message in repl
e963c2c
Currently the error is rather ambiguous and does not inform folks that
static import is not supported in the repl. This overrides the default
error message with one that is more informative.
Closes: #33576
PR-URL: #33588Fixes: #33576
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Guy Bedford <guybedford@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
@MylesBorinsMylesBorins mentioned this pull request Nov 3, 2020
Merged
MylesBorins added a commit that referenced this pull request Nov 16, 2020
@MylesBorins
repl: improve static import error message in repl
18f01dd
Currently the error is rather ambiguous and does not inform folks that
static import is not supported in the repl. This overrides the default
error message with one that is more informative.
Closes: #33576
PR-URL: #33588Fixes: #33576
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Guy Bedford <guybedford@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@TrottTrottTrott left review comments
@jasnelljasnelljasnell approved these changes
@guybedfordguybedfordguybedford approved these changes
@BridgeARBridgeARBridgeAR approved these changes
+1 more reviewer
@GMartignyGMartignyGMartigny left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

author readyPRs that have at least one approval, no pending requests for changes, and a CI started.esmIssues and PRs related to the ECMAScript Modules implementation.replIssues and PRs related to the REPL subsystem.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Better error message for using static import in repl

7 participants

@MylesBorins@nodejs-github-bot@BridgeAR@jasnell@guybedford@Trott@GMartigny