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: --lockfile-version to handle string number #3900

Closed
wants to merge 1 commit into from
Closed

fix: --lockfile-version to handle string number #3900

wants to merge 1 commit into from

Conversation

voxpelli
Copy link
Contributor

Running "npm install --lockfile-version 3" fails with the error messages:

npm WARN invalid config lockfile-version="3" set in command line options
npm WARN invalid config Must be one of: null, 1, 2, 3

This is probably because all CLI input is handled as strings and nopt strictly comparing those strings to numbers without casting them to numbers. Replacing the [null, 1, 2, 3] with [null, Number] makes nopt type cast and accept all numbers.

With this change eg. --lockfile-version 6 will still fail later on, so it won't open the flood gates for all version numbers. Will still fail with this later in the script:

npm ERR! Invalid lockfileVersion config: 6

References

Related to #3880

As all CLI input is considered to be string, eg. a "npm install --lockfile-version 3" otherwise fails with the error messages:

npm WARN invalid config lockfile-version="3" set in command line options
npm WARN invalid config Must be one of: null, 1, 2, 3

Replacing `1, 2, 3` with `Number` avoids that and eg. `--lockfile-version 6` will still fail later on with `npm ERR! Invalid lockfileVersion config: 6`, so this is in a way a more DRY approach as well.
@voxpelli voxpelli requested a review from a team as a code owner October 15, 2021 10:55
@voxpelli voxpelli changed the title Fix --lockfile-version to handle string number fix: --lockfile-version to handle string number Oct 15, 2021
@isaacs
Copy link
Contributor

isaacs commented Oct 15, 2021

Hmm, well this is tricky. We want it to be a number type, but also we want to catch it as a config error rather than a later crash.

But you're right, it's not converting the type properly and seeing that it should be numeric. (We cannot get off of nopt soon enough, omg. npm 9 needs to ship like yesterday 😅)

I guess in the meantime, until we have a way to set type and possible values, we could take this. Another approach might be to make it [null, 1, 2, 3, '1', '2', '3'], since it does get converted to a number later on.

@voxpelli
Copy link
Contributor Author

I tried the [null, 1, 2, 3, '1', '2', '3'] I think, but then I believe it was caught in @npm/arborist saying it was an invalid lockfileVersion here: https://github.com/npm/arborist/blob/5fab942b1562de94879034769aa315b52802a9e4/lib/arborist/index.js#L54-L64

@isaacs
Copy link
Contributor

isaacs commented Oct 15, 2021

@voxpelli Ah, yes. Does this do the trick?

diff --git a/lib/utils/config/definitions.js b/lib/utils/config/definitions.js
index 3bb8a4210..5dafe0c8d 100644
--- a/lib/utils/config/definitions.js
+++ b/lib/utils/config/definitions.js
@@ -1144,7 +1144,7 @@ define('location', {
 
 define('lockfile-version', {
   default: null,
-  type: [null, 1, 2, 3],
+  type: [null, 1, 2, 3, '1', '2', '3'],
   defaultDescription: `
     Version 2 if no lockfile or current lockfile version less than or equal to
     2, otherwise maintain current lockfile version
@@ -1166,7 +1166,9 @@ define('lockfile-version', {
     on disk than lockfile version 2, but not interoperable with older npm
     versions.  Ideal if all users are on npm version 7 and higher.
   `,
-  flatten,
+  flatten: (key, obj, flatOptions) => {
+    flatOptions.lockfileVersion = obj[key] && parseInt(obj[key], 10)
+  },
 })
 
 define('loglevel', {

@isaacs isaacs self-assigned this Oct 15, 2021
@isaacs isaacs added the Bug thing that needs fixing label Oct 15, 2021
@darcyclarke darcyclarke added semver:patch semver patch level for changes Release 8.x work is associated with a specific npm 8 release labels Oct 21, 2021
@lukekarrys lukekarrys changed the base branch from latest to release-next October 26, 2021 20:52
@lukekarrys
Copy link
Member

Closing this in favor of #3949.

@lukekarrys lukekarrys closed this Oct 27, 2021
@voxpelli voxpelli deleted the patch-1 branch October 29, 2021 22:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug thing that needs fixing Release 8.x work is associated with a specific npm 8 release semver:patch semver patch level for changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants