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.

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

fix(log): pass in logger to external modules #4359

Merged
merged 1 commit into from
Feb 3, 2022

Conversation

wraithgar
Copy link
Member

Most of these module use npm-registry-fetch under the hood, which will
log things like the npm-notice header if seen. Currently we aren't
passing in a logger to them, which means that log message is never seen,
among any other logged messages those modules may need to make.

I added tests where I could. Some tests were in a state where the entire
libnpm* module was an empty mocked function, so asserting that it got
passed a log attribute was onerous.

@npm-robot
Copy link
Contributor

npm-robot commented Feb 1, 2022

found 20 benchmarks with statistically significant performance improvements

  • app-large: clean, lock-only, cache-only, cache-only:peer-deps, modules-only, no-lock, no-cache, no-modules, no-clean, no-clean:audit
  • app-medium: clean, lock-only, cache-only, cache-only:peer-deps, modules-only, no-lock, no-cache, no-modules, no-clean, no-clean:audit
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
npm@8 56.306 ±0.53 36.000 ±0.18 22.488 ±0.09 25.277 ±0.56 3.635 ±0.04 3.659 ±0.14 2.914 ±0.05 14.984 ±0.00 2.881 ±0.03 4.079 ±0.09
#4359 0.479 ±0.00 0.522 ±0.03 0.482 ±0.00 0.481 ±0.01 0.485 ±0.01 0.486 ±0.03 0.490 ±0.00 0.517 ±0.02 0.492 ±0.00 0.486 ±0.01
app-medium clean lock-only cache-only cache-only
peer-deps
modules-only no-lock no-cache no-modules no-clean no-clean
audit
npm@8 43.522 ±2.12 27.296 ±0.00 16.257 ±0.08 17.641 ±0.02 3.356 ±0.01 3.297 ±0.08 2.947 ±0.08 11.051 ±0.30 2.763 ±0.03 3.631 ±0.10
#4359 0.483 ±0.01 0.470 ±0.01 0.462 ±0.00 0.504 ±0.00 0.504 ±0.03 0.505 ±0.01 0.469 ±0.00 0.491 ±0.01 0.480 ±0.01 0.495 ±0.01

Most of these module use npm-registry-fetch under the hood, which will
log things like the `npm-notice` header if seen.  Currently we aren't
passing in a logger to them, which means that log message is never seen,
among any other logged messages those modules may need to make.

I added tests where I could. Some tests were in a state where the entire
libnpm* module was an empty mocked function, so asserting that it got
passed a `log` attribute was onerous.
@ruyadorno ruyadorno added Release 8.x work is associated with a specific npm 8 release release: next These items should be addressed in the next release semver:patch semver patch level for changes labels Feb 3, 2022
@wraithgar wraithgar merged commit 1b93385 into release-next Feb 3, 2022
@wraithgar wraithgar deleted the gar/log-to-commands branch February 3, 2022 14:43
@ruyadorno ruyadorno mentioned this pull request Feb 3, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release: next These items should be addressed in the next release Release 8.x work is associated with a specific npm 8 release semver:patch semver patch level for changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants