repl: Support for eager evaluation #22875

antsmartian · 2018-09-15T14:55:13Z

Address the issue #20977

~~Well, I would say its "initial" checkins. May be work will be required for handling cursors, additional~~ ~~edge cases (may be 🤔 ) etc, but I guess this should be a good starting point.~~

Added code for handling cursors and also for line refresh, when necessary.

Thanks for your time on this.

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

Trott · 2018-09-15T16:39:03Z

@nodejs/repl

devsnek · 2018-09-15T16:49:07Z

any reason this is limited to call expressions?

antsmartian · 2018-09-15T16:53:21Z

@devsnek I need to extend to other expressions as well, but just wanted to post my progress here so that I can get review from the community (mainly on whether or not I'm in right direction).

Edit: Or we can first go with call expressions and expand there-after. As always, its all depends on what community says :)

antsmartian · 2018-09-16T07:52:06Z

@devsnek Added support for other expressions and as well for Identifier too.

devsnek · 2018-09-16T16:54:32Z

lib/repl.js

+    }
+
+    try {
+      if (isValidExpression(line)) {


there's no need to check if its a valid expression, or even an expression at all.

I just thought sending code for every key press for eval might be unnecessary.

Yes we can remove them, anyways the code block is in try catch block, so in worst case that should catch the exception if any.

v8 will detect invalid code anyway, so there's no need to manually check.

@devsnek Ok make sense, if I remove the check, then there would be preview results as necessary. Hence, few test cases will be failing (I could see test-repl-persistent-history.js failing already). Let me work on fixing those test cases and update the PR.

Fishrock123

This pretty neat but certainly needs more work (as you seem to be aware)!

Some things I noticed:

Not all edits to the current line cause the eager eval to re-appear.
The eager eval should probably be colored with grey and the colors of inspect outputs should probably also go though a grey-ing filter.

lib/readline.js

antsmartian · 2018-09-17T15:54:31Z

@Fishrock123 Yes, you are correct. Not all edits (with valid code) trigger eager eval. I knew that it needs to be worked out. Regarding color will change them. Thanks for taking time to review this!

danbev · 2018-09-19T08:42:19Z

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

antsmartian · 2018-10-09T13:15:32Z

Just to give an update, I will find some time later this week, afterwhich I will be updating the PR with comments addressed. Sorry for the delay :)

antsmartian · 2018-10-14T04:48:33Z

@devsnek @Fishrock123 Addressed your comments. Now eager eval will work on all lines (when v8 says it has a preview). Also changed the color to grey. Updated the tests.

Let me know if anything else is needed. Thanks!

antsmartian · 2018-10-18T03:58:29Z

Friendly remainder : @devsnek @Fishrock123 🔉

thefourtheye · 2018-10-24T10:02:17Z

lib/readline.js

@@ -456,6 +458,28 @@ Interface.prototype._insertString = function(c) {

    // a hack to get the line refreshed if it's needed
    this._moveCursor(0);
+    // emit current line for generating preview
+    this.emit('buffer', this.line);


This line is there in both the branches, at the end. Perhaps this can be moved out of the if..else.

thefourtheye · 2018-10-24T10:08:31Z

lib/repl.js

+        }, (error, result) => {
+
+          if (error) {
+            // mostly possible side effect exception


Its better to expect the specific exceptions and ignore them. Is that possible?

Also if there is an error, will the result object always have result property?

Yes result will always have result even in case of error. Will refactor the variable to previewResult, so that its better readable.

And about the exception, it can be syntax issues, side effects etc. Not sure if I can catch them specifically. @devsnek can give some thoughts on this.

thefourtheye · 2018-10-24T10:13:54Z

test/parallel/test-repl-preview-result.js

+
+  assert.ok(actual[0].includes(wrapWithColorCode('\'test\'')));
+  // side effect scenario, preview is not available
+  assert.ok(!actual[2].includes('// '));


appendPreview doesn't have a space character after //. Is this intentional?

Nope, it was a miss. Good catch. Will fix them.

antsmartian · 2018-10-27T14:32:38Z

@thefourtheye I have addressed your comments.

@devsnek @jdalton @Fishrock123 Can you please have a look?

Fishrock123

Getting there... also needs some more tests. :)

I'd really like to see this make it into a release though!

Fishrock123 · 2018-11-15T20:33:22Z

lib/readline.js

+};
+
+Interface.prototype.appendPreview = function(result) {
+  this.previewResult = `\u001b[90m //${result}\u001b[39m`;


Suggested change

this.previewResult = `\u001b[90m //${result}\u001b[39m`;

this.previewResult = `\u001b[90m // ${result}\u001b[39m`;

Fishrock123 · 2018-11-15T20:37:02Z

lib/repl.js

@@ -844,10 +844,49 @@ REPLServer.prototype.createContext = function() {
  });

  addBuiltinLibsToObject(context);
+  previewResults.call(this);


This shouldn't be necessary - can you instead add the function on the class but either prefix the function name with _ or, better, put it behind a Symbol? Something like kPreviewResults = new Symbol('Preview Results Fn'), and then you can do this[kPreviewResults]().

Fishrock123 · 2018-11-15T20:38:46Z

lib/repl.js


  return context;
 };

