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: exports Recoverable #3488

Closed

Conversation

blakeembrey
Copy link
Contributor

Allow REPL consumers to callback with a Recoverable error instance and trigger multi-line REPL prompts.

Closes #2939

@mscdex mscdex added the repl Issues and PRs related to the REPL subsystem. label Oct 22, 2015
@Fishrock123 Fishrock123 added the semver-minor PRs that contain new features and should be released in the next minor version. label Oct 22, 2015
@jasnell
Copy link
Member

jasnell commented Oct 22, 2015

No opinion yet on the actual change, but a test case and doc+example would be helpful on this kind of change

@bnoordhuis
Copy link
Member

#2939 says it's so require('repl') users can do multi-line completion more easily. I can get behind that but I'm not sure if exporting Recoverable is the best possible way to do it, that's mostly an artifact of the current implementation.

@blakeembrey
Copy link
Contributor Author

@bnoordhuis Do you have any other thoughts on it? I thought it was actually a pretty clean solution to give control to the consumer. The alternative of using a function option to check [the eval error] wouldn't be ideal for places where the code is being statically analyzed, like TypeScript.

@jasnell Sure, I can add some docs/example - do you have an existing or similar test I can use for reference here? I didn't see an existing multi-line test (I'm sure I probably missed it this morning).

@Trott
Copy link
Member

Trott commented Oct 23, 2015

If I'm understanding "multi-line test" correctly (a test for the REPL that involves sending some stuff and then a new line with some more stuff), there are indeed ones you can use as models. One is in test/parallel/test-repl-tab-completion-crash.js. It sends this to the REPL:

