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.

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

repl: better handling of recoverable errors #18915

Closed
wants to merge 1 commit into from

Conversation

princejwesley
Copy link
Contributor

Better handling of recoverable errors in REPL module

Below syntax errors are handled without force .break/clear

  • Unexpected Token (prefix errors)
  • missing ) after argument list

In the multiline expression, recoverable errors are truly
recoverable, otherwise syntax error will be thrown.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines
Affected core subsystem(s)

repl

@nodejs-github-bot nodejs-github-bot added the repl Issues and PRs related to the REPL subsystem. label Feb 21, 2018
Copy link
Contributor

@Leko Leko left a comment

Choose a reason for hiding this comment

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

LGTM

@Leko
Copy link
Contributor

Leko commented Feb 24, 2018

@Leko
Copy link
Contributor

Leko commented Feb 25, 2018

CI failed but I think those errors are not related to this PR.

not ok 712 parallel/test-http-pause
  ---
  duration_ms: 0.213
  severity: fail
  stack: |-
  ...

https://ci.nodejs.org/job/node-test-commit-linux/nodes=debian8-64/16655/console

not ok 2002 sequential/test-fs-readfile-tostring-fail
  ---
  duration_ms: 8.50
  severity: crashed
  stack: |-
    oh no!
    exit code: CRASHED (Signal: 9)
  ...

https://ci.nodejs.org/job/node-test-commit-linux/nodes=centos7-64/16655/console

@Leko
Copy link
Contributor

Leko commented Feb 26, 2018

@0joshuaolson1
Copy link

0joshuaolson1 commented Feb 27, 2018

Is this the right place to add other syntax errors that the repl doesn't handle correctly? I can't find an issue for it.

Copy link
Member

@BridgeAR BridgeAR left a comment

Choose a reason for hiding this comment

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

LGTM. Just a nit and a question.

lib/repl.js Outdated
const pos = frames.findIndex((f) => f.match(/^\s*\^+$/));
if (pos > 0 && frames[pos - 1].length === frames[pos].length)
return true;
else return false;
Copy link
Member

Choose a reason for hiding this comment

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

Nit: please remove the else. This is a style that is normally not used here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

rebasing and amending to same commit

@@ -163,13 +163,23 @@ const errorTests = [
send: '.break',
expect: ''
},
// Template expressions can cross lines
// Template expressions
{
send: '`io.js ${"1.0"',
Copy link
Member

Choose a reason for hiding this comment

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

I guess recovering from e.g.

'`abc ${ test'

is not possible anymore?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, only some forms are not possible. For instance, below one is possible.

`abc ${
   test}`

Its mostly consistent with chrome console behaviour except for couple of cases.

  1. we handle 'missing ) after argument list' error better than chrome console.
  2. we allow repl commands in multiline mode which should otherwise be treated as properties/functions.
    princejwesley@c1796b8 (wip)
> (function() {
... x = { help: () => 'help received' };
... return x
...         .help ();    <-- no repl command parsing in multiline mode
.break    Sometimes you get stuck, this gets you out
.clear    Alias for .break
.editor   Enter editor mode
.exit     Exit the repl
.help     Print this help message
.load     Load JS from a file into the REPL session
.save     Save all evaluated commands in this REPL session to a file
...

(I'll give PR over the weekend)

@BridgeAR
Copy link
Member

BridgeAR commented Mar 2, 2018

@0joshuaolson1 please open a new issue in case you think the repl should recover from errors that you ran into.

@BridgeAR BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Mar 2, 2018
Below syntax errors are handled without force .break/clear
  - Unexpected Token (prefix errors)
  - missing ) after argument list

In the multiline expression, recoverable errors are truly
recoverable, otherwise syntax error will be thrown.
@BridgeAR
Copy link
Member

BridgeAR commented Mar 6, 2018

@BridgeAR
Copy link
Member

BridgeAR commented Mar 11, 2018

Landed in ebfa8b1 🎉

@BridgeAR BridgeAR closed this Mar 11, 2018
BridgeAR pushed a commit that referenced this pull request Mar 11, 2018
Below syntax errors are handled without force .break/clear
  - Unexpected Token (prefix errors)
  - missing ) after argument list

In the multiline expression, recoverable errors are truly
recoverable, otherwise syntax error will be thrown.

PR-URL: #18915
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Shingo Inoue <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
targos pushed a commit that referenced this pull request Mar 17, 2018
Below syntax errors are handled without force .break/clear
  - Unexpected Token (prefix errors)
  - missing ) after argument list

In the multiline expression, recoverable errors are truly
recoverable, otherwise syntax error will be thrown.

PR-URL: #18915
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Shingo Inoue <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
@targos targos mentioned this pull request Mar 18, 2018
MylesBorins pushed a commit that referenced this pull request Mar 20, 2018
Below syntax errors are handled without force .break/clear
  - Unexpected Token (prefix errors)
  - missing ) after argument list

In the multiline expression, recoverable errors are truly
recoverable, otherwise syntax error will be thrown.

PR-URL: #18915
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Shingo Inoue <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
MayaLekova pushed a commit to MayaLekova/node that referenced this pull request May 8, 2018
Below syntax errors are handled without force .break/clear
  - Unexpected Token (prefix errors)
  - missing ) after argument list

In the multiline expression, recoverable errors are truly
recoverable, otherwise syntax error will be thrown.

PR-URL: nodejs#18915
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Shingo Inoue <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
jasnell pushed a commit to jasnell/node that referenced this pull request Aug 17, 2018
Below syntax errors are handled without force .break/clear
  - Unexpected Token (prefix errors)
  - missing ) after argument list

In the multiline expression, recoverable errors are truly
recoverable, otherwise syntax error will be thrown.

PR-URL: nodejs#18915
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Shingo Inoue <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
MylesBorins pushed a commit that referenced this pull request Sep 6, 2018
Below syntax errors are handled without force .break/clear
  - Unexpected Token (prefix errors)
  - missing ) after argument list

In the multiline expression, recoverable errors are truly
recoverable, otherwise syntax error will be thrown.

Backport-PR-URL: #22380
PR-URL: #18915
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Shingo Inoue <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Sep 6, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. repl Issues and PRs related to the REPL subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants