GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — repl: support previews by eager evaluating input by BridgeAR · Pull Request #30811 · nodejs/node

Conversation

@BridgeAR

@BridgeAR BridgeAR commented Dec 5, 2019

Copy link
Copy Markdown
Member

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
jasnell, lin7sh, bricss, ZYSzys, char8x, and bl-ue reacted with thumbs up emoji bl-ue reacted with hooray emoji lin7sh, bricss, and bl-ue reacted with heart emoji lin7sh, bricss, and bl-ue reacted with rocket emoji bricss and bl-ue reacted with eyes emoji
All reactions
  • 👍 6 reactions
  • 🎉 1 reaction
  • ❤️ 3 reactions
  • 🚀 3 reactions
  • 👀 2 reactions
@nodejs-github-bot nodejs-github-bot added the repl Issues and PRs related to the REPL subsystem. label Dec 5, 2019
BridgeAR
BridgeAR commented Dec 5, 2019
View reviewed changes
Comment thread test/parallel/test-repl-top-level-await.js Outdated

@BridgeAR BridgeAR Dec 5, 2019

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@BridgeAR BridgeAR added the semver-minor PRs that contain new features and should be released in the next minor version. label Dec 5, 2019
@nodejs-github-bot

This comment has been minimized.

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

mscdex commented Dec 5, 2019

Copy link
Copy Markdown
Contributor

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@jasnell

jasnell commented Dec 5, 2019

Copy link
Copy Markdown
Member

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@BridgeAR

BridgeAR commented Dec 6, 2019

Copy link
Copy Markdown
Member Author

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@nodejs-github-bot

This comment has been minimized.

Sign in to view
@BridgeAR
BridgeAR force-pushed the implement-repl-previews branch from 580ed6e to 87b35e4 Compare December 6, 2019 01:40
@nodejs-github-bot

This comment has been minimized.

Sign in to view
mscdex
mscdex reviewed Dec 6, 2019
View reviewed changes
Comment thread lib/repl.js Outdated

@mscdex mscdex Dec 6, 2019

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@BridgeAR BridgeAR Dec 6, 2019

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
antsmartian
antsmartian reviewed Dec 6, 2019
View reviewed changes
Comment thread lib/internal/repl/utils.js Resolved

Uh oh!

There was an error while loading. Please reload this page.
@antsmartian

antsmartian commented Dec 6, 2019

Copy link
Copy Markdown
Contributor

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
antsmartian
antsmartian reviewed Dec 6, 2019
View reviewed changes
Comment thread test/parallel/test-repl-history-navigation.js Resolved

Uh oh!

There was an error while loading. Please reload this page.
antsmartian
antsmartian reviewed Dec 6, 2019
View reviewed changes
Comment thread lib/internal/repl/utils.js Resolved

Uh oh!

There was an error while loading. Please reload this page.
@nodejs-github-bot

This comment has been minimized.

Sign in to view
@BridgeAR

BridgeAR commented Dec 6, 2019

Copy link
Copy Markdown
Member Author

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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@nodejs-github-bot

This comment has been minimized.

Sign in to view
@BridgeAR BridgeAR added the notable-change PRs with changes that should be highlighted in changelogs. label Dec 6, 2019
@nodejs-github-bot

This comment has been minimized.

Sign in to view
@BridgeAR BridgeAR added the review wanted PRs that need review. label Dec 6, 2019
@nodejs-github-bot

This comment has been minimized.

Sign in to view
@nodejs-github-bot

This comment has been minimized.

Sign in to view
@nodejs-github-bot

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

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Collaborator

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
ZYSzys
ZYSzys reviewed Dec 8, 2019
View reviewed changes
Comment thread lib/repl.js Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread lib/internal/repl/utils.js Resolved

Uh oh!

There was an error while loading. Please reload this page.
@BridgeAR
repl: support previews by eager evaluating input
8bba55a 
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
BridgeAR force-pushed the implement-repl-previews branch from 2d80fc5 to 8bba55a Compare December 8, 2019 20:06
@BridgeAR

BridgeAR commented Dec 8, 2019

Copy link
Copy Markdown
Member Author

Rebased due to conflicts.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@nodejs-github-bot

