repl: move complete to its own file

[RedYetiDev]

This PR moves a large function from repl.js into it's own file, internal/repl/complete.js. This is an attempt to slowly declutter the file.

[codecov]

Codecov Report

Attention: Patch coverage is 97.90476% with 11 lines in your changes missing coverage. Please review.

Project coverage is 88.08%. Comparing base (7014e50) to head (5a62c67).
Report is 272 commits behind head on main.

Files with missing lines	Patch %	Lines
lib/internal/repl/complete.js	97.70%	8 Missing and 3 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #54989      +/-   ##
==========================================
+ Coverage   88.06%   88.08%   +0.01%     
==========================================
  Files         652      653       +1     
  Lines      183545   183705     +160     
  Branches    35854    35853       -1     
==========================================
+ Hits       161648   161818     +170     
+ Misses      15146    15139       -7     
+ Partials     6751     6748       -3     

Files with missing lines	Coverage Δ
lib/internal/repl/utils.js	95.92% <100.00%> (+0.18%)	⬆️
lib/repl.js	93.97% <100.00%> (-0.90%)	⬇️
lib/internal/repl/complete.js	97.70% <97.70%> (ø)

... and 28 files with indirect coverage changes

[RedYetiDev]

@nodejs/repl

[anonrig]

Moving these to new files will have an impact on node.js startup time.

[RedYetiDev]

Moving these to new files will have an impact on node.js startup time.

@anonrig How much of an impact? It is negligible?

[aduh95]

Moving these to new files will have an impact on node.js startup time.

Why would that be?

[anonrig]

Please run the startup benchmark for the impact.

[RedYetiDev]

I don't think startup-core covers the REPL, and given the drastic changes and both directions, I don't believe this is an accurate benchmark:

                                                                                       confidence improvement accuracy (*)   (**)  (***)
misc/startup-core.js n=30 mode='process' script='benchmark/fixtures/require-builtins'        ***     -6.71 %       ±1.43% ±1.92% ±2.53%
misc/startup-core.js n=30 mode='process' script='test/fixtures/semicolon'                    ***     -7.85 %       ±1.26% ±1.67% ±2.18%
misc/startup-core.js n=30 mode='process' script='test/fixtures/snapshot/typescript'            *     -1.22 %       ±1.04% ±1.38% ±1.79%
misc/startup-core.js n=30 mode='worker' script='benchmark/fixtures/require-builtins'         ***      5.81 %       ±1.91% ±2.55% ±3.32%
misc/startup-core.js n=30 mode='worker' script='test/fixtures/semicolon'                     ***      9.23 %       ±1.28% ±1.70% ±2.22%
misc/startup-core.js n=30 mode='worker' script='test/fixtures/snapshot/typescript'                    0.88 %       ±1.63% ±2.17% ±2.84%

Be aware that when doing many comparisons the risk of a false-positive
result increases. In this case, there are 6 comparisons, you can thus
expect the following amount of false-positive results:
  0.30 false positives, when considering a   5% risk acceptance (*, **, ***),
  0.06 false positives, when considering a   1% risk acceptance (**, ***),
  0.01 false positives, when considering a 0.1% risk acceptance (***)

[RedYetiDev]

Hey, any more notes on the performance of this change? I don't think there's a perf impact IMO.

[jasnell]

Not really seeing a lot of value in this change

[RedYetiDev]

Why? The REPL file is giant, wouldn't de-cluttering it be a good thing? IMHO we don't need all these functions in a single file.

[anonrig]

Agree with @jasnell. There is no benefit of this change.

[RedYetiDev]

Because you insist, I'm closing this.