repl: fix disruptive autocomplete without inspector #40661

Linkgoron · 2021-10-29T23:38:15Z

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

Trott

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

Linkgoron · 2021-10-30T16:41:56Z

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'
  ]

test/parallel/test-repl-preview-without-inspector.js

Trott · 2021-10-31T05:00:03Z

@nodejs/repl

nodejs-github-bot · 2021-11-01T12:41:27Z

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

Trott · 2021-11-02T02:09:21Z

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

nodejs-github-bot · 2021-11-02T11:16:13Z

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

BridgeAR

LGTM with my comment addressed.

BridgeAR · 2021-11-04T21:09:47Z

lib/internal/repl/utils.js

@@ -366,7 +366,8 @@ function setupPreview(repl, contextSymbol, bufferSymbol, active) {

  const showPreview = () => {
    // Prevent duplicated previews after a refresh.
-    if (inputPreview !== null || !repl.isCompletionEnabled) {
+    if (inputPreview !== null || !repl.isCompletionEnabled ||
+        !process.features.inspector) {


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).

nodejs-github-bot · 2021-11-14T23:40:12Z

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

nodejs-github-bot · 2021-11-15T23:58:12Z

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

nodejs-github-bot · 2022-01-31T09:59:40Z

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

nodejs-github-bot · 2022-01-31T11:51:23Z

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

nodejs-github-bot · 2022-01-31T14:20:28Z

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

nodejs-github-bot · 2022-02-01T08:56:11Z

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

nodejs-github-bot · 2024-05-11T13:41:44Z

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

nodejs-github-bot · 2024-05-12T07:19:01Z

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

aduh95 · 2024-05-12T09:15:27Z

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

test/parallel/test-repl-preview-without-inspector.js Outdated Show resolved Hide resolved

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 pending requests for changes, and a CI started. label May 11, 2024

aduh95 merged commit 1223294 into nodejs:main May 12, 2024
41 of 53 checks passed

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

repl: fix disruptive autocomplete without inspector #40661

repl: fix disruptive autocomplete without inspector #40661

Linkgoron commented Oct 29, 2021 •

edited

Loading

Trott left a comment

Linkgoron commented Oct 30, 2021

Trott commented Oct 31, 2021

nodejs-github-bot commented Nov 1, 2021

Trott commented Nov 2, 2021

nodejs-github-bot commented Nov 2, 2021

BridgeAR left a comment

BridgeAR Nov 4, 2021 •

edited

Loading

BridgeAR Nov 29, 2021

Linkgoron Nov 30, 2021

BridgeAR Nov 30, 2021

Linkgoron Jan 29, 2022 •

edited

Loading

nodejs-github-bot commented Nov 14, 2021

nodejs-github-bot commented Nov 15, 2021

nodejs-github-bot commented Jan 31, 2022

nodejs-github-bot commented Jan 31, 2022

nodejs-github-bot commented Jan 31, 2022

nodejs-github-bot commented Feb 1, 2022

nodejs-github-bot commented May 11, 2024

nodejs-github-bot commented May 12, 2024

aduh95 commented May 12, 2024

repl: fix disruptive autocomplete without inspector #40661

repl: fix disruptive autocomplete without inspector #40661

Conversation

Linkgoron commented Oct 29, 2021 • edited Loading

Trott left a comment

Choose a reason for hiding this comment

Linkgoron commented Oct 30, 2021

Trott commented Oct 31, 2021

nodejs-github-bot commented Nov 1, 2021

Trott commented Nov 2, 2021

nodejs-github-bot commented Nov 2, 2021

BridgeAR left a comment

Choose a reason for hiding this comment

BridgeAR Nov 4, 2021 • edited Loading

Choose a reason for hiding this comment

BridgeAR Nov 29, 2021

Choose a reason for hiding this comment

Linkgoron Nov 30, 2021

Choose a reason for hiding this comment

BridgeAR Nov 30, 2021

Choose a reason for hiding this comment

Linkgoron Jan 29, 2022 • edited Loading

Choose a reason for hiding this comment

nodejs-github-bot commented Nov 14, 2021

nodejs-github-bot commented Nov 15, 2021

nodejs-github-bot commented Jan 31, 2022

nodejs-github-bot commented Jan 31, 2022

nodejs-github-bot commented Jan 31, 2022

nodejs-github-bot commented Feb 1, 2022

nodejs-github-bot commented May 11, 2024

nodejs-github-bot commented May 12, 2024

aduh95 commented May 12, 2024

Linkgoron commented Oct 29, 2021 •

edited

Loading

BridgeAR Nov 4, 2021 •

edited

Loading

Linkgoron Jan 29, 2022 •

edited

Loading