fix: clear exclusive param siblings when setting from CLI#9023
fix: clear exclusive param siblings when setting from CLI#9023umeshmore45 wants to merge 2 commits intonpm:latestfrom
Conversation
|
Thanks for working on this @umeshmore45 — the approach of fixing it generically in set-envs.js for all exclusive params is the right direction per @wraithgar's feedback. However, the bug is still reproducible with this PR applied. The core issue is what the child process sees when pacote prepares a git dep, which you can test directly: npm_config_min_release_age=2 node bin/npm-cli.js install --before=2026-02-23T00:00:00.000Z --dry-run
# => --min-release-age cannot be provided when using --beforeThis fails identically with and without the PR. The fix resets npm_config_before="" in the parent's env, but the child's loadEnv skips empty strings, so it has no effect. The actual conflict is between npm_config_min_release_age (still in env) and --before (CLI arg added by pacote) — the fix clears the sibling, but the original config causing the conflict is untouched. |
|
Thanks for the detailed explanation. I see the distinction now. My However, I also have a fix in I tested this with a git dependency ( Could you try reproducing the failure with both changes applied ( |
| for (const exclusive of this.definitions[key].exclusive) { | ||
| if (!this.isDefault(exclusive)) { | ||
| // when loading from env, skip exclusive check — already-set values take precedence | ||
| if (where === 'env') { |
There was a problem hiding this comment.
I think this is too broad. it'll silently accept npm_config_before=2026-01-01 npm_config_min_release_age=2 npm install
|
This is a tricky one because realistically There is a flag that should solve this already: Setting it to |
|
Hey, quick question on an edge case I ran into while testing. Right now with my changes, Should this still be an error? If so, I can add a separate validation step post-load rather than catching it during env loading. Let me know how you'd like me to handle this. |
Added logic to skip exclusive option checks when an environment variable is set if a sibling option was provided via the command line. This ensures that environment configurations do not conflict with CLI arguments. Additionally, a new test case was introduced to verify this behavior.
4fcd783 to
9dbc055
Compare
|
Yeah, that should still be an error, the user is explicitly setting two conflicting options. The current check is still too broad because |
Refined logic to ensure that exclusive options from the environment are correctly skipped when a sibling option is set via the command line. This change prevents conflicts between environment variables and CLI arguments. Additionally, updated test cases to validate the new behavior and ensure proper functionality.
|
Hey, I’ve updated everything based on your feedback Changes
Tested
Let me know if anything else should be adjusted. |
fix: clear exclusive sibling configs from env when one is set via CLI
What's the problem?
If you set an exclusive param via CLI (e.g.
--save-prod) but a sibling(
npm_config_save_dev=true) is already in the environment, child processesinherit both and crash with a conflict. This was also the root cause of the
--min-release-age+--beforeissue in #9005.What changed
When
setEnvsexports a non-default exclusive config, it now resets thatparam's siblings to their defaults in the env — so child processes never
see a conflict. Works generically for all exclusive pairs, not just this one.
Tests
Added a test for the case where
save-prodis set via CLI whilesave-devis already in env — verifies
save-devgets reset to its default.References
Fixes #9005