-
Notifications
You must be signed in to change notification settings - Fork 3.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: refactor terminal display #7339
Conversation
From what I saw, this will help the initialization like my refactoring for One thing that I noticed you didn't call the |
4677f8b
to
d162325
Compare
8791b08
to
7bc5682
Compare
…o it again in the future
The formatter will properly convert for logfiles
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.
Reviewed this while still in draft mode, and reviewed changes as we went.
I also triple checked that we really do only need to strip C0 and C1 from strings now.
With this PR merged: $ hyperfine --warmup 3 "./bin/npm-cli.js run empty" "~/.asdf/installs/nodejs/21.6.2/bin/node ./bin/npm-cli.js run empty"
Benchmark 1: ./bin/npm-cli.js run empty
Time (mean ± σ): 155.1 ms ± 1.3 ms [User: 141.4 ms, System: 46.0 ms]
Range (min … max): 152.1 ms … 157.0 ms 19 runs
Benchmark 2: ~/.asdf/installs/nodejs/21.6.2/bin/node ./bin/npm-cli.js run empty
Time (mean ± σ): 77.2 ms ± 0.7 ms [User: 71.0 ms, System: 17.8 ms]
Range (min … max): 75.8 ms … 79.2 ms 38 runs
Summary
'~/.asdf/installs/nodejs/21.6.2/bin/node ./bin/npm-cli.js run empty' ran
2.01 ± 0.03 times faster than './bin/npm-cli.js run empty' Great work! |
Tracking remaining todos here:
display.js
(was waiting for API to be stable enough)util.format