Skip to content

Conversation

@reggi
Copy link
Contributor

@reggi reggi commented Jan 5, 2026

Alternate version of #8776

npm cli config changes

  • config controls all user input / options to the cli from numerous sources
  • currently the config only allows for global cli flags, this causes a phenomenon historically where we started to prefix certain flags with a command name like --init-author
  • The main issue is warnings, recent versions of npm warn when an unknown config value is used in any source. The issue being that we need to process the argv positional to route the command and get the command-level-config, and not log.warn on that first pass...

Original Solution

  • Added a new flags source
  • Queue all warnings
  • Allowed for the ability to load in definitions post first load
  • Removed warnings "solved" by the newly loaded definitions file
  • Not isolated from the global config
  • rc / ini files always get loaded into config (even if they're not in definitions)

New Plan

  • Queue warnings ✅
  • Ability to remove a queued warning based on command definitions ✅
  • In BaseCommand we have a flags method that uses nopt on the command definitions and simply returns the value.
  • Command .exec can just call this.flags() and get command level flags.

@reggi reggi requested a review from a team as a code owner January 5, 2026 21:56
@reggi reggi changed the title Isolated Command Flags fix: per-command config Jan 5, 2026
edmistonnicolas-ctrl referenced this pull request Jan 6, 2026
…arison (#8689)

Resolves the issue where different override contexts (like Vaadin's
$@vaadin/react-components vs $@vaadin/react-components-pro) were
incorrectly treated as conflicts by structural comparison.

Fixes #8688
Copy link
Contributor

@owlstronaut owlstronaut left a comment

Choose a reason for hiding this comment

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

This looks great to me

Comment on lines +238 to +239
// this needs to be rest after because some commands run
// this.npm.config.checkUnknown('publishConfig', key)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// this needs to be rest after because some commands run
// this.npm.config.checkUnknown('publishConfig', key)
// this needs to be reset after because some commands run `this.npm.config.checkUnknown('publishConfig', key)`

Copy link
Contributor

Choose a reason for hiding this comment

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

mostly rest->reset

Comment on lines +40 to +41
#argv = undefined
#excludeNpmCwd = undefined
Copy link
Contributor

Choose a reason for hiding this comment

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

Are these used somewhere?

@ljharb
Copy link
Contributor

ljharb commented Jan 6, 2026

just to clarify with this new PR - does this PR include a way for users to specify per-command configs?

If not, how would a user override any built-in per-command configuration?

@reggi
Copy link
Contributor Author

reggi commented Jan 8, 2026

@ljharb

just to clarify with this new PR - does this PR include a way for users to specify per-command configs?

no

If not, how would a user override any built-in per-command configuration?

new command definitions are flag-only for now on a case by case basis we could have prefixed long globals exist

@reggi reggi marked this pull request as draft January 8, 2026 23:36
@reggi
Copy link
Contributor Author

reggi commented Jan 8, 2026

drafting this PR because this has some issues i've fixed them locally and am working them out

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.

3 participants