This comment has been minimized.

Sign in to view
BridgeAR added a commit that referenced this pull request Dec 9, 2019
@BridgeAR
repl: support previews by eager evaluating input
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 <[email protected]>
Reviewed-By: Anto Aravinth <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
@BridgeAR

BridgeAR commented Dec 9, 2019

Copy link
Copy Markdown
Member Author

Landed in 6bdf8d1 🎉
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@BridgeAR BridgeAR closed this Dec 9, 2019
@BridgeAR BridgeAR mentioned this pull request Dec 9, 2019
repl: Support for eager evaluation #22875
Closed
3 of 4 tasks
targos pushed a commit that referenced this pull request Dec 10, 2019
@BridgeAR @targos
repl: support previews by eager evaluating input
e551c16 
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 <[email protected]>
Reviewed-By: Anto Aravinth <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Dec 13, 2019
v13.4.0 proposal #30937
Merged
MylesBorins added a commit that referenced this pull request Dec 16, 2019
@MylesBorins
2019-12-17, Version 13.4.0 (Current)
57bf5b7 
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
MylesBorins added a commit that referenced this pull request Dec 16, 2019
@MylesBorins
2019-12-17, Version 13.4.0 (Current)
906a1fd 
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
MylesBorins added a commit that referenced this pull request Dec 16, 2019
@MylesBorins
2019-12-17, Version 13.4.0 (Current)
f15fe94 
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
MylesBorins added a commit that referenced this pull request Dec 17, 2019
@MylesBorins
2019-12-17, Version 13.4.0 (Current)
bef1e87 
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
@targos targos added baking-for-lts PRs whose changes need more time before an LTS backport. lts-watch-v12.x labels Jan 14, 2020
@BridgeAR
BridgeAR deleted the implement-repl-previews branch January 20, 2020 12:07
@targos targos removed baking-for-lts PRs whose changes need more time before an LTS backport. lts-watch-v12.x author ready PRs with CI started, the required approvals, and no outstanding review comments. labels Apr 25, 2020
targos pushed a commit to targos/node that referenced this pull request Apr 25, 2020
@BridgeAR @targos
repl: support previews by eager evaluating input
bf30154 
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 <[email protected]>
Reviewed-By: Anto Aravinth <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
targos pushed a commit that referenced this pull request Apr 28, 2020
@BridgeAR @targos
repl: support previews by eager evaluating input
b14440f 
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 <[email protected]>
Reviewed-By: Anto Aravinth <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
@targos targos mentioned this pull request May 2, 2020
v12.17.0 release proposal #33197
Merged
@mvduin

mvduin commented Sep 22, 2020

Copy link
Copy Markdown

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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@BridgeAR

BridgeAR commented Sep 22, 2020

Copy link
Copy Markdown
Member Author

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@mvduin

mvduin commented Sep 23, 2020

Copy link
Copy Markdown

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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@BridgeAR

BridgeAR commented Sep 23, 2020

Copy link
Copy Markdown
Member Author

--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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@mvduin

mvduin commented Sep 23, 2020

Copy link
Copy Markdown

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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bl-ue bl-ue mentioned this pull request Mar 3, 2021
Strange repl eager-eval behavior #37585
Closed
@bl-ue

bl-ue commented Mar 3, 2021

Copy link
Copy Markdown
Contributor

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@coderabbitai coderabbitai Bot mentioned this pull request Jul 27, 2026
test(repl): expand Node 26.5 parity coverage PerryTS/perry#6869
Merged
11 tasks done
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ZYSzys ZYSzys ZYSzys approved these changes
+3 more reviewers
@mscdex mscdex mscdex left review comments
@antsmartian antsmartian antsmartian approved these changes
@targos targos targos approved these changes
Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

notable-change PRs with changes that should be highlighted in changelogs. repl Issues and PRs related to the REPL subsystem. semver-minor PRs that contain new features and should be released in the next minor version.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Implement "eager evaluation" in the REPL

Uh oh!

There was an error while loading. Please reload this page.

9 participants

@BridgeAR @nodejs-github-bot @mscdex @jasnell @antsmartian @mvduin @bl-ue @targos @ZYSzys
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.