fix: issue-5280 Ensure save flags update package.json

[ficocelliguy]

closes #5280

Ensure that flags like --save-dev imply --save, and are not overridden by npm config.

Currently, the npm config 'save' flag takes precedence, meaning that users who have that config set to false cannot update their package.json even if specifically using --save-dev or similar, unlike how package-lock-only implies package-lock.

This change ensures that save-exact, save-dev, save-prod, save-optional, and save-peer all imply 'save'

[ficocelliguy]

@wraithgar apologies for the ping. Is there a process for getting CI workflows approved? The test and lint targets pass in my local, but I'd like to make sure this and #7208 are clean and ready to go to avoid wasting reviewer's time.

[ljharb]

I agree that save-dev should imply save - so too should save-peer, save-prod, and any other save commands there.

[wraithgar]

I'm adding comments to the PR so I can triple check we're setting the flag only on the config items we want to, they don't require any response from you.

[wraithgar]

Save is set to true when user specifies:

• save-dev
• save-optional
• save-peer
• save-prod

It is not changed when user specifies:

• save-exact
• save-prefix
• save-bundle

This looks correct.

[wraithgar]

Test failure brings up a good point. What if save is true and folks do --save-dev=false?

In the test save is true because of a previous parsing of --save-dev=true but I think the question still stands. Should we be clearing flatOptions.save if they set a --save-xxx to false? Because save is parsed before these, we'd be undoing any specific setting of that value already done.

I think the use case here is if they have save-dev=true in an .npmrc file, and then try to turn that off via cli flag.

[ljharb]

It seems like the implicit "save" should be applied as the last step, based on whatever save-category flags end up enabled?

[ficocelliguy]

@ljharb where would a good place be to conditionally apply that implied save as a last step?

[ljharb]

That's a good question; i'd assume that whatever's populating the config object could do it, but i don't know where that is off the top of my head.

[ficocelliguy]

I have moved the logic to be contained as a last step in the flatOptions getter. I have tied the presence of a 'saveType' to the save flag, meaning we only set it to true if a save type is actually present after all of the configs and cli flags are parsed.

Note that this does not clear save if they set --save-dev to false etc; setting that to false will clear out the saveType , though, preventing the new logic from running. I believe this will handle the .npmrc example you mentioned, @wraithgar

[wraithgar]

This still bypasses the explicit --no-save

~/D/n/s/save $ npm pkg get devDependencies
{}
~/D/n/s/save $ node /Users/wraithgar/Development/npm/cli/branches/main i lodash --no-save

added 1 package, and audited 2 packages in 719ms

found 0 vulnerabilities
~/D/n/s/save $ npm pkg get devDependencies
{
  "lodash": "^4.17.21"
}
~/D/n/s/save $ cat .npmrc
save-dev=true

BUT the consolidation you did was perfect because it allows you do so a single check to know whether or not you should overwrite save:

~/D/n/c/b/main (5280_ensure_save_flags_imply_save|✔) $ git diff
diff --git a/workspaces/config/lib/index.js b/workspaces/config/lib/index.js
index 20efdcf38..a483f3af4 100644
--- a/workspaces/config/lib/index.js
+++ b/workspaces/config/lib/index.js
@@ -235,7 +235,9 @@ class Config {
 
     // Ensure 'save' is true if a saveType has been specified
     if (this.#flatOptions.saveType) {
-      this.#flatOptions.save = true
+      if (!this.isDefault('save')) {
+        this.#flatOptions.save = true
+      }
     }

[wraithgar]

Actually now that I think about it that may circle us back around to the original problem. I believe that npm applies config in order (so a flag on the cli will apply after a project level npmrc directive) and we can probably start there. This is not a simple problem to fix, but I think it is fixable.

[ficocelliguy]

simply detecting that the save config comes from default wasn't enough (for example, setting save to false from env would be non-default, and thus set save to true, which is not what we want) so I think we need to keep the saveType check.

This seems to handle overwriting env, user, and global save:false configs whenever a saveType-setting flag is passed in (and that saveType flag is not set to false)

Additionally, to handle a command with both --save-dev --no-save , we have to look through the passed argv to ensure the user isn't manually preventing save (to maintain current functionality in that case). There isn't another way to detect --no-save, as far as I can tell, since simply looking for save:false could be coming from environment etc in addition to the cli flag

[wraithgar]

I think for now having both --save-dev and --no-save on the cli can be something we don't support. That's not a "valid" use case, like the one where we've set save to false in a config, and want to override it on the cli.

[ficocelliguy]

Does that mean I should remove the argv check and test case from the implementation, and let --no-save be ignored if a saveType flag is also provided?

[ficocelliguy]

I have updated this PR to remove the special --no-save handling, per the above comments.