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: move all definitions to @npmcli/config package #6497

Merged
merged 1 commit into from
Jun 7, 2023

Conversation

lukekarrys
Copy link
Contributor

@lukekarrys lukekarrys commented May 24, 2023

This builds on #6490 to avoid conflicts with the config changes there.

Marking this as a draft until that lands, then this can be rebased if necessary and marked as ready for review.

This is ready

@lukekarrys lukekarrys requested a review from a team as a code owner May 24, 2023 22:30
@lukekarrys lukekarrys marked this pull request as draft May 24, 2023 22:30
@lukekarrys lukekarrys force-pushed the lk/move-definitions-to-config branch 4 times, most recently from 1ad829d to f9468c5 Compare May 25, 2023 02:14
@lukekarrys lukekarrys marked this pull request as ready for review May 30, 2023 17:12
Base automatically changed from bdehamer/provenance-path to latest May 31, 2023 17:37
@lukekarrys lukekarrys force-pushed the lk/move-definitions-to-config branch from f9468c5 to 098c450 Compare May 31, 2023 17:54
@npm-cli-bot
Copy link
Collaborator

npm-cli-bot commented May 31, 2023

found 1 benchmarks with statistically significant performance improvements

  • app-large: clean
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 39.254 ±1.75 17.043 ±0.14 16.552 ±0.04 19.297 ±0.60 2.706 ±0.03 2.691 ±0.02 2.211 ±0.02 11.381 ±0.01 2.209 ±0.00 3.430 ±0.13 0.368 ±0.00 0.424 ±0.01
#6497 34.551 ±0.70 17.567 ±0.48 16.643 ±0.21 20.024 ±0.50 2.775 ±0.06 2.724 ±0.02 2.284 ±0.03 11.717 ±0.12 2.255 ±0.02 3.399 ±0.07 0.382 ±0.00 0.429 ±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
show-version run-script
npm@9 25.342 ±1.21 13.361 ±0.12 12.815 ±0.00 13.590 ±0.02 2.404 ±0.01 2.394 ±0.01 2.197 ±0.03 8.618 ±0.10 2.117 ±0.01 2.903 ±0.07 0.365 ±0.01 0.412 ±0.00
#6497 24.911 ±0.45 13.421 ±0.12 12.944 ±0.08 13.793 ±0.06 2.430 ±0.04 2.416 ±0.05 2.171 ±0.02 8.686 ±0.07 2.097 ±0.03 2.927 ±0.02 0.372 ±0.00 0.427 ±0.00

@matz3
Copy link

matz3 commented Jun 5, 2023

@lukekarrys does this PR change the fundamental message of the package?

It does the management of configuration files that npm uses, but importantly, does not define all the configuration defaults or types, as those parts make more sense to live within the npm CLI itself.

We're looking for the best approach to move from libnpmconfig to @npmcli/config for a use case where we download packages via pacote and want to ensure to take the user configuration into account.

@lukekarrys
Copy link
Contributor Author

lukekarrys commented Jun 5, 2023

does this PR change the fundamental message of the package?

good catch! yes, with this change that statement is no longer true. we've found it is best to have all the config definitions live alongside the logic in @npmcli/config.

i haven't taken a look at updating the docs yet, since i have a larger refactor in process that will contain a few breaking changes, and was updating the documentation in that branch.

this PR is a stop-gap to implement the diff-heavy work of moving the config files, without changing anything about the implementation.

We're looking for the best approach to move from libnpmconfig to @npmcli/config

one of the goals of the next major version, will be to limit what is required to be passed in to new Config(opts), so that should be helpful to your use case. i don't have any definitive timeline yet for when that might land yet.

Copy link
Member

@wraithgar wraithgar left a comment

Choose a reason for hiding this comment

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

We're getting close!

@lukekarrys lukekarrys force-pushed the lk/move-definitions-to-config branch from f1c826a to 39cb7bb Compare June 6, 2023 21:56
@lukekarrys lukekarrys force-pushed the lk/move-definitions-to-config branch 2 times, most recently from 07dd35e to c9b7f8f Compare June 7, 2023 17:50
@lukekarrys lukekarrys merged commit e722439 into latest Jun 7, 2023
@lukekarrys lukekarrys deleted the lk/move-definitions-to-config branch June 7, 2023 18:28
@github-actions github-actions bot mentioned this pull request Jun 7, 2023
@d3xter666
Copy link

Hi @lukekarrys,

Do you plan a new release with those changes?

Cheers

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants