Skip to content

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
Contributor

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.

4 participants