repl: fix disruptive autocomplete without inspector by Linkgoron · Pull Request #40661 · nodejs/node · GitHub

Fix an issue where the autocomplete wrongly autocompletes a value from a correct value to an undefined value when hitting return - when node is built with the --without-inspector flag by disabling the preview view.

I could think of three ways on how to solve this, however I'd be happy to implement something different, if there are better options. The three solutions that I thought of were:

Completely disable the completer when the inspector is disabled.
Show the autocompletion preview, but don't "commit" it when hitting return (I thought that it's a bit confusing when I tried it out).
Disable the preview but keep the completer (e.g. for tab completion, for keywords or built-in modules).

I chose the third option, as it's still possible to use tab completion for built-in modules, keywords and maybe other completions that don't need the inspector. Note that this does make it possible to be on a "correct" value and press tab and move to a different value, even though the original value exists, but I thought that it's an OK compromise vs removing auto-complete completely.

fixes: #40635

Would it be reasonable/not-onerous to add a test for this?

Would it be reasonable/not-onerous to add a test for this?

Done. Basically just copied the test-repl-preview.js and modified it to my new fix. For reference, the test fails without the fix with the following error:

  [
+   'r\x1B[90me\x1B[39m\x1B[9G\x1B[0Ke\r',
+   'Uncaught ReferenceError: re is not defined'
-   'r\r',
-   '\x1B[33m5\x1B[39m'
  ]

@nodejs/repl

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

You'll want to rebase against master to fix a few CI issues. Sorry about the inconvenience.

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

LGTM with my comment addressed.

Please move the condition up to the start of the setup function. There are already two conditions that deactivate the preview as well and it would be good to combine this instead of executing lots of code that is not required anyway.

See https://github.com/nodejs/node/pull/40661/files#diff-d2785f46e53f8d956b9a838d9b22ea519b7eb6f51cc84d33885c231043cf4530R144-R145

Ping @Linkgoron

Do you want me to move the prexisting condition as well (!repl.isCompletionEnabled), or just the specific condition that I've added?

Thanks! The isCompletionEnabled is something that changes during usage. It is set while the repl is paused. As such, it should stay where it is.

@BridgeAR This change had a behavioural change, it broke the move cursor completion without the inspector (e.g. test-repl-history-navigation.js line 559). The issue is caused by _moveCursor not being redefined in line 491 in utils.js (as it still works for built-in modules like util).

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

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

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

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

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

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

Fix an issue where the autocomplete wrongly autocompletes a value from a correct value to an undefined value when node is built without an inspector by disabling the preview view fixes: nodejs#40635

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

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

Landed in 1223294

Fix an issue where the autocomplete wrongly autocompletes a value from a correct value to an undefined value when `node` is built without an inspector by disabling the preview view. fixes: #40635 PR-URL: #40661 Fixes: #40635 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: James M Snell <jasnell@gmail.com>

Fix an issue where the autocomplete wrongly autocompletes a value from a correct value to an undefined value when `node` is built without an inspector by disabling the preview view. fixes: nodejs#40635 PR-URL: nodejs#40661 Fixes: nodejs#40635 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: James M Snell <jasnell@gmail.com>

nodejs-github-bot added needs-ci PRs that need a full CI run. repl Issues and PRs related to the REPL subsystem. labels Oct 29, 2021

Trott reviewed Oct 30, 2021

View reviewed changes

Linkgoron force-pushed the repl-no-ispector-fix branch from 04cc504 to daacae8 Compare October 30, 2021 16:44

Trott reviewed Oct 31, 2021

View reviewed changes

Comment thread test/parallel/test-repl-preview-without-inspector.js Outdated

Trott approved these changes Oct 31, 2021

View reviewed changes

Linkgoron force-pushed the repl-no-ispector-fix branch from daacae8 to 5d99dd2 Compare November 1, 2021 12:37

github-actions Bot mentioned this pull request Nov 2, 2021

CI Reliability 2021-11-02 nodejs/reliability#105

Open

35 tasks

Linkgoron force-pushed the repl-no-ispector-fix branch from 5d99dd2 to 8d24eef Compare November 2, 2021 09:09

This was referenced Nov 3, 2021

CI Reliability 2021-11-03 nodejs/reliability#106

Open

CI Reliability 2021-11-04 nodejs/reliability#107

Open

BridgeAR approved these changes Nov 4, 2021

View reviewed changes

This was referenced Nov 5, 2021

CI Reliability 2021-11-05 nodejs/reliability#108

Open

CI Reliability 2021-11-06 nodejs/reliability#109

Open

CI Reliability 2021-11-07 nodejs/reliability#110

Open

CI Reliability 2021-11-08 nodejs/reliability#111

Open

jasnell approved these changes Nov 13, 2021

View reviewed changes

Linkgoron force-pushed the repl-no-ispector-fix branch from 8d24eef to 82e0766 Compare November 30, 2021 14:56

Linkgoron force-pushed the repl-no-ispector-fix branch from eba1b6e to fa45fb5 Compare January 31, 2022 09:16

Linkgoron force-pushed the repl-no-ispector-fix branch from 0f5f20b to e69ebdf Compare January 31, 2022 12:45

github-actions Bot mentioned this pull request Feb 1, 2022

CI Reliability 2022-02-01 nodejs/reliability#189

Open

31 tasks

This was referenced Feb 2, 2022

CI Reliability 2022-02-02 nodejs/reliability#190

Open

CI Reliability 2022-02-03 nodejs/reliability#191

Open

CI Reliability 2022-02-04 nodejs/reliability#192

Open

CI Reliability 2022-02-05 nodejs/reliability#193

Open

Trott force-pushed the main branch from 2d76238 to ca3ed36 Compare November 12, 2022 01:49

Linkgoron and others added 4 commits May 11, 2024 16:41

repl: fix disruptive autocomplete without inspector

c18cd78

Fix an issue where the autocomplete wrongly autocompletes a value from a correct value to an undefined value when node is built without an inspector by disabling the preview view fixes: nodejs#40635

fixup! repl: fix disruptive autocomplete without inspector

5083c3f

fixup! repl: fix disruptive autocomplete without inspector

dba04ca

fixup! fixup! repl: fix disruptive autocomplete without inspector

95b5272

aduh95 force-pushed the repl-no-ispector-fix branch from e69ebdf to 95b5272 Compare May 11, 2024 13:41

aduh95 added the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label May 11, 2024

aduh95 merged commit 1223294 into nodejs:main May 12, 2024

targos mentioned this pull request May 13, 2024

v22.2.0 release proposal #52971

Merged

marco-ippolito mentioned this pull request Jun 17, 2024

v20.15.0 proposal #53486

Merged

repl: fix disruptive autocomplete without inspector by Linkgoron · Pull Request #40661 · nodejs/node · GitHub

Uh oh!

Uh oh!

repl: fix disruptive autocomplete without inspector - #40661

repl: fix disruptive autocomplete without inspector#40661aduh95 merged 4 commits intonodejs:mainnodejs/node:mainfrom Linkgoron:repl-no-ispector-fixLinkgoron/node:repl-no-ispector-fixCopy head branch name to clipboard

Conversation

Linkgoron commented Oct 29, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Trott left a comment

Choose a reason for hiding this comment

Uh oh!

Linkgoron commented Oct 30, 2021

Uh oh!

Uh oh!

Trott commented Oct 31, 2021

Uh oh!

nodejs-github-bot commented Nov 1, 2021

Uh oh!

Trott commented Nov 2, 2021

Uh oh!

nodejs-github-bot commented Nov 2, 2021

Uh oh!

BridgeAR left a comment

Choose a reason for hiding this comment

Uh oh!

BridgeAR Nov 4, 2021 • 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!

BridgeAR Nov 29, 2021

Choose a reason for hiding this comment

Uh oh!

Linkgoron Nov 30, 2021

Choose a reason for hiding this comment

Uh oh!

BridgeAR Nov 30, 2021

Choose a reason for hiding this comment

Uh oh!

Linkgoron Jan 29, 2022 • 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!

nodejs-github-bot commented Nov 14, 2021

Uh oh!

nodejs-github-bot commented Nov 15, 2021

Uh oh!

nodejs-github-bot commented Jan 31, 2022

Uh oh!

nodejs-github-bot commented Jan 31, 2022

Uh oh!

nodejs-github-bot commented Jan 31, 2022

Uh oh!

nodejs-github-bot commented Feb 1, 2022

Uh oh!

nodejs-github-bot commented May 11, 2024

Uh oh!

nodejs-github-bot commented May 12, 2024

Uh oh!

aduh95 commented May 12, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

repl: fix disruptive autocomplete without inspector#40661
aduh95 merged 4 commits into
nodejs:mainfrom
Linkgoron:repl-no-ispector-fix

Linkgoron commented Oct 29, 2021 •
edited
Loading

BridgeAR Nov 4, 2021 •
edited
Loading

Linkgoron Jan 29, 2022 •
edited
Loading