repl: fix tab completion not working with computer string properties
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:
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.
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: |
: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.
CI: https://ci.nodejs.org/job/node-test-pull-request/67470/
CI: https://ci.nodejs.org/job/node-test-pull-request/67471/
CI: https://ci.nodejs.org/job/node-test-pull-request/67477/
CI: https://ci.nodejs.org/job/node-test-pull-request/67478/
CI: https://ci.nodejs.org/job/node-test-pull-request/67479/
CI: https://ci.nodejs.org/job/node-test-pull-request/67481/
Landed in 07220230d9effcb4822d7a55563a86af3764540c
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: