repl: refactor `LineParser` implementation#6171

blakeembrey · 2016-04-12T19:50:49Z

Checklist

tests and code linting passes
the commit message follows commit guidelines

Affected core subsystem(s)

repl

Description of change

Move the core logic from LineParser should fail handling into the recoverable error check for the REPL default eval. This was originally from #3488, and I've split it out for separate review and hopefully speed it up. The idea is to make the current recoverable check part of the default eval instead of limiting the usefulness of the REPL functionality to only JavaScript languages (E.g. enable TypeScript).

Fishrock123 · 2016-06-17T20:56:27Z

lib/repl.js

Could have a semicolon after the }?

Absolutely. I didn't want to change too much code with this refactor though - I've only changed how it's run (E.g. to run after the user hits enter, instead of each line). Here's the original check: https://github.com/nodejs/node/pull/6171/files/680c88111413ad25bafbe9d7578079d6c0ca5ff0#diff-b13d72249263845d8e8341db0426f9d3L426. Could definitely change to check to include a semicolon.

Fishrock123 · 2016-06-17T21:00:04Z

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

Seems to me this changes enough properties that it is probably semver-major..

Fishrock123 · 2016-06-17T21:02:06Z

Looks like the CI is refusing to run with a rebase, CI without rebasing: https://ci.nodejs.org/job/node-test-pull-request/3019/

blakeembrey · 2016-10-14T19:21:46Z

@Fishrock123 I just rebased again on top of the changes that have occurred. Is there anything I can do to make this land? It'd be very convenient for myself and others that have built custom REPLs for transpiled programming languages.

I intend to use this to support running TypeScript in a REPL with multiple lines. Although not a big deal for TypeScript, since it follows JavaScript semantics, it's might be more useful for transpile language targets that may fail the existing "should code fail" checks used in the line parser. It also helps to separate the existing concerns that are mixed as new features get added to the JavaScript implementation that aren't relevant for transpiled languages.

Fishrock123 · 2016-10-14T19:37:43Z

@nodejs/collaborators can someone review?

jasnell · 2017-03-01T01:13:21Z

@blakeembrey ... do you still want to pursue this?

blakeembrey · 2017-03-01T03:53:52Z

@jasnell I will refactor again if someone will review it and thinks it's useful. Otherwise, not really. It is still required to improve interop with transpiler REPLs where recovery/new lines can be detected differently.

jasnell · 2017-03-01T04:15:14Z

Yep. Understood. I'll commit to reviewing.

targos · 2017-03-01T05:49:58Z

I'll review it too.

blakeembrey · 2017-03-03T06:21:22Z

@targos @jasnell I just updated the PR and it's passing locally. I'm not sure if the CI build is meant to be running?

Edit: I see I'm meant to wait (https://github.com/nodejs/node/blob/master/CONTRIBUTING.md#ci-testing) 😄

gibfahn · 2017-03-03T08:27:12Z

CI 3: https://ci.nodejs.org/job/node-test-commit/8236/

EDIT: CI is green

cjihrig · 2017-03-13T15:37:34Z

This has conflicts.

gibfahn · 2017-03-13T15:46:42Z

@blakeembrey could you rebase?

Move the core logic from `LineParser` should fail handling into the recoverable error check for the REPL default eval.

blakeembrey · 2017-03-13T20:20:45Z

@cjihrig @gibfahn Updated again.

targos · 2017-03-14T16:23:05Z

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

Move the core logic from `LineParser` should fail handling into the recoverable error check for the REPL default eval. PR-URL: #6171 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Michaël Zasso <[email protected]>

jasnell · 2017-03-14T22:09:06Z

Landed in 39d9afe. Thank you @blakeembrey. Sorry again for this taking so long!

blakeembrey · 2017-03-14T22:16:08Z

Thanks @jasnell for landing this, and everyone else involved 😄

hybrist · 2017-03-15T19:37:14Z

lib/repl.js

