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: fix disruptive autocomplete without inspector by Linkgoron · Pull Request #40661 · nodejs/node

Conversation

@Linkgoron

@Linkgoron Linkgoron commented Oct 29, 2021 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Contributor

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:

  1. Completely disable the completer when the inspector is disabled.
  2. 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).
  3. 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

Sorry, something went wrong.

Uh oh!

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

bricss reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
@nodejs-github-bot 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
Trott reviewed Oct 30, 2021
View reviewed changes

@Trott Trott left a comment

Copy link
Copy Markdown
Member

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.

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

Sorry, something went wrong.

Uh oh!

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

All reactions
@Linkgoron

Linkgoron commented Oct 30, 2021

Copy link
Copy Markdown
Contributor Author

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

Sorry, something went wrong.

Uh oh!

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

@Linkgoron
Linkgoron force-pushed the repl-no-ispector-fix branch from 04cc504 to daacae8 Compare October 30, 2021 16:44
Trott
Trott reviewed Oct 31, 2021
View reviewed changes
Comment thread test/parallel/test-repl-preview-without-inspector.js Resolved

Uh oh!

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

Trott
Trott approved these changes Oct 31, 2021
View reviewed changes
@Trott

Trott commented Oct 31, 2021

Copy link
Copy Markdown
Member

@nodejs/repl

All reactions

Sorry, something went wrong.

Uh oh!

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

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

nodejs-github-bot commented Nov 1, 2021

Copy link
Copy Markdown
Collaborator

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

All reactions

Sorry, something went wrong.

Uh oh!

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

@github-actions github-actions Bot mentioned this pull request Nov 2, 2021
CI Reliability 2021-11-02 nodejs/reliability#105
Open
35 tasks
@Trott

Trott commented Nov 2, 2021

Copy link
Copy Markdown
Member

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

Linkgoron reacted with rocket emoji
All reactions
  • 🚀 1 reaction

Sorry, something went wrong.

Uh oh!

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

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

nodejs-github-bot commented Nov 2, 2021

Copy link
Copy Markdown
Collaborator

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

All reactions

Sorry, something went wrong.

Uh oh!

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

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
BridgeAR approved these changes Nov 4, 2021
View reviewed changes

@BridgeAR BridgeAR left a comment

Copy link
Copy Markdown
Member

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.

LGTM with my comment addressed.

Sorry, something went wrong.

Uh oh!

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

All reactions
Comment thread lib/internal/repl/utils.js Outdated

@BridgeAR BridgeAR Nov 4, 2021 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Member

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.

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

Sorry, something went wrong.

Uh oh!

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

All reactions

@BridgeAR BridgeAR Nov 29, 2021

Copy link
Copy Markdown
Member

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.

Ping @Linkgoron

Sorry, something went wrong.

Uh oh!

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

Linkgoron reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

@Linkgoron Linkgoron Nov 30, 2021

Copy link
Copy Markdown
Contributor 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.

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

Sorry, something went wrong.

Uh oh!

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

All reactions

@BridgeAR BridgeAR Nov 30, 2021

Copy link
Copy Markdown
Member

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.

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.

Sorry, something went wrong.

Uh oh!

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

All reactions

@Linkgoron Linkgoron Jan 29, 2022 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Contributor 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.

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

Sorry, something went wrong.

Uh oh!

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

All reactions
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
jasnell approved these changes Nov 13, 2021
View reviewed changes
@nodejs-github-bot

nodejs-github-bot commented Nov 14, 2021

Copy link
Copy Markdown
Collaborator

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

All reactions

Sorry, something went wrong.

Uh oh!

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

@nodejs-github-bot

nodejs-github-bot commented Nov 15, 2021

Copy link
Copy Markdown
Collaborator

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

All reactions

Sorry, something went wrong.

Uh oh!

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

This was referenced Nov 16, 2021
CI Reliability 2021-11-16 nodejs/reliability#119
Open
CI Reliability 2021-11-17 nodejs/reliability#120
Open
CI Reliability 2021-11-18 nodejs/reliability#121
Open
CI Reliability 2021-11-19 nodejs/reliability#122
Open
CI Reliability 2021-11-20 nodejs/reliability#123
Open
@Linkgoron
Linkgoron force-pushed the repl-no-ispector-fix branch from 8d24eef to 82e0766 Compare November 30, 2021 14:56
@Linkgoron
Linkgoron force-pushed the repl-no-ispector-fix branch from eba1b6e to fa45fb5 Compare January 31, 2022 09:16
@nodejs-github-bot

nodejs-github-bot commented Jan 31, 2022

Copy link
Copy Markdown
Collaborator

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

All reactions

Sorry, something went wrong.

Uh oh!

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

@nodejs-github-bot

nodejs-github-bot commented Jan 31, 2022

Copy link
Copy Markdown
Collaborator

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

All reactions

Sorry, something went wrong.

Uh oh!

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

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

nodejs-github-bot commented Jan 31, 2022

Copy link
Copy Markdown
Collaborator

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

All reactions

Sorry, something went wrong.

Uh oh!

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

@github-actions github-actions Bot mentioned this pull request Feb 1, 2022
CI Reliability 2022-02-01 nodejs/reliability#189
Open
31 tasks
@nodejs-github-bot

nodejs-github-bot commented Feb 1, 2022

Copy link
Copy Markdown
Collaborator

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

All reactions

Sorry, something went wrong.

Uh oh!

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

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
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
@Linkgoron @aduh95
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
@aduh95
fixup! repl: fix disruptive autocomplete without inspector
5083c3f
@Linkgoron @aduh95
fixup! repl: fix disruptive autocomplete without inspector
dba04ca
@Linkgoron @aduh95
fixup! fixup! repl: fix disruptive autocomplete without inspector
95b5272
@aduh95
aduh95 force-pushed the repl-no-ispector-fix branch from e69ebdf to 95b5272 Compare May 11, 2024 13:41
@aduh95 aduh95 added the author ready PRs with CI started, the required approvals, and no outstanding review comments. label May 11, 2024
@nodejs-github-bot

nodejs-github-bot commented May 11, 2024

Copy link
Copy Markdown
Collaborator

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

All reactions

Sorry, something went wrong.

Uh oh!

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

@nodejs-github-bot

nodejs-github-bot commented May 12, 2024

Copy link
Copy Markdown
Collaborator

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

All reactions

Sorry, something went wrong.

Uh oh!

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

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

aduh95 commented May 12, 2024

Copy link
Copy Markdown
Contributor

Landed in 1223294

All reactions

Sorry, something went wrong.

Uh oh!

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

targos pushed a commit that referenced this pull request May 12, 2024
@Linkgoron @targos
repl: fix disruptive autocomplete without inspector
04de576
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 <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@targos targos mentioned this pull request May 13, 2024
v22.2.0 release proposal #52971
Merged
@marco-ippolito marco-ippolito mentioned this pull request Jun 17, 2024
v20.15.0 proposal #53486
Merged
marco-ippolito pushed a commit that referenced this pull request Jun 17, 2024
@Linkgoron @marco-ippolito
repl: fix disruptive autocomplete without inspector
eceac78
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 <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: James M Snell <[email protected]>
soophoo pushed a commit to soophoo/node that referenced this pull request Jun 20, 2024
@Linkgoron @soophoo
repl: fix disruptive autocomplete without inspector
294a06a
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 <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@BridgeAR BridgeAR BridgeAR approved these changes
@aduh95 aduh95 aduh95 left review comments
@jasnell jasnell jasnell approved these changes
@Trott Trott Trott approved these changes
+1 more reviewer
@antsmartian antsmartian antsmartian 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

author ready PRs with CI started, the required approvals, and no outstanding review comments. needs-ci PRs that need a full CI run. repl Issues and PRs related to the REPL subsystem.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

repl: internal repl's autocomplete is disruptive without inspector

Uh oh!

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

7 participants

@Linkgoron @Trott @nodejs-github-bot @aduh95 @jasnell @antsmartian @BridgeAR
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.