disable REPL completion on proxies and getters

[dario-piotrowicz]

the REPL completion logic evaluates code, this is generally ok
but it can be problematic when there are objects with nested
properties since the code evaluation would trigger their potential
getters (e.g. the obj.foo.b line would trigger the getter of
foo), the changes here disable the completion logic when proxies
and getters are involved thus making sure that code evaluation does
not take place when it can potentially (and surprisingly to the user)
trigger side effectful behaviors

---

Partially addresses #57829

[BridgeAR]

Instead of deactivating the completion completely for nested properties, we could check for the object being proxied or if the property is a getter and to only bail out in those two cases.

That would keep the completion on in most cases.

[dario-piotrowicz]

Instead of deactivating the completion completely for nested properties, we could check for the object being proxied or if the property is a getter and to only bail out in those two cases.

That would keep the completion on in most cases.

yeah that's a good call, let me have a look 👀 👍

[codecov]

Codecov Report

Attention: Patch coverage is 98.85057% with 1 line in your changes missing coverage. Please review.

Project coverage is 90.24%. Comparing base (968e2f4) to head (0f1f1e4).
Report is 73 commits behind head on main.

Files with missing lines	Patch %	Lines
lib/repl.js	98.85%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #57909      +/-   ##
==========================================
+ Coverage   90.21%   90.24%   +0.02%     
==========================================
  Files         635      635              
  Lines      187580   187537      -43     
  Branches    36853    36855       +2     
==========================================
+ Hits       169231   169240       +9     
+ Misses      11108    11065      -43     
+ Partials     7241     7232       -9     

Files with missing lines	Coverage Δ
lib/repl.js	95.13% <98.85%> (+0.11%)	⬆️

... and 42 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[dario-piotrowicz]

Thanks for the suggestion @BridgeAR, I've implemented the solution you proposed and this indeed seems to be a much nicer solution (especially DX wise) 🫶

However I feel a bit uneasy with the code changes I needed to make in the tests to get them to pass locally (I wonder how they'll perform in CI, I generally haven't had much luck with REPL timing issues in CI as of late 😓), me making complete async and running multiple evals will for sure slow things down to a certain degree, for a REPL this increased delay feels acceptable to me but I'd love to hear your thoughts on it

[nodejs-github-bot]

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

[BridgeAR]

This definitely seems like a good implementation and it does seem like it improves the situation, while not matching all cases.

I did not look at the tests, so I can't yet say much about them.

[BridgeAR]

LGTM! Thanks for the follow-up. I just left two minor suggestions.

The tests would probably not need the refactoring right now, due to the sync implementation while it's probably a good idea to be prepared already (I guess we might switch to a async one at some point).

[nodejs-github-bot]

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

[nodejs-github-bot]

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

[nodejs-github-bot]

Landed in 1093f38

[aduh95]

This doesn't land cleanly on v22.x-staging, it would require to be manually backported alongside the other REPL changes