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

 // 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: #40661Fixes: #40635 Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: James M Snell <[email protected]>

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#40661Fixes: nodejs#40635 Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: James M Snell <[email protected]>

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 daacae8Compare October 30, 2021 16:44

Trott reviewed Oct 31, 2021
View reviewed changes

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

Trott approved these changes Oct 31, 2021
View reviewed changes

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

github-actionsbot 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 8d24eefCompare 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 82e0766Compare November 30, 2021 14:56

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

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

github-actionsbot 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 ca3ed36Compare November 12, 2022 01:49

Linkgoronand 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 95b5272Compare 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:mainMay 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

Uh oh!

repl: fix disruptive autocomplete without inspector#40661

repl: fix disruptive autocomplete without inspector #40661

Uh oh!

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!

BridgeARNov 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!

BridgeARNov 29, 2021

Choose a reason for hiding this comment

Uh oh!

LinkgoronNov 30, 2021

Choose a reason for hiding this comment

Uh oh!

BridgeARNov 30, 2021

Choose a reason for hiding this comment

Uh oh!

LinkgoronJan 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

Linkgoron commented Oct 29, 2021•
edited
Loading

BridgeARNov 4, 2021•
edited
Loading

LinkgoronJan 29, 2022•
edited
Loading