Skip to content

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

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
Merged
merged 1 commit into from
Feb 3, 2022
Merged

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

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.
@wraithgar wraithgar requested a review from a team as a code owner February 1, 2022 22:46
@wraithgar wraithgar force-pushed the gar/log-to-commands branch from cab3368 to cc1e51f Compare February 1, 2022 22:51
@npm-robot
Copy link
Contributor

npm-robot commented Feb 1, 2022 •
edited
Loading

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
@wraithgar
fix(log): pass in logger to external modules
0ee55ad 
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.
@wraithgar wraithgar force-pushed the gar/log-to-commands branch from cc1e51f to 0ee55ad Compare February 2, 2022 14:44
@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
Merged
@npm-robot npm-robot mentioned this pull request Feb 3, 2022
Merged
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

3 participants

@wraithgar @npm-robot @ruyadorno