-return;
-}elseif(!self.bufferedCommand){
-self.outputStream.write('Invalid REPL keyword\n');
+if(trimmedCmd){


I think this broke the command line debugger. Previously an empty command would be sent to the eval function. Now <enter> is silently ignored.
The CLI debugger was using <enter> for "repeat last command".

Ugh. Ok, we can back it out if necessary but let's see if we can find a fix first.
/cc @blakeembrey

Let's also see if we can get a regression test added since this case obviously wasn't being tested in CI :-(

Don't think this is super urgent, was just digging through why it passed against the last 8.x nightly but not against master.

I think the fix is as simple as removing that else branch: #11871

This fixes a regression introduced in nodejs#6171

This fixes a regression introduced in #6171 PR-URL: #11871 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>

Move the core logic from `LineParser` should fail handling into the recoverable error check for the REPL default eval. PR-URL: nodejs#6171 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Michaël Zasso <[email protected]>

This fixes a regression introduced in nodejs#6171 PR-URL: nodejs#11871 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>

Delete unused method call. PR-URL: #12685 Refs: #6171 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>

lance · 2017-10-04T18:04:40Z

Added backport-requested-v6.x since that will fix #15704. PR coming soon.

Move the core logic from `LineParser` should fail handling into the recoverable error check for the REPL default eval. Fixes: #15704 Backport-PR-URL: #15773 PR-URL: #6171 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Michaël Zasso <[email protected]>

mscdex added the repl Issues and PRs related to the REPL subsystem. label Apr 12, 2016

blakeembrey mentioned this pull request Apr 15, 2016
repl: exports Recoverable#3488
Closed

estliberitas force-pushed the master branch 2 times, most recently from 7da4fd4 to c7066fbCompare April 26, 2016 05:23

Fishrock123 reviewed Jun 17, 2016
View reviewed changes

Fishrock123 added the semver-major PRs that contain breaking changes and should be released in the next major version. label Jun 17, 2016

blakeembrey force-pushed the repl-custom-eval branch from 680c881 to 44ba60aCompare June 28, 2016 01:16

Trott force-pushed the master branch from b0df363 to c5ce7f4Compare September 21, 2016 00:09

blakeembrey mentioned this pull request Sep 26, 2016
Allow multi-line REPL prompts like node TypeStrong/ts-node#3
Closed

blakeembrey force-pushed the repl-custom-eval branch 2 times, most recently from bc6e18c to 34a3f83Compare October 14, 2016 19:14

rvagg force-pushed the master branch 2 times, most recently from c133999 to 83c7a88Compare October 18, 2016 17:01

MylesBorins force-pushed the master branch from 8df7ee0 to 54fef67Compare February 1, 2017 01:00

jasnell added the stalled Issues and PRs that are stalled. label Mar 1, 2017

targos self-assigned this Mar 1, 2017

blakeembrey force-pushed the repl-custom-eval branch from 34a3f83 to 632c58eCompare March 3, 2017 03:56

jasnell approved these changes Mar 3, 2017
View reviewed changes

blakeembrey force-pushed the repl-custom-eval branch from 632c58e to c06f9c3Compare March 13, 2017 20:15

repl: refactor LineParser implementation
6e05e03
Move the core logic from `LineParser` should fail handling into the recoverable error check for the REPL default eval.

blakeembrey force-pushed the repl-custom-eval branch from c06f9c3 to 6e05e03Compare March 13, 2017 20:16

targos approved these changes Mar 14, 2017
View reviewed changes

jasnell closed this Mar 14, 2017

blakeembrey deleted the repl-custom-eval branch March 14, 2017 22:10

hybrist reviewed Mar 15, 2017
View reviewed changes

hybrist pushed a commit to hybrist/node that referenced this pull request Mar 15, 2017
repl: Empty command should be sent to eval function
7683ef3
This fixes a regression introduced in nodejs#6171

hybrist mentioned this pull request Mar 15, 2017
repl: Empty command should be sent to eval function #11871
Closed
3 tasks

jasnell mentioned this pull request Apr 4, 2017
8.0.0 Release Proposal #12220
Closed

vsemozhetbyt mentioned this pull request Apr 27, 2017
doc: update example in repl.md #12685
Closed
2 tasks

vsemozhetbyt added a commit that referenced this pull request Apr 28, 2017
doc: update example in repl.md
d8f8096
Delete unused method call. PR-URL: #12685 Refs: #6171 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>

lance added the backport-requested-v6.x label Oct 4, 2017

lance mentioned this pull request Oct 4, 2017
[v6.x backport] repl: refactor LineParser implementation #15773
Closed
3 tasks

MylesBorins mentioned this pull request Nov 21, 2017
v6.12.1 proposal #17180
Merged

targos removed their assignment Oct 31, 2021

Uh oh!

repl: refactor LineParser implementation#6171

repl: refactor LineParser implementation #6171

Uh oh!

Conversation

blakeembrey commented Apr 12, 2016

Checklist

Affected core subsystem(s)

Description of change

Uh oh!

Fishrock123Jun 17, 2016

Choose a reason for hiding this comment

Uh oh!

blakeembreyJun 17, 2016• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Fishrock123 commented Jun 17, 2016

Uh oh!

Fishrock123 commented Jun 17, 2016

Uh oh!

blakeembrey commented Oct 14, 2016

Uh oh!

Fishrock123 commented Oct 14, 2016

Uh oh!

jasnell commented Mar 1, 2017

Uh oh!

blakeembrey commented Mar 1, 2017• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jasnell commented Mar 1, 2017

Uh oh!

targos commented Mar 1, 2017

Uh oh!

blakeembrey commented Mar 3, 2017• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gibfahn commented Mar 3, 2017• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cjihrig commented Mar 13, 2017

Uh oh!

gibfahn commented Mar 13, 2017

Uh oh!

blakeembrey commented Mar 13, 2017

Uh oh!

targos commented Mar 14, 2017

Uh oh!

jasnell commented Mar 14, 2017

Uh oh!

blakeembrey commented Mar 14, 2017

Uh oh!

hybristMar 15, 2017

Choose a reason for hiding this comment

Uh oh!

jasnellMar 15, 2017

Choose a reason for hiding this comment

Uh oh!

jasnellMar 15, 2017

Choose a reason for hiding this comment

Uh oh!

hybristMar 15, 2017

Choose a reason for hiding this comment

Uh oh!

hybristMar 15, 2017

Choose a reason for hiding this comment

Uh oh!

lance commented Oct 4, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

repl: refactor `LineParser` implementation#6171

repl: refactor `LineParser` implementation #6171

blakeembreyJun 17, 2016•
edited
Loading

blakeembrey commented Mar 1, 2017•
edited
Loading

blakeembrey commented Mar 3, 2017•
edited
Loading

gibfahn commented Mar 3, 2017•
edited
Loading