.clear
function () {
   arguments.\t

(Node 4.2.1 blows up at the \t tab completion, but current master branch sends a ReferenceError which is logical because arguments is undefined until the function is invoked so the tab is asking for tab completion for properties on an undefined object.)

It in turn was modeled on the more general test/parallel/test-repl-tab-complete.js which has many examples of tests that involve sending multiple lines to the REPL.

(And if I'm misunderstanding your question, sorry for the noise.)

@blakeembrey
Copy link
Contributor Author

@Trott Thanks for the link, just wrote a similar test 😄

@blakeembrey
Copy link
Contributor Author

@jasnell Updated the repl.markdown documentation example.

@Fishrock123
Copy link
Contributor

The example makes sense, but I wonder if it isn't relying in implementation details too much?

@blakeembrey if you were to suggest an ideal API for this, what would it look like? Would it look any different?

@blakeembrey
Copy link
Contributor Author

@Fishrock123 I was thinking about it, and this is pretty ideal already. In fact, it's how I'd want to consume it - I'd filter recoverable errors from TypeScript diagnostics and then hand the execution back to repl if it's alright to recover. Anything else would most likely be more work to maintain and support. The closest API that otherwise makes sense is using a map and/or a third callback argument - then you'd use it like cb(null, null, { recoverable: true }). I can't imagine any future options yet though.

@ggentzke
Copy link

Commenting for attention as I've been encountering this issue recently and found my way here through nodejs/node-v0.x-archive#8640.

@Trott
Copy link
Member

Trott commented Nov 12, 2015

try {
result = vm.runInThisContext(cmd);
} catch (e) {
if (isRecoverableError(e)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Have we defined this anywhere in the docs?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, I can add it - it was only an example, but the actual implementation of isRecoverableError could be much complicated. Is there a psuedo-function I should show here?

Copy link
Contributor

Choose a reason for hiding this comment

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

@blakeembrey Can we use the simplified form of the one actually used by REPL. May be something like

function isRecoverableError(e) {
  if (e && e.name === 'SyntaxError') {
    if (e.message === 'Missing } in template expression') {
      return true;
    }
    return /^(Unexpected end of input|Unexpected token)/.test(e.message);
  }
  return false;
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added your snippet to the PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

@blakeembrey To think of it, this part

    if (e.message === 'Missing } in template expression') {
      return true;
    }

may not be readily understandable for the users. So, lets remove that part alone.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So, lets remove that part alone.

What do you mean by this? Shouldn't this be removed all together, the following regexp test is pretty clear.

Copy link
Contributor

Choose a reason for hiding this comment

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

Only the if condition I mentioned above can be removed., So it will look like this

function isRecoverableError(e) {
  if (e && e.name === 'SyntaxError') {
    return /^(Unexpected end of input|Unexpected token)/.test(e.message);
  }
  return false;
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Already updated below. I'm only using error.name because it's assumed the try..catch would have an error object already to check.

Copy link
Contributor

Choose a reason for hiding this comment

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

Better now :-)

@blakeembrey blakeembrey force-pushed the repl-export-recoverable branch 2 times, most recently from e59ab0e to 159e3af Compare November 13, 2015 04:41
@thefourtheye
Copy link
Contributor

Change LGTM. If there is no other better option, then we can go ahead with this. @bnoordhuis thoughts?

@bnoordhuis
Copy link
Member

I still appreciate the spirit if not the actual implementation of this PR. :-)

I'm also not sure if it's correct in all cases. The actual check in lib/repl.js is:

if (e instanceof Recoverable && !self.lineParser.shouldFail) {

If .shouldFail is true, it's still going to fail even though the user may not want that.

@thefourtheye
Copy link
Contributor

Oh yeah. You are correct. Users cannot override comments check and string literal checks.

@anshumanf
Copy link

This would likely fix babel/babel#1741

@blakeembrey
Copy link
Contributor Author

@bnoordhuis You're right, I can move the line parser into the JavaScript eval implementation.

@ghost
Copy link

ghost commented Nov 13, 2015

Closely following this. If fixed this would help improve my workflow in babel. Thanks for the effort.

@blakeembrey
Copy link
Contributor Author

@bnoordhuis Now I see now why I missed it. It was added after I opened this PR. I'm going to refactor it into the eval implementation (edit: today or tomorrow).

Edit: 6cf1910

@blakeembrey
Copy link
Contributor Author

@bnoordhuis @thefourtheye Ok, it's a bit more complicated now since a bit of it is tightly coupled and other parts I'm not sure actually work as expected. I can still update this with the same changes, but things like the "saved RegExp population" - wouldn't that currently be failing when run in the non-global context?

Edit: Another bug. The "line parser" is trimming whitespace and not handling template strings, so any trailing whitespace is lost.

Edit 2: Another issue. Chaining in the CLI? Any lines that start with . and aren't numeric are treated as a REPL command. I'll leave that behaviour in tact.

@blakeembrey
Copy link
Contributor Author

@bnoordhuis @thefourtheye I made some refactors and improvements to the line parser code. It only runs on a syntax error now, but it's running within the context of eval so it's always checking all the code. I narrowed down the allowed unexpected tokens too, since I have commonly typed function () { and continued just to realize the typo and not be able to escape. It also fixes the multiline template string input.

Edit: If it makes sense now too, I'm open to making a new PR for this outside of this one and rebasing it back when it's merged.

@bnoordhuis
Copy link
Member

I'm going to decline reviewing this but e.g. @thefourtheye is welcome to pick it up.

@blakeembrey
Copy link
Contributor Author

@mscdex Thanks, fixed that. I didn't recall an option just to run linting.

@mscdex
Copy link
Contributor

mscdex commented May 7, 2016

@blakeembrey Ah yeah, make jslint to just lint js, make cpplint to lint just c++, and make lint to do both.

@blakeembrey
Copy link
Contributor Author

Cool, thanks. Should have seen it 😄 Anyway, passed locally - did a full test.

Fishrock123 pushed a commit to Fishrock123/node that referenced this pull request May 12, 2016
Allow REPL consumers to callback with a `Recoverable` error instance
and trigger multi-line REPL prompts.

Fixes: nodejs#2939
PR-URL: nodejs#3488
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Jeremiah Senkpiel <[email protected]>
@Fishrock123
Copy link
Contributor

Thanks! Landed in 9552e3b

@blakeembrey sorry for the extremely long delay on this.. 😓

@blakeembrey
Copy link
Contributor Author

Awesome! Thank you 😄

Next is #6171.

@blakeembrey blakeembrey deleted the repl-export-recoverable branch May 12, 2016 19:25
evanlucas pushed a commit that referenced this pull request May 17, 2016
Allow REPL consumers to callback with a `Recoverable` error instance
and trigger multi-line REPL prompts.

Fixes: #2939
PR-URL: #3488
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Jeremiah Senkpiel <[email protected]>
evanlucas added a commit that referenced this pull request May 17, 2016
- **buffer**: fix lastIndexOf and indexOf in various edge cases (Anna
  Henningsen) [#6511](#6511)
- **child_process**: use /system/bin/sh on android (Ben Noordhuis)
  [#6745](#6745)
- **deps**:
  - upgrade npm to 3.8.9 (Rebecca Turner)
    [#6664](#6664)
  - upgrade to V8 5.0.71.47 (Ali Ijaz Sheikh)
    [#6572](#6572)
  - upgrade libuv to 1.9.1 (Saúl Ibarra Corretgé)
    [#6796](#6796)
  - Intl: ICU 57 bump (Steven R. Loomis)
    [#6088](#6088)
- **repl**:
  - copying tabs shouldn't trigger completion (Eugene Obrezkov)
    [#5958](#5958)
  - exports `Recoverable` (Blake Embrey)
    [#3488](#3488)
- **src**: add O_NOATIME constant (Rich Trott)
  [#6492](#6492)
- **src,module**: add --preserve-symlinks command line flag (James M
  Snell) [#6537](#6537)
- **util**: adhere to `noDeprecation` set at runtime (Anna Henningsen)
  [#6683](#6683)
evanlucas added a commit that referenced this pull request May 17, 2016
- **buffer**: fix lastIndexOf and indexOf in various edge cases (Anna
  Henningsen) [#6511](#6511)
- **child_process**: use /system/bin/sh on android (Ben Noordhuis)
  [#6745](#6745)
- **deps**:
  - upgrade npm to 3.8.9 (Rebecca Turner)
    [#6664](#6664)
  - upgrade to V8 5.0.71.47 (Ali Ijaz Sheikh)
    [#6572](#6572)
  - upgrade libuv to 1.9.1 (Saúl Ibarra Corretgé)
    [#6796](#6796)
  - Intl: ICU 57 bump (Steven R. Loomis)
    [#6088](#6088)
- **repl**:
  - copying tabs shouldn't trigger completion (Eugene Obrezkov)
    [#5958](#5958)
  - exports `Recoverable` (Blake Embrey)
    [#3488](#3488)
- **src**: add O_NOATIME constant (Rich Trott)
  [#6492](#6492)
- **src,module**: add --preserve-symlinks command line flag (James M
  Snell) [#6537](#6537)
- **util**: adhere to `noDeprecation` set at runtime (Anna Henningsen)
  [#6683](#6683)

As of this release the 6.X line now includes 64-bit binaries for Linux
on Power Systems running in big endian mode in addition to the existing
64-bit binaries for running in little endian mode.

PR-URL: #6810
evanlucas added a commit that referenced this pull request May 17, 2016
- **buffer**: fix lastIndexOf and indexOf in various edge cases (Anna
  Henningsen) [#6511](#6511)
- **child_process**: use /system/bin/sh on android (Ben Noordhuis)
  [#6745](#6745)
- **deps**:
  - upgrade npm to 3.8.9 (Rebecca Turner)
    [#6664](#6664)
  - upgrade to V8 5.0.71.47 (Ali Ijaz Sheikh)
    [#6572](#6572)
  - upgrade libuv to 1.9.1 (Saúl Ibarra Corretgé)
    [#6796](#6796)
  - Intl: ICU 57 bump (Steven R. Loomis)
    [#6088](#6088)
- **repl**:
  - copying tabs shouldn't trigger completion (Eugene Obrezkov)
    [#5958](#5958)
  - exports `Recoverable` (Blake Embrey)
    [#3488](#3488)
- **src**: add O_NOATIME constant (Rich Trott)
  [#6492](#6492)
- **src,module**: add --preserve-symlinks command line flag (James M
  Snell) [#6537](#6537)
- **util**: adhere to `noDeprecation` set at runtime (Anna Henningsen)
  [#6683](#6683)

As of this release the 6.X line now includes 64-bit binaries for Linux
on Power Systems running in big endian mode in addition to the existing
64-bit binaries for running in little endian mode.

PR-URL: #6810
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
repl Issues and PRs related to the REPL subsystem. semver-minor PRs that contain new features and should be released in the next minor version.
Projects
None yet
Development

Successfully merging this pull request may close these issues.