+function previewResults() {


Do we really need this wrapping function? Also, it's confusing, since there is a variable with the same name.

Fishrock123 · 2018-11-15T20:41:11Z

lib/repl.js

+        }
+
+        if (undefined !== previewResult.result.value) {
+          const value = util.inspect(previewResult.result.value);


Are you sure this is what you think it is?

It currently seems to be printing Objects as Arrays of values:

> process.versions { http_parser: '2.8.0', node: '12.0.0-pre', v8: '7.0.276.38-node.11', uv: '1.24.0', zlib: '1.2.11', ares: '1.15.0', modules: '67', nghttp2: '1.34.0', napi: '3', openssl: '1.1.0i', icu: '63.1', unicode: '11.0', cldr: '34.0', tz: '2018e' } > process.versions //[ '2.8.0', '12.0.0-pre', '7.0.276.38-node.11', '1.24.0', '1.2.11' ]

Fishrock123 · 2018-11-15T20:42:20Z

lib/repl.js

+
+        if (previewResult.result.preview) {
+          const previewResults = previewResult.result.preview.properties;
+          const previewData = previewResults.map((preview) => preview.value); // eslint-disable-line max-len


I think this should avoid the eslint message:

Suggested change

const previewData = previewResults.map((preview) => preview.value); // eslint-disable-line max-len

const previewData = previewResults.map(

(preview) => preview.value

);

Fishrock123 · 2018-11-15T20:42:55Z

lib/repl.js

+          Interface.prototype.appendPreview.call(this, value);
+        }
+      });
+    }, () => []);


Did you mean this?

Suggested change

}, () => []);

}, () => {});

Fishrock123 · 2018-11-15T20:43:36Z

test/parallel/test-repl-preview-result.js

