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

Configuration Definition Refactor, Phase 1 #2878

Merged
merged 1 commit into from
Mar 18, 2021

Conversation

isaacs
Copy link
Contributor

@isaacs isaacs commented Mar 16, 2021

This is the first step in refactoring our configuration definition, flattening, and documentation.

Still TODO:

  • Remove all use of npm.flatOptions in lib/cli, other than passing to dependencies. (@wraithgar is working on this in another PR)
  • Audit every dependency's use of every option key, so that we know which things are used in which ways. (And so that we can stop flattening options just for our own internal usage, since we can access config.get() which is fine.)
  • Categorize config keys into groups for the purpose of easily documenting which configs are used by which commands.
  • Identify (ie, in the config definition) which flatOptions are affected by which config fields.
  • Move back to a single place where flatOptions are flattened (so that logic isn't spread across multiple config definitions in cases like saveType)

Furthermore, explore whether the flatOptions dirty flag should be "you need to regenerate all of these", or a dirty flag for the individual fields that might have changed, or just an object full of getters for each one. The current approach (generate flat options if missing, throw away when config.set() changes anything) is naive, but simple and probably performant enough.

@isaacs isaacs requested a review from a team as a code owner March 16, 2021 00:28
@isaacs isaacs force-pushed the isaacs/config-refactor branch 4 times, most recently from 08161ea to 9013367 Compare March 16, 2021 00:50
@isaacs
Copy link
Contributor Author

isaacs commented Mar 16, 2021

This is ready to review for basic structure and approach, but there are a few blockers to landing it:

  • Should remove all use of npm.flatOptions from lib/*.js, except for passing to other deps.
  • Finish audit of config field usage (this is also turning up a few errors which will be fixed shortly).
  • Land changes to node_modules/@npmcli/config upstream and publish a major, so that when we pull it in, it's a "clean fork" from the published module before making any other changes.

@darcyclarke darcyclarke added the Release 7.x work is associated with a specific npm 7 release label Mar 17, 2021
@isaacs isaacs force-pushed the isaacs/config-refactor branch 4 times, most recently from d08ed0a to 0206855 Compare March 18, 2021 18:58
@isaacs isaacs changed the base branch from latest to release-next March 18, 2021 18:59
@isaacs isaacs merged commit aaafab8 into release-next Mar 18, 2021
wallrat added a commit to wallrat/cli that referenced this pull request Mar 28, 2021
The default value for 'maxsockets' was changed during the refactoring in npm#2878 from 50 to 'Inifinity', this PR changes it back to the previous value of 50.

Fixes npm#2978
ruyadorno pushed a commit to wallrat/cli that referenced this pull request Mar 29, 2021
The default value for 'maxsockets' was changed during the refactoring
in npm#2878 from 50 to 'Inifinity', this PR changes it to the more
accurate value of 15, which was the default used in:
https://github.com/npm/make-fetch-happen/blob/785af652ec0c8f108a43004903afd2183af93904/agent.js#L15

Fixes npm#2978

PR-URL: npm#2979
Credit: @wallrat
Close: npm#2979
Reviewed-by: @ruyadorno

Co-authored-by: Gar <gar+gh@danger.computer>
ruyadorno pushed a commit that referenced this pull request Mar 29, 2021
The default value for 'maxsockets' was changed during the refactoring
in #2878 from 50 to 'Inifinity', this PR changes it to the more
accurate value of 15, which was the default used in:
https://github.com/npm/make-fetch-happen/blob/785af652ec0c8f108a43004903afd2183af93904/agent.js#L15

Fixes #2978

PR-URL: #2979
Credit: @wallrat
Close: #2979
Reviewed-by: @ruyadorno

Co-authored-by: Gar <gar+gh@danger.computer>
@nlf nlf deleted the isaacs/config-refactor branch March 28, 2022 17:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Release 7.x work is associated with a specific npm 7 release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants