node icon indicating copy to clipboard operation
node copied to clipboard

repl: fix tab completion not working with computer string properties

Open dario-piotrowicz opened this issue 6 months ago • 3 comments

I noticed that the repl tab completion doesn't correctly handle computed properties targeted by strings.

For example given the following context:

const obj = { one: 1 };

The current repl would provide valid (number) completions for lines such as:

obj.one.to

But would not provide correct completions for lines such as:

obj['one'].to

In fact it would provide incorrect (string) completions.

See: Screenshot at 2025-06-15 01-03-50

This PR addresses the above issue

[!Note] This PR is not making everything work perfectly, for example it is not addressing lines such as obj['o' + 'ne'].to (but it's not making them any worse either). I think that a generally great solution would require more substantial code changes (potentially with some AST analysis). I am planning on keeping improving things here but I figured I could do it incrementally and that I'd open this PR to start moving things in the right direction anyways.

dario-piotrowicz avatar Jun 15 '25 00:06 dario-piotrowicz

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 90.13%. Comparing base (d6dade5) to head (da0d780). Report is 4 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main   #58709   +/-   ##
=======================================
  Coverage   90.12%   90.13%           
=======================================
  Files         637      637           
  Lines      188121   188121           
  Branches    36892    36892           
=======================================
+ Hits       169552   169568   +16     
- Misses      11313    11316    +3     
+ Partials     7256     7237   -19     
Files with missing lines Coverage Δ
lib/repl.js 94.87% <100.00%> (-0.26%) :arrow_down:

... and 23 files with indirect coverage changes

:rocket: New features to boost your workflow:
  • :snowflake: Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • :package: JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

codecov[bot] avatar Jun 15 '25 01:06 codecov[bot]

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

nodejs-github-bot avatar Jun 15 '25 21:06 nodejs-github-bot

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

nodejs-github-bot avatar Jun 15 '25 22:06 nodejs-github-bot

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

nodejs-github-bot avatar Jun 16 '25 08:06 nodejs-github-bot

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

nodejs-github-bot avatar Jun 16 '25 10:06 nodejs-github-bot

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

nodejs-github-bot avatar Jun 16 '25 11:06 nodejs-github-bot

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

nodejs-github-bot avatar Jun 16 '25 13:06 nodejs-github-bot

Landed in 07220230d9effcb4822d7a55563a86af3764540c

nodejs-github-bot avatar Jun 17 '25 00:06 nodejs-github-bot

We'd better use acorn to detect these things in the future though. We are for example still missing support for whitespace in-between.

I 100% agree, as I mentioned in the PR description this was a first fix I figured I could already land (it also already includes some new tests which will help assuring that changes later work as intended)

I'll most likely be using acron on my next iteration here, also I really really want to get rid of this regex: https://github.com/nodejs/node/blob/da0d780c157872c2455fa954d5c5cd4aa395fbfd/lib/repl.js#L1228-L1229

:grin:

(since it's very complex and not even fully correct)


We also miss support for partial matches inside of a bracket. That would also be great.

Can you elaborate? :slightly_smiling_face:

dario-piotrowicz avatar Jun 17 '25 09:06 dario-piotrowicz