repl: do not consider `...` as a REPL command by shivanth · Pull Request #14467 · nodejs/node · GitHub

This fix makes ... in REPL to be considered as a javascript construct
rather than a REPL keyword

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
commit message follows commit guidelines

Affected core subsystem(s)

repl

This fix makes ... in REPL to be considered as a javascript construct rather than a REPL keyword Fixes: nodejs#14426

Thanks for the contribution! Please add a test! REPL tests can be tricky, but this is probably something that can be tested by adding something to test/parallel/test-repl.js. Additional info about our tests in general can be found in https://github.com/nodejs/node/blob/master/doc/guides/writing-tests.md

Change looks good. For adding a test, check out https://github.com/nodejs/node/blob/master/test/parallel/test-repl.js.

Please use strict not equals, so trimmedCmd.charAt(1) !== '.'

FWIW commit message is missing a space after the colon.

@shivanth thank for your contribution 🥇. Don't be alarmed by all the reviews, as far as I can see they are just in order to make your submission even better. I would really want to see you follow up, so this PR will land.

P.S. as far as I can see this change also enables ..* to be treated as a non REPL command. IMHO that's good since it's just as invalid as ...[] 👍 So when you add a test, try that as well.

@refack The ...command seems to put the repl into an inconsistent state, it always shows up a multiline command, which can only be exited by pressing ctrl-c.

> ..save
... dad
... ()
... blah
...
>

I came across this when I tried to add a new test case

The...command seems to put the repl into an inconsistent state

That's something that should be investigated separately. Any kind of invalid syntax does it, but is has its uses too:

> a=
... 1;
1

Same also works on the shell:

$ node -p "a=\
dquote> 1"
1

The question is if it can be determined if a line can never be valid, like ...something, so we don't show the line continuation and output the error immediately, but I guess this can get complex.

As a first step, I'm fine if you just make sure it not gets parsed as a REPL command.

Because my test case is leaving the REPL in an inconsistent state, the tests that follow my new test fails ...

sending "...[]"
Unix data: "... ", expecting "... "
sending "ref = 1"
Unix data: "... ", expecting /^ReferenceError:\sref\sis\snot\sdefined\n\s+at\srepl:1:5/
assert.js:43
  throw new errors.AssertionError({
  ^

AssertionError [ERR_ASSERTION]: '... ' === /^ReferenceError:\sref\sis\snot\sdefined\n\s+at\srepl:1:5/

Done 👍

@Trott @TimothyGu

The test as it stands right now does not fail on current master so it is not testing the feature implemented here.

test added, dismissing review

@princejwesley @addaleax @Fishrock123

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

the linter might fail due to no space between the comment start?

It passes as we do not use the spaced-comment rule. I fixed it anyways.

Thanks, landed in 46d3ff2!

I fixed the whitespace issues in the test and wrapped the long line in lib so it passed the linter.

This fix makes ... in REPL to be considered as a javascript construct rather than a REPL keyword. Fixes: #14426 PR-URL: #14467 Reviewed-By: Roman Reiss <me@silverwind.io> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>

Should this be backported to v6.x-staging? If yes please follow the guide and raise a backport PR, if no let me know or add the dont-land-on label.

I'd say so. @shivanth wanna do it?

I'm in 👍

This fix makes ... in REPL to be considered as a javascript construct rather than a REPL keyword. Fixes: nodejs#14426 PR-URL: nodejs#14467 Reviewed-By: Roman Reiss <me@silverwind.io> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>

This fix makes ... in REPL to be considered as a javascript construct rather than a REPL keyword. Fixes: #14426 Backport-PR-URL: #14915 PR-URL: #14467 Reviewed-By: Roman Reiss <me@silverwind.io> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>

Was an issue ever created for the aforementioned problem where some syntax errors put the repl in an 'inconsistent' ... state until you Ctrl+C?

I found #18915, but its a PR...

repl:Do not consider ... as a REPL command

d5b6ec8

This fix makes ... in REPL to be considered as a javascript construct rather than a REPL keyword Fixes: nodejs#14426

nodejs-github-bot added the repl Issues and PRs related to the REPL subsystem. label Jul 25, 2017

Trott previously requested changes Jul 25, 2017

View reviewed changes

TimothyGu reviewed Jul 25, 2017

View reviewed changes

refack changed the title repl:vDo not consider ... as a REPL command repl: do not consider ... as a REPL command Jul 25, 2017

use !== instead of !=

e2a61f6

Add test for REPL

067ed99

Fix the test, add the .break command after the test

cfce5a2

Make a more concrete test case

08327ff

silverwind approved these changes Jul 27, 2017

View reviewed changes

addaleax approved these changes Jul 27, 2017

View reviewed changes

Fishrock123 approved these changes Jul 27, 2017

View reviewed changes

silverwind closed this Jul 29, 2017

addaleax mentioned this pull request Aug 2, 2017

v8.3.0 proposal #14594

Merged

MylesBorins added the lts-watch-v6.x label Aug 16, 2017

shivanth mentioned this pull request Aug 18, 2017

[v6.x backport] Repl: do not consider ... as a REPL command #14915

Closed

4 tasks

MylesBorins added backported-to-v6.x and removed lts-watch-v6.x labels Sep 19, 2017

MylesBorins mentioned this pull request Sep 20, 2017

v6.11.4 proposal #15506

Merged

jagretz mentioned this pull request Sep 27, 2018

Can't npm start - destructured props - Unexpected token ... jxnblk/mdx-deck#184

Closed

                   // display next prompt and return.
                   if (trimmedCmd) {
-                    if (trimmedCmd.charAt(0) === '.' && isNaN(parseFloat(trimmedCmd))) {
+                    if (trimmedCmd.charAt(0) === '.' && trimmedCmd.charAt(1) != '.' && isNaN(parseFloat(trimmedCmd))) {

                   { client: client_unix, send: ' \t  \n',
-                    expect: prompt_unix }
+                    expect: prompt_unix },
+                  //Do not parse `...[]` as a REPL keyword

repl: do not consider `...` as a REPL command by shivanth · Pull Request #14467 · nodejs/node · GitHub

Uh oh!

Uh oh!

repl: do not consider ... as a REPL command - #14467

repl: do not consider ... as a REPL command#14467shivanth wants to merge 5 commits intonodejs:masternodejs/node:masterfrom shivanth:repl_fixshivanth/node:repl_fixCopy head branch name to clipboard

Conversation

shivanth commented Jul 25, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Affected core subsystem(s)

Uh oh!

Trott left a comment

Choose a reason for hiding this comment

Uh oh!

silverwind commented Jul 25, 2017

Uh oh!

TimothyGu Jul 25, 2017 • 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!

mscdex commented Jul 25, 2017

Uh oh!

refack commented Jul 25, 2017

Uh oh!

refack commented Jul 25, 2017

Uh oh!

shivanth commented Jul 25, 2017

Uh oh!

silverwind commented Jul 25, 2017

Uh oh!

shivanth commented Jul 26, 2017

Uh oh!

shivanth commented Jul 26, 2017

Uh oh!

shivanth commented Jul 27, 2017

Uh oh!

Trott commented Jul 27, 2017

Uh oh!

Trott commented Jul 27, 2017

Uh oh!

Fishrock123 left a comment

Choose a reason for hiding this comment

Uh oh!

Fishrock123 Jul 27, 2017

Choose a reason for hiding this comment

Uh oh!

silverwind Jul 29, 2017

Choose a reason for hiding this comment

Uh oh!

silverwind commented Jul 29, 2017

Uh oh!

MylesBorins commented Aug 16, 2017

Uh oh!

silverwind commented Aug 16, 2017

Uh oh!

shivanth commented Aug 16, 2017

Uh oh!

0joshuaolson1 commented Feb 27, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

11 participants

repl: do not consider ... as a REPL command#14467
shivanth wants to merge 5 commits into
nodejs:masterfrom
shivanth:repl_fix

shivanth commented Jul 25, 2017 •
edited
Loading

TimothyGu Jul 25, 2017 •
edited
Loading