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: refactor `LineParser` implementation #6171

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
9 participants
@blakeembrey
Contributor

blakeembrey commented Apr 12, 2016

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

@mscdex mscdex added the repl label Apr 12, 2016

@Fishrock123

View changes

Show outdated Hide outdated lib/repl.js
var wrappedCmd = false;
var input = code;
if (/^\s*\{/.test(code) && /\}\s*$/.test(code)) {

This comment has been minimized.

@Fishrock123

Fishrock123 Jun 17, 2016

Member

Could have a semicolon after the }?

@Fishrock123

Fishrock123 Jun 17, 2016

Member

Could have a semicolon after the }?

This comment has been minimized.

@blakeembrey

blakeembrey Jun 17, 2016

Contributor

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.

@blakeembrey

blakeembrey Jun 17, 2016

Contributor

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

View changes

Show outdated Hide outdated lib/repl.js
// first, create the Script object to check the syntax
while (true) {
try {
if (!/^\s*$/.test(code) &&

This comment has been minimized.

@Fishrock123

Fishrock123 Jun 17, 2016

Member

Isn't this still needed?

@Fishrock123

Fishrock123 Jun 17, 2016

Member

Isn't this still needed?

This comment has been minimized.

@blakeembrey

blakeembrey Jun 17, 2016

Contributor

It's been a while, but IIRC correctly you can never get here with just whitespace because there's a check on the trimmed code before trying to eval. It made sense to move it there since it's applicable to all languages, not just the default eval.

@blakeembrey

blakeembrey Jun 17, 2016

Contributor

It's been a while, but IIRC correctly you can never get here with just whitespace because there's a check on the trimmed code before trying to eval. It made sense to move it there since it's applicable to all languages, not just the default eval.

@Fishrock123

This comment has been minimized.

Show comment
Hide comment
@Fishrock123

Fishrock123 Jun 17, 2016

Member

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

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

Member

Fishrock123 commented Jun 17, 2016

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

This comment has been minimized.

Show comment
Hide comment
@Fishrock123

Fishrock123 Jun 17, 2016

Member

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

Member

Fishrock123 commented Jun 17, 2016

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

This comment has been minimized.

Show comment
Hide comment
@blakeembrey

blakeembrey Oct 14, 2016

Contributor

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

Contributor

blakeembrey commented Oct 14, 2016

@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

This comment has been minimized.

Show comment
Hide comment
@Fishrock123

Fishrock123 Oct 14, 2016

Member

@nodejs/collaborators can someone review?

Member

Fishrock123 commented Oct 14, 2016

@nodejs/collaborators can someone review?

@jasnell

This comment has been minimized.

Show comment
Hide comment
@jasnell

jasnell Mar 1, 2017

Member

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

Member

jasnell commented Mar 1, 2017

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

@jasnell jasnell added the stalled label Mar 1, 2017

@blakeembrey

This comment has been minimized.

Show comment
Hide comment
@blakeembrey

blakeembrey Mar 1, 2017

Contributor

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

Contributor

blakeembrey commented Mar 1, 2017

@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

This comment has been minimized.

Show comment
Hide comment
@jasnell

jasnell Mar 1, 2017

Member

Yep. Understood. I'll commit to reviewing.

Member

jasnell commented Mar 1, 2017

Yep. Understood. I'll commit to reviewing.

@targos targos self-assigned this Mar 1, 2017

@targos

This comment has been minimized.

Show comment
Hide comment
@targos

targos Mar 1, 2017

Member

I'll review it too.

Member

targos commented Mar 1, 2017

I'll review it too.

@blakeembrey

This comment has been minimized.

Show comment
Hide comment
@blakeembrey

blakeembrey Mar 3, 2017

Contributor

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

Contributor

blakeembrey commented Mar 3, 2017

@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

This comment has been minimized.

Show comment
Hide comment
@gibfahn
Member

gibfahn commented Mar 3, 2017

@jasnell

jasnell approved these changes Mar 3, 2017

@cjihrig

This comment has been minimized.

Show comment
Hide comment
@cjihrig

cjihrig Mar 13, 2017

Contributor

This has conflicts.

Contributor

cjihrig commented Mar 13, 2017

This has conflicts.

@gibfahn

This comment has been minimized.

Show comment
Hide comment
@gibfahn

gibfahn Mar 13, 2017

Member

@blakeembrey could you rebase?

Member

gibfahn commented Mar 13, 2017

@blakeembrey could you rebase?

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

This comment has been minimized.

Show comment
Hide comment
@blakeembrey

blakeembrey Mar 13, 2017

Contributor

@cjihrig @gibfahn Updated again.

Contributor

blakeembrey commented Mar 13, 2017

@cjihrig @gibfahn Updated again.

@targos

targos approved these changes Mar 14, 2017

@targos

This comment has been minimized.

Show comment
Hide comment

jasnell added a commit that referenced this pull request Mar 14, 2017

repl: refactor `LineParser` implementation
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>
@jasnell

This comment has been minimized.

Show comment
Hide comment
@jasnell

jasnell Mar 14, 2017

Member

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

Member

jasnell commented Mar 14, 2017

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

@jasnell jasnell closed this Mar 14, 2017

@blakeembrey blakeembrey deleted the blakeembrey:repl-custom-eval branch Mar 14, 2017

@blakeembrey

This comment has been minimized.

Show comment
Hide comment
@blakeembrey

blakeembrey Mar 14, 2017

Contributor

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

Contributor

blakeembrey commented Mar 14, 2017

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

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

This comment has been minimized.

@jkrems

jkrems Mar 15, 2017

Contributor

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

@jkrems

jkrems Mar 15, 2017

Contributor

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

This comment has been minimized.

@jasnell

jasnell Mar 15, 2017

Member

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

@jasnell

jasnell Mar 15, 2017

Member

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

This comment has been minimized.

@jasnell

jasnell Mar 15, 2017

Member

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

@jasnell

jasnell Mar 15, 2017

Member

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

This comment has been minimized.

@jkrems

jkrems Mar 15, 2017

Contributor

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

@jkrems

jkrems Mar 15, 2017

Contributor

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

This comment has been minimized.

@jkrems

jkrems Mar 15, 2017

Contributor

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

@jkrems

jkrems Mar 15, 2017

Contributor

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

jkrems added a commit to jkrems/node that referenced this pull request Mar 15, 2017

@jkrems jkrems referenced this pull request Mar 15, 2017

Closed

repl: Empty command should be sent to eval function #11871

3 of 3 tasks complete

jasnell added a commit that referenced this pull request Mar 17, 2017

repl: Empty command should be sent to eval function
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>

jungx098 added a commit to jungx098/node that referenced this pull request Mar 21, 2017

repl: refactor `LineParser` implementation
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>

jungx098 added a commit to jungx098/node that referenced this pull request Mar 21, 2017

repl: Empty command should be sent to eval function
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>

@jasnell jasnell referenced this pull request Apr 4, 2017

Closed

8.0.0 Release Proposal #12220

vsemozhetbyt added a commit to vsemozhetbyt/node that referenced this pull request Apr 27, 2017

doc: update example in repl.md
Delete unused method call.

Refs: nodejs#6171

@vsemozhetbyt vsemozhetbyt referenced this pull request Apr 27, 2017

Closed

doc: update example in repl.md #12685

2 of 2 tasks complete

vsemozhetbyt added a commit that referenced this pull request Apr 28, 2017

doc: update example in repl.md
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 a commit to lance/node that referenced this pull request Oct 4, 2017

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

Fixes: nodejs#15704

PR-URL: nodejs#6171
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>

lance added a commit to lance/node that referenced this pull request Oct 4, 2017

repl: refactor `LineParser` implementation
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>
@lance

This comment has been minimized.

Show comment
Hide comment
@lance

lance Oct 4, 2017

Contributor

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

Contributor

lance commented Oct 4, 2017

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

@lance lance referenced this pull request Oct 4, 2017

Closed

[v6.x backport] repl: refactor `LineParser` implementation #15773

3 of 3 tasks complete

MylesBorins added a commit that referenced this pull request Nov 14, 2017

repl: refactor `LineParser` implementation
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>

MylesBorins added a commit that referenced this pull request Nov 14, 2017

repl: refactor `LineParser` implementation
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>

@MylesBorins MylesBorins referenced this pull request Nov 21, 2017

Merged

v6.12.1 proposal #17180

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment