repl: support previews by eager evaluating input #30811

BridgeAR · 2019-12-05T19:14:20Z

This adds input previews by using the inspectors eager evaluation
functionality.
It is implemented as additional line that is not counted towards
the actual input. In case no colors are supported, it will be visible
as comment. Otherwise it's grey.
It will be triggered on any line change. It is heavily tested against
edge cases and adheres to "dumb" terminals (previews are deactived
in that case).

Small videos that outline the behavior:

https://asciinema.org/a/YwJ92D8vrj7bD5XGNGZzFhxU8
https://asciinema.org/a/bgalyUuG49w96RSzwfhYzLpXL

This is an alternative to #22875.

Fixes: #20977

Checklist

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

BridgeAR · 2019-12-05T19:30:35Z

test/parallel/test-repl-top-level-await.js

+    [ 'var ll = await Promise.resolve(2);', 'undefined' ],
+    [ 'll', ['// 2\r', '2'] ],
+    [ 'foo(await koo())',
+      [ 'f', '// 5oo', '// [Function: foo](await koo())\r', '4' ] ],


For reviewers: this output looks strange but it's correct. As soon as f is entered, the preview is generated and printed. Right afterwards oo is entered and the preview for the next variable is printed.

mscdex · 2019-12-05T22:26:16Z

Can we have this be configurable in the repl API (other than relying on changing terminal)?

jasnell · 2019-12-05T23:40:04Z

+1 to have this be configurable on the repl API. But I really like it being enabled by default for the default repl.

BridgeAR · 2019-12-06T00:20:31Z

I just updated the code and made it configurable. PTAL.

mscdex · 2019-12-06T02:29:37Z

lib/repl.js

@@ -203,6 +204,9 @@ function REPLServer(prompt,
    }
  }

+  const preview = options.terminal &&
+    (options.preview !== undefined ? !!options.preview : true);


Doesn't this still prevent previews from being enabled if terminal is false-y? Why not only take .terminal into account if options.preview === undefined?

That is correct. I would have liked to do that but the current proposal does not support previews for non-terminals yet. That requires additional work and I would rather do that in a follow-up PR.

lib/internal/repl/utils.js

antsmartian · 2019-12-06T06:00:35Z

@BridgeAR While I was expecting few reviews for my PR (#22875), saw this PR :) Anyways, I would love to make the preview feature our own Repl. Looks like this PR handles more cases like color etc (which my PR doesn't), so happy to close my PR and get this PR land soon! Thanks.

test/parallel/test-repl-history-navigation.js

lib/internal/repl/utils.js

BridgeAR · 2019-12-06T16:50:49Z

I just pushed some small fixes that improve the preview even further: it should now be able to print everything that has no side effects correctly and the preview is even going to stick around after a window resize.

nodejs-github-bot · 2019-12-07T00:43:14Z

CI: https://ci.nodejs.org/job/node-test-pull-request/27429/ ✔️

lib/repl.js

lib/internal/repl/utils.js

This adds input previews by using the inspectors eager evaluation functionality. It is implemented as additional line that is not counted towards the actual input. In case no colors are supported, it will be visible as comment. Otherwise it's grey. It will be triggered on any line change. It is heavily tested against edge cases and adheres to "dumb" terminals (previews are deactived in that case). Fixes: nodejs#20977

BridgeAR · 2019-12-08T20:07:11Z

Rebased due to conflicts.

This adds input previews by using the inspectors eager evaluation functionality. It is implemented as additional line that is not counted towards the actual input. In case no colors are supported, it will be visible as comment. Otherwise it's grey. It will be triggered on any line change. It is heavily tested against edge cases and adheres to "dumb" terminals (previews are deactived in that case). PR-URL: #30811 Fixes: #20977 Reviewed-By: Yongsheng Zhang <zyszys98@gmail.com> Reviewed-By: Anto Aravinth <anto.aravinth.cse@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com>

BridgeAR · 2019-12-09T23:27:24Z

Landed in 6bdf8d1 🎉

This adds input previews by using the inspectors eager evaluation functionality. It is implemented as additional line that is not counted towards the actual input. In case no colors are supported, it will be visible as comment. Otherwise it's grey. It will be triggered on any line change. It is heavily tested against edge cases and adheres to "dumb" terminals (previews are deactived in that case). PR-URL: #30811 Fixes: #20977 Reviewed-By: Yongsheng Zhang <zyszys98@gmail.com> Reviewed-By: Anto Aravinth <anto.aravinth.cse@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com>

This is a security release. This release includes a single commit, an update to npm to 6.13.4. For more details about the vulnerability please consult the npm blog: https://blog.npmjs.org/post/189618601100/binary-planting-with-the-npm-cli Notable Changes: * deps: - update npm to 6.13.4 #30904 - update uvwasi (Anna Henningsen) #30745 - upgrade to libuv 1.34.0 (Colin Ihrig) #30783 * doc: - docs deprecate http finished (Robert Nagy) #28679 * events: - add captureRejection option (Matteo Collina) #27867 * http: - add captureRejection support (Matteo Collina) #27867 - llhttp opt-in insecure HTTP header parsing (Sam Roberts) #30567 * http2: - implement capture rection for 'request' and 'stream' events (Matteo Collina) #27867 * net: - implement capture rejections for 'connection' event (Matteo Collina) #27867 * repl: - support previews by eager evaluating input (Ruben Bridgewater) #30811 * stream: - add support for captureRejection option (Matteo Collina) #27867 * tls: - implement capture rejections for 'secureConnection' event (Matteo Collina) #27867 - expose IETF name for current cipher suite (Sam Roberts) #30637 * worker: - add argv constructor option (legendecas) #30559 PR-URL: #30937

This is a security release. This release includes a single commit, an update to npm to 6.13.4. For more details about the vulnerability please consult the npm blog: https://blog.npmjs.org/post/189618601100/binary-planting-with-the-npm-cli Notable Changes: * deps: * update npm to 6.13.4 #30904 * update uvwasi (Anna Henningsen) #30745 * upgrade to libuv 1.34.0 (Colin Ihrig) #30783 * doc: * docs deprecate http finished (Robert Nagy) #28679 * events: * add captureRejection option (Matteo Collina) #27867 * http: * add captureRejection support (Matteo Collina) #27867 * llhttp opt-in insecure HTTP header parsing (Sam Roberts) #30567 * http2: * implement capture rection for 'request' and 'stream' events (Matteo Collina) #27867 * net: * implement capture rejections for 'connection' event (Matteo Collina) #27867 * repl: * support previews by eager evaluating input (Ruben Bridgewater) #30811 * stream: * add support for captureRejection option (Matteo Collina) #27867 * tls: * implement capture rejections for 'secureConnection' event (Matteo Collina) #27867 * expose IETF name for current cipher suite (Sam Roberts) #30637 * worker: * add argv constructor option (legendecas) #30559 PR-URL: #30937

This is a security release. For more details about the vulnerability please consult the npm blog: https://blog.npmjs.org/post/189618601100/binary-planting-with-the-npm-cli Notable Changes: * deps: * update npm to 6.13.4 #30904 * update uvwasi (Anna Henningsen) #30745 * upgrade to libuv 1.34.0 (Colin Ihrig) #30783 * doc: * docs deprecate http finished (Robert Nagy) #28679 * events: * add captureRejection option (Matteo Collina) #27867 * http: * add captureRejection support (Matteo Collina) #27867 * llhttp opt-in insecure HTTP header parsing (Sam Roberts) #30567 * http2: * implement capture rection for 'request' and 'stream' events (Matteo Collina) #27867 * net: * implement capture rejections for 'connection' event (Matteo Collina) #27867 * repl: * support previews by eager evaluating input (Ruben Bridgewater) #30811 * stream: * add support for captureRejection option (Matteo Collina) #27867 * tls: * implement capture rejections for 'secureConnection' event (Matteo Collina) #27867 * expose IETF name for current cipher suite (Sam Roberts) #30637 * worker: * add argv constructor option (legendecas) #30559 PR-URL: #30937

This adds input previews by using the inspectors eager evaluation functionality. It is implemented as additional line that is not counted towards the actual input. In case no colors are supported, it will be visible as comment. Otherwise it's grey. It will be triggered on any line change. It is heavily tested against edge cases and adheres to "dumb" terminals (previews are deactived in that case). PR-URL: nodejs#30811 Fixes: nodejs#20977 Reviewed-By: Yongsheng Zhang <zyszys98@gmail.com> Reviewed-By: Anto Aravinth <anto.aravinth.cse@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com>

This adds input previews by using the inspectors eager evaluation functionality. It is implemented as additional line that is not counted towards the actual input. In case no colors are supported, it will be visible as comment. Otherwise it's grey. It will be triggered on any line change. It is heavily tested against edge cases and adheres to "dumb" terminals (previews are deactived in that case). PR-URL: #30811 Fixes: #20977 Reviewed-By: Yongsheng Zhang <zyszys98@gmail.com> Reviewed-By: Anto Aravinth <anto.aravinth.cse@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com>

mvduin · 2020-09-22T21:01:34Z

So ehm... how can I turn it off ?

When I'm typing a command in a repl I want to focus on what I'm typing and I find it very distracting to have text dancing around my input line. I've also had cases where it was eagerly evaluating fairly expensive computations and the repl started lagging, and I've had a case where I was editing a line that invoked a v8 native (which are rather intolerant to bad inputs) and eager evaluation resulted in a v8 crash.

BridgeAR · 2020-09-22T21:44:05Z

@mvduin it is possible to pass through any options that you like when starting the REPL directly. It's possible to deactivate the preview mode by passing through preview: false as in: node -e 'repl.start({ preview: false })'.

Do you still remember the command that caused the crash? Any method that might cause a side effect is normally not triggered. If it is marked as side effect free while it's not, than that's a bug and we should fix it.

mvduin · 2020-09-23T00:05:30Z

That invocation worked but lacks persistent history. I tried node -e 'repl.start({ preview: false }).setupHistory( path.join( os.homedir(), ".node_repl_history" ) )' but it appears the callback is mandatory and I have no idea what it wants from me.

It would be nice if there were a simple way to set default options for the repl instance created when you invoke nodejs interactively, via a config file or environment variable. I did just come up with a gross but functional workaround: I set NODE_OPTIONS="-r $HOME/.node_defaults.js" where that file contains;

'use strict';
const repl = require('repl');
const orig_start = repl.start;
repl.start = options => orig_start({
	...options,
	preview: false,
});

And the problem wasn't side-effects, it's that all v8 natives use asserts rather than exceptions for type-checking their arguments. An example expression that crashes if typed with preview enabled is %HasFastPackedElements(1).

I also just realized another danger of preview: all of its evaluations still contribute to the optimizer statistics and it's not hard to imagine accidently calling things with abnormal types while editing a line and making a mess. This could be especially bad if you're trying to do benchmarks or analyze performance problems.

However, while trying to make an example to demonstrate that, I found a significantly more serious problem: start node --allow-natives-syntax (I tried both node v12 and v14) and copy-paste:

const foo = (x) => [ x, x, x, x ];
for( let i = 0; i < 100; i++ ) foo(42);
%OptimizeFunctionOnNextCall(foo)
let x = foo(42)  // x = prevents preview
x = foo(42)  // x = prevents preview
assert.equal( %GetOptimizationStatus(foo), 0b110001 );  // TurboFan optimized

Now type foo(42) to preview it and the optimization is instantly gone from foo and I have no clue why since it should have no reason for deopt. Using a debug build of node with --trace-deopt shows it indeed happening though only one line of the deopt log is visible, no doubt due to the state of the terminal when the deopt happens.

BridgeAR · 2020-09-23T07:41:12Z

--allow-natives-syntax is not supported. Activating that is definitely going to cause issues. It should be fine to deactivate the preview if this flag is active during bootstrapping though.

mvduin · 2020-09-23T13:19:22Z

It is a very useful tool to better understand v8 and optimization.

The demonstration that preview of foo(42) is inexplicably triggering deoptimization of foo is a good example of that: instead of %OptimizeFunctionOnNextCall you can just call foo(42) enough times to trigger V8 to optimize it and instead of %GetOptimizationStatus you can use a performance measurement to notice deoptimization (although that will require replacing foo by something that's slow enough to be able to tell the difference) or use you can use a debug build of node with --trace-deopt. But using natives is just more convenient.

bl-ue · 2021-03-03T16:18:19Z

@BridgeAR this was a great feature, but it has a problem, see #37585.

nodejs-github-bot added the repl Issues and PRs related to the REPL subsystem. label Dec 5, 2019

BridgeAR commented Dec 5, 2019

View reviewed changes

BridgeAR added the semver-minor PRs that contain new features and should be released in the next minor version. label Dec 5, 2019

This comment has been minimized.

Sign in to view

BridgeAR force-pushed the implement-repl-previews branch from 9fc4363 to 54a9b87 Compare December 5, 2019 22:26

This comment has been minimized.

Sign in to view

BridgeAR force-pushed the implement-repl-previews branch from 580ed6e to 87b35e4 Compare December 6, 2019 01:40

This comment has been minimized.

Sign in to view

mscdex reviewed Dec 6, 2019

View reviewed changes

antsmartian reviewed Dec 6, 2019

View reviewed changes

lib/internal/repl/utils.js Show resolved Hide resolved

antsmartian reviewed Dec 6, 2019

View reviewed changes

test/parallel/test-repl-history-navigation.js Show resolved Hide resolved

antsmartian reviewed Dec 6, 2019

View reviewed changes

lib/internal/repl/utils.js Show resolved Hide resolved

This comment has been minimized.

Sign in to view

BridgeAR added the notable-change PRs with changes that should be highlighted in changelogs. label Dec 6, 2019

This comment has been minimized.

Sign in to view

BridgeAR added the review wanted PRs that need reviews. label Dec 6, 2019

This comment has been minimized.

Sign in to view

ZYSzys reviewed Dec 8, 2019

View reviewed changes

lib/repl.js Show resolved Hide resolved

lib/internal/repl/utils.js Show resolved Hide resolved

BridgeAR force-pushed the implement-repl-previews branch from 2d80fc5 to 8bba55a Compare December 8, 2019 20:06

This comment has been minimized.

Sign in to view

BridgeAR removed the review wanted PRs that need reviews. label Dec 9, 2019

BridgeAR closed this Dec 9, 2019

BridgeAR mentioned this pull request Dec 9, 2019

repl: Support for eager evaluation #22875

Closed

4 tasks

MylesBorins mentioned this pull request Dec 13, 2019

v13.4.0 proposal #30937

Merged

targos added baking-for-lts PRs that need to wait before landing in a LTS release. lts-watch-v12.x labels Jan 14, 2020

BridgeAR deleted the implement-repl-previews branch January 20, 2020 12:07

targos removed baking-for-lts PRs that need to wait before landing in a LTS release. lts-watch-v12.x author ready PRs that have at least one approval, no pending requests for changes, and a CI started. labels Apr 25, 2020

targos mentioned this pull request May 2, 2020

v12.17.0 release proposal #33197

Merged

bl-ue mentioned this pull request Mar 3, 2021

Strange repl eager-eval behavior #37585

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

repl: support previews by eager evaluating input #30811

repl: support previews by eager evaluating input #30811

BridgeAR commented Dec 5, 2019

BridgeAR Dec 5, 2019

This comment has been minimized.

mscdex commented Dec 5, 2019

jasnell commented Dec 5, 2019

BridgeAR commented Dec 6, 2019

This comment has been minimized.

This comment has been minimized.

mscdex Dec 6, 2019

BridgeAR Dec 6, 2019

antsmartian commented Dec 6, 2019

This comment has been minimized.

BridgeAR commented Dec 6, 2019

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

nodejs-github-bot commented Dec 7, 2019 •

edited by Trott

BridgeAR commented Dec 8, 2019

This comment has been minimized.

BridgeAR commented Dec 9, 2019

mvduin commented Sep 22, 2020

BridgeAR commented Sep 22, 2020

mvduin commented Sep 23, 2020

BridgeAR commented Sep 23, 2020

mvduin commented Sep 23, 2020

bl-ue commented Mar 3, 2021

repl: support previews by eager evaluating input #30811

repl: support previews by eager evaluating input #30811

Conversation

BridgeAR commented Dec 5, 2019

Checklist

BridgeAR Dec 5, 2019

Choose a reason for hiding this comment

This comment has been minimized.

mscdex commented Dec 5, 2019

jasnell commented Dec 5, 2019

BridgeAR commented Dec 6, 2019

This comment has been minimized.

This comment has been minimized.

mscdex Dec 6, 2019

Choose a reason for hiding this comment

BridgeAR Dec 6, 2019

Choose a reason for hiding this comment

antsmartian commented Dec 6, 2019

This comment has been minimized.

BridgeAR commented Dec 6, 2019

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

nodejs-github-bot commented Dec 7, 2019 • edited by Trott

BridgeAR commented Dec 8, 2019

This comment has been minimized.

BridgeAR commented Dec 9, 2019

mvduin commented Sep 22, 2020

BridgeAR commented Sep 22, 2020

mvduin commented Sep 23, 2020

BridgeAR commented Sep 23, 2020

mvduin commented Sep 23, 2020

bl-ue commented Mar 3, 2021

nodejs-github-bot commented Dec 7, 2019 •

edited by Trott