+r.on('exit', common.mustCall(() => {
+  const actual = output.split('\n');
+
+  const wrapWithColorCode = (str) => `\u001b[90m //${str}\u001b[39m`;


Suggested change

const wrapWithColorCode = (str) => `\u001b[90m //${str}\u001b[39m`;

const wrapWithColorCode = (str) => `\u001b[90m // ${str}\u001b[39m`;

antsmartian · 2018-11-16T01:12:33Z

@Fishrock123 Thanks for your review, will update the thread once I'm done with your review comments.

antsmartian · 2018-11-22T15:25:46Z

@Fishrock123 Addressed review comments. I guess few things I need do to:

Add more test. I thought of covering the below:
a. For a simple fn expression
b. Nested objects.
c. Nested objects along with methods.
When a huge object is inspected, we do get cursor move out of the current position, I need to fix that as well.

antsmartian · 2018-11-28T11:23:14Z

@nodejs/repl PTAL.. Have added test cases and made changes requested by @Fishrock123

lib/readline.js

antsmartian · 2019-01-19T06:01:51Z

@BridgeAR will address your comments.
@nodejs/repl @Fishrock123 Friendly ping. Can we get some reviews on this? I want to get this to a closure.. Its open for sometime

antsmartian · 2019-04-01T16:16:13Z

@nodejs/repl @Fishrock123 @BridgeAR PTAL..

BridgeAR · 2019-04-01T20:28:07Z

@antsmartian have you seen my comments? They still seem to apply?

antsmartian · 2019-04-02T14:00:11Z

@BridgeAR Yes, I have addressed the comments and also added check for color part in the prev commit, may be you can have a look now. Thanks!

BridgeAR · 2019-04-02T14:10:44Z

lib/readline.js

+
+  if (columns) {
+    const colorDepth = this.output.getColorDepth();
+    const s = colorDepth > 256 ?


The maximum color depth is 24. You can also use hasColors() instead of getColorDepth() as it's more intuitive to use.

BridgeAR · 2019-04-02T14:12:26Z

lib/readline.js

+    const colorDepth = this.output.getColorDepth();
+    const s = colorDepth > 256 ?
+      `${this._prompt}${this.line}${this.previewResult}` :
+      `${this._prompt}${this.line}${result}`;


This is not identical to the color part (it does not only strip the colors).

BridgeAR · 2019-04-02T14:14:05Z

lib/readline.js

+    const s = colorDepth > 256 ?
+      `${this._prompt}${this.line}${this.previewResult}` :
+      `${this._prompt}${this.line}${result}`;
+    this.output.write(s.length < columns ?


The length check includes the colors but that should ideally not be the case. Colors are not visible characters and should as such not be calculated towards the total length.

BridgeAR · 2019-04-02T14:14:14Z

lib/readline.js

+      `${this._prompt}${this.line}${result}`;
+    this.output.write(s.length < columns ?
+      s : `${s.slice(0, columns - 3)
+        .replace(/\r?\n|\r/g, '')}...\u001b[39m`);


This adds the color unconditionally. Can you also please elaborate what exactly this does? I am not sure I understand why this part is sliced and replaced.

The slice and replace part is mainly for splitting the new lines. Since in output terminal has only limited columns, we need to split them and replace with empty space so that we can show them the content in single line.

We already use util.inspect() on the preview. We should use options that deactivate the colors for the preview and make sure the output is printed on a single line. That can be done with the compact options set to true in combination with breakLength set to Infinity and colors set to false.

I would then limit the preview only instead of limiting the input as well. Right now we unconditionally slice the input value and that doesn't seem right.

@BridgeAR: (sorry for a very late reply)

What you are saying is actually a good idea. But I was just trying to play with the options provided by you but still that doesn't seems to be atleast fixing the problem what we have now. For example, if the user types process, we get a long string, so if we apply the options like below:

util.inspect(process, {breakLength: Infinity, compact: true})

still we are getting results like below:

This actually breaks the cursor position as the output is huge. I feel, the only way for us to slice the string to the length of the tty column. May be is there any other way we could achieve it? 🤔

BridgeAR · 2019-04-02T14:16:41Z

lib/readline.js

+
+// Append eager eval result to the
+// Current line
+Interface.prototype._appendPreview = function(result) {


It would be possible to add internal functionality in a separate file that would only be loaded internally. That way we do not have to pollute the prototype further. An underscore in a property name would not hinder anyone from using or monkey patching this function.

Agreed, I will move them to internal/readline/util.

BridgeAR · 2019-04-02T14:25:47Z

lib/repl.js

+        if (!previewResult.exceptionDetails && previewResult.result.objectId) {
+          eagerSession.post('Runtime.callFunctionOn', {
+            functionDeclaration:
+              'function(arg) { return util.inspect(arg, {depth: Infinity}) }',


depth: Infinity does not sound like a good default to me. The preview should ideally be short and on the same line. This could cause the terminal to add lots of output.

BridgeAR · 2019-04-02T14:33:54Z

lib/readline.js

+      s : `${s.slice(0, columns - 3)
+        .replace(/\r?\n|\r/g, '')}...\u001b[39m`);
+  } else {
+    this.output.write(this._prompt + this.line + this.previewResult);


This adds colors unconditionally.

BridgeAR · 2019-04-07T14:50:52Z

lib/readline.js

+  const columns = this.output.columns;
+  const hasColors = this.output.hasColors();
+  const s = hasColors ?
+    `${this._prompt}${this.line}${this.previewResult}` : line;


I personally would keep the preview, even if no colors are supported. The comment should be indication enough.

If we do not want to have a preview in case the colors are deactivated, we should not start the eagerSession in the REPL. That way no code is run unnecessary.

And is it correct that this._prompt is necessary in one case but not in the other?

Hmmm, I'm planning to send the output to this function and then use util.inspect within the function to customize our output. Do you think that would be a better approach? Because anyways I need to call util.inspect in my REPL session.

BridgeAR · 2019-04-07T14:56:30Z

lib/readline.js

+      `${this._prompt}${this.line}${result}`;
+    this.output.write(s.length < columns ?
+      s : `${s.slice(0, columns - 3)
+        .replace(/\r?\n|\r/g, '')}...\u001b[39m`);


We already use util.inspect() on the preview. We should use options that deactivate the colors for the preview and make sure the output is printed on a single line. That can be done with the compact options set to true in combination with breakLength set to Infinity and colors set to false.

I would then limit the preview only instead of limiting the input as well. Right now we unconditionally slice the input value and that doesn't seem right.

lundibundi · 2019-11-24T14:12:15Z

ping @antsmartian, will you be working on this further?

antsmartian · 2019-11-25T13:03:02Z

@lundibundi Yes. This is open for long time ( I know :( ), I wanted it to get closed. Will look at it this week. Will update the PR soon.

(outdated review)

lundibundi

👍 just a few nits and code refactoring suggestions.

lib/repl.js

lib/internal/readline/utils.js

lib/repl.js

lundibundi · 2019-11-27T17:10:04Z

lib/repl.js

+        // If no exception and we have objectId
+        // Run the expression via callFunctionOn
+        // And return it from util inspect.


Nit:

// If there was no exception and we've got an objectId // stringify it with `util.inspect` via Runtime.callFunctionOn.

antsmartian · 2019-12-03T11:10:13Z

@lundibundi @nodejs/repl PTAL.. 🔉

lundibundi

Mostly LGTM from me, but I'm not that familiar with repl code to put a ✔️. This will have to get approval from @BridgeAR or someone else from @nodejs/repl.
Great work, would love to see preview in the repl.

lundibundi · 2019-12-03T11:18:14Z

lib/internal/repl/utils.js

+    return readline;
+  };
+
+  const { cursorTo, clearScreenDown } = lazyReadline();


🤔 can we just const { cursorTo, clearScreenDown } = require('readline') at the top now?

lundibundi · 2019-12-03T11:18:56Z

lib/internal/repl/utils.js

+    repl.output.write(s);
+  }
+
+  // Move back the cursor to the original position


Nit:

Suggested change

// Move back the cursor to the original position

// Move back the cursor to the original position.

lundibundi · 2019-12-03T11:19:47Z

lib/internal/repl/utils.js

@@ -87,7 +89,98 @@ function isRecoverableError(e, code) {
  }
 }

+// Appends the preview of the result
+// to the tty.
+function appendPreview(repl, result, cursorTo, clearScreenDown) {


Suggested change

function appendPreview(repl, result, cursorTo, clearScreenDown) {

function appendPreview(repl, preview, cursorTo, clearScreenDown) {

maybe? To make it a little clearer.

lundibundi · 2019-12-03T11:20:03Z

lib/internal/repl/utils.js

+// Appends the preview of the result
+// to the tty.


Nit: I think this will fit on one line.

lundibundi · 2019-12-03T11:39:19Z

lib/readline.js

@@ -490,6 +491,8 @@ Interface.prototype._insertString = function(c) {
    // A hack to get the line refreshed if it's needed
    this._moveCursor(0);
  }
+  // Emit current line for generating preview


Nit:

Suggested change

// Emit current line for generating preview

// Emit current line for generating preview.

lundibundi · 2019-12-03T11:40:39Z

lib/internal/repl/utils.js

+      return;
+    }
+
+    if (undefined !== previewResult.result.value) {


Nit:

Suggested change

if (undefined !== previewResult.result.value) {

if (previewResult.result.value !== undefined) {

lundibundi · 2019-12-03T11:47:00Z

lib/repl.js

+  eagerSession.once('Runtime.executionContextCreated',
+                    ({ params: { context } }) => {


Perhaps to avoid big indentation:

eagerSession.once( 'Runtime.executionContextCreated', ({ params: { context } }) => { })

lundibundi · 2019-12-03T11:50:44Z

lib/internal/repl/utils.js

+    repl.output.write(line.length < columns ?
+      s : `${s.slice(0, columns - 3)
+        .replace(/\r?\n|\r/g, '')}...\u001b[39m`);


Nit:

repl.output.write(line.length < columns ? s : s.slice(0, columns - 3).replace(/\r?\n|\r/g, '') + '...\u001b[39m');

nodejs-github-bot · 2019-12-03T13:40:26Z

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

BridgeAR · 2019-12-09T23:31:13Z

This got superseded by #30811
@antsmartian thank you very much for the awesome work!

nodejs-github-bot added readline Issues and PRs related to the built-in readline module. repl Issues and PRs related to the REPL subsystem. labels Sep 15, 2018

antsmartian changed the title ~~repl: Initial support for eager evaluation~~ repl: Support for eager evaluation Sep 16, 2018

antsmartian force-pushed the eager-20977 branch from 32d6067 to 047b6e6 Compare September 16, 2018 07:47

antsmartian force-pushed the eager-20977 branch from 047b6e6 to 7138252 Compare September 16, 2018 08:17

devsnek reviewed Sep 16, 2018

View reviewed changes

Fishrock123 reviewed Sep 17, 2018

View reviewed changes

lib/readline.js Outdated Show resolved Hide resolved

antsmartian force-pushed the eager-20977 branch 3 times, most recently from bcadcce to e005596 Compare October 14, 2018 04:35

thefourtheye reviewed Oct 24, 2018

View reviewed changes

antsmartian force-pushed the eager-20977 branch from e005596 to 2d59a2e Compare October 27, 2018 14:31

Fishrock123 previously requested changes Nov 15, 2018

View reviewed changes

antsmartian force-pushed the eager-20977 branch from e48a315 to 0b0783f Compare November 22, 2018 15:22

antsmartian force-pushed the eager-20977 branch 3 times, most recently from 319ea89 to 87b14fc Compare November 28, 2018 10:23

BridgeAR reviewed Dec 20, 2018

View reviewed changes

lib/readline.js Outdated Show resolved Hide resolved

lib/readline.js Outdated Show resolved Hide resolved

antsmartian mentioned this pull request Mar 26, 2019

Implement "eager evaluation" in the REPL #20977

Closed

antsmartian force-pushed the eager-20977 branch from 5077115 to 7361d93 Compare April 1, 2019 16:15

antsmartian force-pushed the eager-20977 branch from 7361d93 to 8d7c5d9 Compare April 2, 2019 13:57

BridgeAR reviewed Apr 2, 2019

View reviewed changes

antsmartian force-pushed the eager-20977 branch from 8d7c5d9 to 163f7fd Compare April 2, 2019 16:35

BridgeAR reviewed Apr 7, 2019

View reviewed changes

Trott force-pushed the master branch from 1ecc406 to 49cf67e Compare September 17, 2019 16:52

repl: support for eager evaluation

700c94e

antsmartian force-pushed the eager-20977 branch from 163f7fd to 700c94e Compare November 27, 2019 01:09

antsmartian added 3 commits November 27, 2019 12:37

repl: moving fns to internal modules

c7b4b10

repl: remove commented code

6e77bfe

repl: fixing the test cases

5710523

lundibundi reviewed Nov 27, 2019

View reviewed changes

repl: moving the logic to internal modules

b3908e4

antsmartian force-pushed the eager-20977 branch from 73af1b2 to b3908e4 Compare December 3, 2019 11:01

lundibundi reviewed Dec 3, 2019

View reviewed changes

antsmartian added the review wanted PRs that need reviews. label Dec 3, 2019

BridgeAR mentioned this pull request Dec 5, 2019

repl: support previews by eager evaluating input #30811

Closed

4 tasks

BridgeAR closed this Dec 9, 2019

	this.previewResult = `\u001b[90m //${result}\u001b[39m`;
	this.previewResult = `\u001b[90m // ${result}\u001b[39m`;

	const wrapWithColorCode = (str) => `\u001b[90m //${str}\u001b[39m`;
	const wrapWithColorCode = (str) => `\u001b[90m // ${str}\u001b[39m`;

	// Move back the cursor to the original position
	// Move back the cursor to the original position.

	function appendPreview(repl, result, cursorTo, clearScreenDown) {
	function appendPreview(repl, preview, cursorTo, clearScreenDown) {

	// Emit current line for generating preview
	// Emit current line for generating preview.

	if (undefined !== previewResult.result.value) {
	if (previewResult.result.value !== undefined) {

		eagerSession.once('Runtime.executionContextCreated',
		({ params: { context } }) => {

repl: Support for eager evaluation #22875

repl: Support for eager evaluation #22875

Conversation

antsmartian commented Sep 15, 2018 • edited

Trott commented Sep 15, 2018

devsnek commented Sep 15, 2018

antsmartian commented Sep 15, 2018 • edited

antsmartian commented Sep 16, 2018

Choose a reason for hiding this comment

antsmartian Sep 16, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Fishrock123 left a comment

Choose a reason for hiding this comment

antsmartian commented Sep 17, 2018

danbev commented Sep 19, 2018

antsmartian commented Oct 9, 2018

antsmartian commented Oct 14, 2018 • edited

antsmartian commented Oct 18, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

antsmartian Oct 27, 2018 • edited

Choose a reason for hiding this comment

antsmartian Oct 27, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

antsmartian commented Oct 27, 2018

Fishrock123 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

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

antsmartian commented Nov 16, 2018

antsmartian commented Nov 22, 2018

antsmartian commented Nov 28, 2018 • edited

antsmartian commented Jan 19, 2019

antsmartian commented Apr 1, 2019

BridgeAR commented Apr 1, 2019

antsmartian commented Apr 2, 2019

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

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

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

lundibundi commented Nov 24, 2019

antsmartian commented Nov 25, 2019

lundibundi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

antsmartian commented Dec 3, 2019

lundibundi 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

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

nodejs-github-bot commented Dec 3, 2019

BridgeAR commented Dec 9, 2019

antsmartian commented Sep 15, 2018 •

edited

antsmartian commented Sep 15, 2018 •

edited

antsmartian Sep 16, 2018 •

edited

antsmartian commented Oct 14, 2018 •

edited

antsmartian commented Oct 18, 2018 •

edited

antsmartian Oct 27, 2018 •

edited

antsmartian Oct 27, 2018 •

edited

antsmartian commented Nov 28, 2018 •

edited