repl: refactor `LineParser` implementation by blakeembrey · Pull Request #6171 · nodejs/node · GitHub

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).

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.

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

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

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

@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.

@nodejs/collaborators can someone review?

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

@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.

Yep. Understood. I'll commit to reviewing.

I'll review it too.

@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) 😄

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

EDIT: CI is green

This has conflicts.

@blakeembrey could you rebase?

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

@cjihrig @gibfahn Updated again.

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 <jasnell@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com>

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

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

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 <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

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 <jasnell@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com>

This fixes a regression introduced in nodejs#6171 PR-URL: nodejs#11871 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

Delete unused method call. PR-URL: #12685 Refs: #6171 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

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 <jasnell@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com>

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 c7066fb Compare 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 44ba60a Compare June 28, 2016 01:16

Trott force-pushed the master branch from b0df363 to c5ce7f4 Compare 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 34a3f83 Compare October 14, 2016 19:14

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

MylesBorins force-pushed the master branch from 8df7ee0 to 54fef67 Compare 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 632c58e Compare 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 c06f9c3 Compare 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 6e05e03 Compare 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 <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

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

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

repl: refactor `LineParser` implementation by blakeembrey · Pull Request #6171 · nodejs/node · GitHub

Uh oh!

Uh oh!

repl: refactor LineParser implementation - #6171

repl: refactor LineParser implementation#6171blakeembrey wants to merge 1 commit intonodejs:masternodejs/node:masterfrom blakeembrey:repl-custom-evalblakeembrey/node:repl-custom-evalCopy head branch name to clipboard

Conversation

blakeembrey commented Apr 12, 2016

Checklist

Affected core subsystem(s)

Description of change

Uh oh!

Fishrock123 Jun 17, 2016

Choose a reason for hiding this comment

Uh oh!

blakeembrey Jun 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!

hybrist Mar 15, 2017

Choose a reason for hiding this comment

Uh oh!

jasnell Mar 15, 2017

Choose a reason for hiding this comment

Uh oh!

jasnell Mar 15, 2017

Choose a reason for hiding this comment

Uh oh!

hybrist Mar 15, 2017

Choose a reason for hiding this comment

Uh oh!

hybrist Mar 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
blakeembrey wants to merge 1 commit into
nodejs:masterfrom
blakeembrey:repl-custom-eval

blakeembrey Jun 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