Skip to content
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.

Sign up for GitHub

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

Jump to bottom

streamlines #6304

Merged
merged 2 commits into from
Mar 30, 2023
Merged

streamlines #6304

merged 2 commits into from
Mar 30, 2023

Conversation

wraithgar
Copy link
Member

see individual commits for details on each change

  • fix: do less work loading ./lib/npm.js.
  • fix: don't break up log message across lines

@wraithgar wraithgar requested a review from a team as a code owner March 29, 2023 21:18
@wraithgar wraithgar requested review from lukekarrys and removed request for a team March 29, 2023 21:18
nlf
nlf suggested changes Mar 29, 2023
View reviewed changes
Copy link
Contributor

@nlf nlf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just double reviewing because it's the end of the day and i want to remember to re-review this in the morning when it's fixed

@npm-cli-bot
Copy link
Collaborator

npm-cli-bot commented Mar 29, 2023 •
edited
Loading

no statistically significant performance changes detected

timing results
app-large clean lock-only cache-only cache-only
peer-deps		 modules-only no-lock no-cache no-modules no-clean no-clean
audit		 show-version run-script
npm@9 42.373 ±0.60 19.137 ±0.12 17.944 ±0.16 20.664 ±0.85 3.197 ±0.07 3.157 ±0.02 2.472 ±0.01 12.076 ±0.02 2.476 ±0.08 3.669 ±0.25 0.502 ±0.00 0.517 ±0.00
#6304 44.994 ±4.14 18.750 ±0.14 17.011 ±0.00 19.978 ±0.53 3.123 ±0.05 3.127 ±0.01 2.423 ±0.03 11.978 ±0.02 2.415 ±0.05 3.688 ±0.16 0.534 ±0.00 0.532 ±0.00
app-medium clean lock-only cache-only cache-only
peer-deps		 modules-only no-lock no-cache no-modules no-clean no-clean
audit		 show-version run-script
npm@9 29.365 ±0.83 14.036 ±0.26 13.126 ±0.29 14.174 ±0.72 2.848 ±0.04 2.917 ±0.07 2.477 ±0.02 8.945 ±0.02 2.311 ±0.03 3.239 ±0.03 0.516 ±0.00 0.518 ±0.01
#6304 31.183 ±3.13 14.612 ±0.31 13.494 ±0.04 14.979 ±0.17 2.912 ±0.01 2.910 ±0.01 2.493 ±0.04 9.191 ±0.10 2.425 ±0.04 3.270 ±0.03 0.532 ±0.01 0.540 ±0.00

@wraithgar
fix: do less work loading ./lib/npm.js
68bf5f3 
Wrapping logging messages is very inacessible and makes for bad logs.
We can let terminals wrap content.

The whole cli.js file needs to parse, not just the single function
checking for compatibility, so we inline it again.
@wraithgar
fix: don't break up log message across lines
fbb6e94
@wraithgar wraithgar merged commit 829503b into latest Mar 30, 2023
@wraithgar wraithgar deleted the gar/cli-streamline branch March 30, 2023 16:49
@github-actions github-actions bot mentioned this pull request Mar 30, 2023
Merged
@npm-cli-bot npm-cli-bot mentioned this pull request Mar 30, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nlf nlf nlf approved these changes

@lukekarrys lukekarrys Awaiting requested review from lukekarrys lukekarrys is a code owner automatically assigned from npm/cli-team

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@wraithgar @npm-cli-bot @nlf