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: issue-5280 Ensure save flags update package.json #7233

Open
wants to merge 6 commits into
base: latest
Choose a base branch
from

Conversation

ficocelliguy
Copy link

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 ficocelliguy requested a review from a team as a code owner February 19, 2024 23:49
@ficocelliguy ficocelliguy changed the title [npm#5280] Ensure save flags update package.json fix: issue-5280 Ensure save flags update package.json Feb 19, 2024
@ficocelliguy
Copy link
Author

@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
Copy link
Contributor

ljharb commented Feb 19, 2024

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

@@ -1813,6 +1813,7 @@ define('save-dev', {
return
}

flatOptions.save = true
Copy link
Member

Choose a reason for hiding this comment

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

save-dev

@wraithgar
Copy link
Member

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.

@@ -1849,6 +1850,8 @@ define('save-optional', {
return
}

flatOptions.save = true
Copy link
Member

Choose a reason for hiding this comment

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

save-optional

@@ -1877,6 +1880,8 @@ define('save-peer', {
return
}

flatOptions.save = true
Copy link
Member

Choose a reason for hiding this comment

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

save-peer

@@ -1928,6 +1933,7 @@ define('save-prod', {
return
}

flatOptions.save = true
Copy link
Member

Choose a reason for hiding this comment

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

save-prod

@wraithgar
Copy link
Member

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
Copy link
Member

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
Copy link
Contributor

ljharb commented Feb 28, 2024

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

@ficocelliguy
Copy link
Author

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

@ljharb
Copy link
Contributor

ljharb commented Mar 1, 2024

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
Copy link
Author

ficocelliguy commented Mar 1, 2024

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
Copy link
Member

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 wraithgar self-assigned this Mar 4, 2024
@wraithgar
Copy link
Member

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
Copy link
Author

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
Copy link
Member

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
Copy link
Author

ficocelliguy commented Mar 5, 2024

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
Copy link
Author

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

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.

[BUG] npm --save-dev does not infer --save
3 participants