repl: allow multiline arrow functions in .load #14861

lance · 2017-08-16T15:51:10Z

Forces the REPL to be in editorMode while loading a file from disk using the .load command.

Fixes: #14022

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

refack · 2017-08-16T16:50:15Z

lib/repl.js

@@ -1256,13 +1256,16 @@ function defineDefaultCommands(repl) {
      try {
        var stats = fs.statSync(file);
        if (stats && stats.isFile()) {
+          this.editorMode = true;
+          REPLServer.super_.prototype.setPrompt.call(this, '');


this is just so it'll be "pretty", right?

Yes, and it duplicates the behavior in the .editor command. As I was working on this, I thought this could benefit from a little refactoring so that there is an enableEditorMode() function that handles everything in one place. Do you think it's needed?

It would feel natural to have the enableEditorMode but I think this should not block the PR from landing.

refack · 2017-08-16T16:54:46Z

test/parallel/test-repl-load-multiline.js

+const assert = require('assert');
+const repl = require('repl');
+
+testLoadMultiline();


You could replace this with an IIFE or even simply a block scope:

{ const command = `...` ... r.close(); }

Why even scope it all, now that you mention it? It's not leaky.

refack · 2017-08-16T17:04:48Z

/cc @Fishrock123 @addaleax

refack · 2017-08-16T17:05:39Z

So this actually enables any multiline expression through .load?

lance · 2017-08-16T18:50:59Z

So this actually enables any multiline expression through .load?

@refack yes, I suppose that's the case. I didn't test anything other than the specific use case identified in #14022.

Fishrock123 · 2017-08-16T21:38:38Z

Isn't #14503 a better solution? (Load and editor mode?)

lance · 2017-08-16T22:00:40Z

@Fishrock123 oh I hadn't seen that PR referenced in #14022 and didn't know about it. It looks fine too.

I guess it's a question of surface area vs. subtle behavior changes.

Modifying .load to always shift into editor mode does not increase the surface area of the REPLServer default commands, but subtly modifies the behavior. Using .load-editor for the implementation keeps .load's behavior exactly as-is but increases the default REPLServer surface area.

I lean towards reduced surface area, and don't really see how the behavior changes would have an external effect. Always hard to predict the future though and somewhere some code may depend on the .load command not being in editor mode for whatever reason (maybe to avoid terminal codes in the output stream).

My philosophical preference aside, if #14503 is already on its way to landing, and folks feel good about it I am fine with this being closed. I didn't mean to jump into something that was already in flight.

Fishrock123 · 2017-08-16T22:06:28Z

I suppose the question would be, does .load not work correctly without this?

If not, .load-editor seems like a better option. (Although perhaps allowing a way to switch into editor from load separately might be nice? Perhaps that doesn't work though either.)

lance · 2017-08-16T22:47:03Z

Well, .load won't work correctly with some .js files depending on how they are formatted. Here is a simple example that does not use arrow functions, which fails simply based on formatting.

function foo() {
  return {
    bar: function() {
      return 'foobar'
    }
  }
}

const foobar = foo()
    .bar();

console.log(foobar);

Running this as $ node test.js produces 'foobar'. Running it via .load produces this.

> .load test.js
> function foo() {
...   return {
.....     bar: function() {
.......       return 'foobar'
.......     }
.....   }
... }
undefined
> const foobar = foo()
undefined
>     .bar();
Invalid REPL keyword
> console.log(foobar);
{ bar: [Function: bar] }
undefined

refack · 2017-08-17T19:12:16Z

I suppose the question would be, does .load not work correctly without this?

I think the answer is yes, .load is formating sensitive without this. AFAICT it fails for any file with multiline . operator expressions, as well as => expressions.

multiline.js

const foo = console
  .log('foo');

d:\code\node$ node
> .load multiline.js
> const foo = console
undefined
>   .log('foo');
Invalid REPL keyword
>

Fishrock123 · 2017-08-17T22:05:29Z

Hmmm, in that case this may be the better approach. I've never actually used .load but perhaps it doesn't work as I'd expect.

lance · 2017-08-18T19:40:01Z

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

EDIT: CI failures all seem unrelated to this change

jasnell · 2017-08-24T04:50:03Z

I agree that this is likely the better of the approaches, although longer term I think a fairly major overhaul of this part of the code may be warranted.

BridgeAR

LGTM and I think this is definitely the right approach.

BridgeAR · 2017-08-25T04:33:05Z

lib/repl.js

          for (var n = 0; n < lines.length; n++) {
            if (lines[n])
              this.write(`${lines[n]}\n`);
          }
+          this.turnOffEditorMode();
+          this.write('\n');


Does turning the turnOffEditorMode and write("\n") around also change the order of the blank line and the undefined (looking at the test)? If so, I think that would be nice but it is not a blocker at all.

@BridgeAR rather than hold up this PR, I will do this separately.

BridgeAR · 2017-08-25T04:37:08Z

lib/repl.js

@@ -1256,13 +1256,16 @@ function defineDefaultCommands(repl) {
      try {
        var stats = fs.statSync(file);
        if (stats && stats.isFile()) {
+          this.editorMode = true;
+          REPLServer.super_.prototype.setPrompt.call(this, '');


It would feel natural to have the enableEditorMode but I think this should not block the PR from landing.

BridgeAR · 2017-09-01T00:17:09Z

Ping @lance

The `.load` command would fail with any file that contains multiline `.` operator expressions. This was particularly noticeable when chaining promises or multi-line arrow expressions. This change Forces the REPL to be in `editorMode` while loading a file from disk using the `.load` command. Fixes: #14022 PR-URL: #14861 Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Refael Ackermann <[email protected]>

lance · 2017-09-01T14:40:47Z

Landed in 4bd44c1

The `.load` command would fail with any file that contains multiline `.` operator expressions. This was particularly noticeable when chaining promises or multi-line arrow expressions. This change Forces the REPL to be in `editorMode` while loading a file from disk using the `.load` command. Fixes: nodejs/node#14022 PR-URL: nodejs/node#14861 Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Refael Ackermann <[email protected]>

The `.load` command would fail with any file that contains multiline `.` operator expressions. This was particularly noticeable when chaining promises or multi-line arrow expressions. This change Forces the REPL to be in `editorMode` while loading a file from disk using the `.load` command. Fixes: #14022 PR-URL: #14861 Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Refael Ackermann <[email protected]>

MylesBorins · 2017-09-20T19:43:14Z

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.

lance · 2017-10-04T18:19:02Z

@MylesBorins I will submit a backport PR for it this week.

The `.load` command would fail with any file that contains multiline `.` operator expressions. This was particularly noticeable when chaining promises or multi-line arrow expressions. This change Forces the REPL to be in `editorMode` while loading a file from disk using the `.load` command. Fixes: #14022 Backport-PR-URL: #15775 PR-URL: #14861 Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Refael Ackermann <[email protected]>

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

jasnell requested review from BridgeAR and refack August 16, 2017 16:43

refack approved these changes Aug 16, 2017

View reviewed changes

refack requested review from Fishrock123 and addaleax August 16, 2017 16:50

refack reviewed Aug 16, 2017

View reviewed changes

lance mentioned this pull request Aug 17, 2017

lib: repl add .load-editor feature #14503

Closed

BridgeAR approved these changes Aug 25, 2017

View reviewed changes

lance closed this Sep 1, 2017

lance deleted the 14022-repl-load-multiline branch September 1, 2017 14:40

MylesBorins mentioned this pull request Sep 10, 2017

v8.5.0 proposal #15308

Merged

MylesBorins added the backport-requested-v6.x label Sep 20, 2017

lance mentioned this pull request Oct 4, 2017

[v6.x backport] repl: force editorMode in .load #15775

Closed

3 tasks

MylesBorins added backported-to-v6.x and removed backport-requested-v6.x labels Oct 10, 2017

MylesBorins mentioned this pull request Oct 17, 2017

v6.12.0 proposal #16263

Merged

MylesBorins mentioned this pull request Nov 3, 2017

v4.8.6 proposal #16500

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

repl: allow multiline arrow functions in .load #14861

repl: allow multiline arrow functions in .load #14861

lance commented Aug 16, 2017

refack Aug 16, 2017

lance Aug 16, 2017

BridgeAR Aug 25, 2017

refack Aug 16, 2017

lance Aug 16, 2017

refack commented Aug 16, 2017

refack commented Aug 16, 2017

lance commented Aug 16, 2017

Fishrock123 commented Aug 16, 2017 •

edited

Loading

lance commented Aug 16, 2017 •

edited

Loading

Fishrock123 commented Aug 16, 2017

lance commented Aug 16, 2017

refack commented Aug 17, 2017

Fishrock123 commented Aug 17, 2017

lance commented Aug 18, 2017 •

edited

Loading

jasnell commented Aug 24, 2017

BridgeAR left a comment

BridgeAR Aug 25, 2017

lance Sep 1, 2017

BridgeAR Aug 25, 2017

BridgeAR commented Sep 1, 2017

lance commented Sep 1, 2017

MylesBorins commented Sep 20, 2017

lance commented Oct 4, 2017

repl: allow multiline arrow functions in .load #14861

repl: allow multiline arrow functions in .load #14861

Conversation

lance commented Aug 16, 2017

Checklist

Affected core subsystem(s)

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

refack commented Aug 16, 2017

refack commented Aug 16, 2017

lance commented Aug 16, 2017

Fishrock123 commented Aug 16, 2017 • edited Loading

lance commented Aug 16, 2017 • edited Loading

Fishrock123 commented Aug 16, 2017

lance commented Aug 16, 2017

refack commented Aug 17, 2017

multiline.js

Fishrock123 commented Aug 17, 2017

lance commented Aug 18, 2017 • edited Loading

jasnell commented Aug 24, 2017

BridgeAR left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BridgeAR commented Sep 1, 2017

lance commented Sep 1, 2017

MylesBorins commented Sep 20, 2017

lance commented Oct 4, 2017

Fishrock123 commented Aug 16, 2017 •

edited

Loading

lance commented Aug 16, 2017 •

edited

Loading

lance commented Aug 18, 2017 •

edited

Loading