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

Support package-lock.json v3 in npm 7 #434

Merged
merged 1 commit into from Oct 12, 2021
Merged

Support package-lock.json v3 in npm 7 #434

merged 1 commit into from Oct 12, 2021

Conversation

remcohaszing
Copy link
Contributor

npm 7 introduces new features which aren’t compatible with older npm versions, such as workspaces. This makes npm 7 a minimum requirement for a project.

Both package-lock.json version 1 and 3 files can be huge. package-lock.json 2 is an intermediate format whose goal is to add backwards compatibility with package-lock.json version 1 and npm 6. It does so by writing both the dependency trees in the version 1 and 3 formats. This means the file is even twice as big as it needs to be. This also means the same change appears twice in a diff, but in a different format, meaning it needs to be reviewed twice. This is unnecessary if the minimal required npm version is 7.

References

Related to npm/feedback#517


npm 7 introduces new features which aren’t compatible with older npm versions, such as workspaces. This makes npm 7 a minimum requirement for a project.

Both `package-lock.json` version 1 and 3 files can be huge. `package-lock.json` 2 is an intermediate format whose goal is to add backwards compatibility with `package-lock.json` version 1 and npm 6. It does so by writing both the dependency trees in the version 1 and 3 formats. This means the file is even twice as big as it needs to be. This also means the same change appears twice in a diff, but in a different format, meaning it needs to be reviewed twice. This is unnecessary if the minimal required npm version is 7.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Both `package-lock.json` version 1 and 3 files can be huge. `package-lock.json` 2 is an intermediate format whose goal is to add backwards compatibility with `package-lock.json` version 1 and npm 6. It does so by writing both the dependency trees in the version 1 and 3 formats. This means the file is even twice as big as it needs to be. This also means the same change appears twice in a diff, but in a different format, meaning it needs to be reviewed twice. This is unnecessary if the minimal required npm version is 7.
Both `package-lock.json` version 1 and 2 files can be huge. `package-lock.json` 2 is an intermediate format whose goal is to add backwards compatibility with `package-lock.json` version 1 and npm 6. It does so by writing both the dependency trees in the version 1 and 3 formats. This means the file is even twice as big as it needs to be. This also means the same change appears twice in a diff, but in a different format, meaning it needs to be reviewed twice. This is unnecessary if the minimal required npm version is 7.

?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nope, I meant versions 1 and 3. Version 2 includes both, which makes it even bigger.

@ljharb
Copy link
Contributor

ljharb commented Aug 13, 2021

Instead of letting the user control this, could it pivot based on the engines.npm value? In other words, if npm 7 is already declared as a requirement, then it knows it doesn't need to write npm 6 back compat stuff.

@remcohaszing
Copy link
Contributor Author

Instead of letting the user control this, could it pivot based on the engines.npm value? In other words, if npm 7 is already declared as a requirement, then it knows it doesn't need to write npm 6 back compat stuff.

I didn’t think of that, but this is an interesting idea.

I think the original proposal is better though, because:

  1. Since the engines field in package.json is published with the package, I think this implies it’s a runtime requirement, whereas the lockfile format is a development requirement for the package itself.
  2. The original proposal to store it in .npmrc is consistent with the usage of package-lock=false

Another idea might be to reuse the package-lock field in .npmrc that already exists.

# Don’t write package-lock.json at all
package-lock=false
# Write package-lock.json version 1
package-lock=v1
# Write package-lock.json version 2
package-lock=v2
# Write package-lock.json version 3
package-lock=v3
# Write the package-lock.json version npm thinks is best (default behaviour, equal to v2 for npm 7)
package-lock=true

@darcyclarke darcyclarke added the Agenda will be discussed at the Open RFC call label Aug 17, 2021

## Rationale and Alternatives

1. A command line flag could be supported instead of an option in `.npmrc`. However, typically one would want to support this on a project level.
Copy link

Choose a reason for hiding this comment

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

My understanding is that typically command line flags are also usable in .npmrc - so supporting npm install --package-lock-version=<number> would also allow you to use package-lock-version=<number> in .npmrc.

IMO the path of both is ideal.

@bnb
Copy link

bnb commented Aug 22, 2021

I particularly like the recommendation to extend the package-lock property. One thing to note is that you'd probably want to start shipping npm with a range of acceptable numbers, a way to find those numbers (perhaps npm info package-lock or something), and to error on a number that isn't within the range of acceptable numbers.

@darcyclarke darcyclarke removed the Agenda will be discussed at the Open RFC call label Aug 25, 2021
@darcyclarke
Copy link
Contributor

@darcyclarke darcyclarke added the Agenda will be discussed at the Open RFC call label Sep 22, 2021
@isaacs
Copy link
Contributor

isaacs commented Sep 22, 2021

We should definitely not make --package-lock more than a simple boolean. It's much better to have one config that's a boolean saying "should I write a package-lock.json at all" and another config that's a number saying "what kind of package lock should be written, when/if we write one".

Mixed-type configs are an antipattern.

@darcyclarke darcyclarke removed the Agenda will be discussed at the Open RFC call label Sep 29, 2021
isaacs added a commit to npm/arborist that referenced this pull request Oct 4, 2021
This will tell Arborist to save an appropriate lockfile based on
the version specified.

Version 3 = packages section only
Version 2 = packages and dependencies sections (default)
Version 1 = dependencies section only (legacy npm v6 support)

Re: npm/rfcs#434
isaacs added a commit to npm/arborist that referenced this pull request Oct 4, 2021
This will tell Arborist to save an appropriate lockfile based on
the version specified.

Version 3 = packages section only
Version 2 = packages and dependencies sections (default)
Version 1 = dependencies section only (legacy npm v6 support)

Re: npm/rfcs#434
@isaacs
Copy link
Contributor

isaacs commented Oct 6, 2021

Suggestions:

  • The config key should be lockfile-version, not package-lock-version, since it applies to both npm-shrinkwrap.json and package-lock.json, and corresponds to the lockfileVersion field in the lock file itself.
  • It should be just a plain old config value, which can be set any of the places that config values can be set. So, --lockfile-version cli option, npm_config_lockfile_version env, project .npmrc, user .npmrc, or global npmrc files.
  • It should be a numeric config value, with a default value of 2. Non-numbers (eg, v2, version 2) etc. will be ignored. (Actually, should be limited to 1, 2, or 3. Arborist should perhaps throw if it gets a lockfileVersion it can't understand.)
  • Config value should be flattened to lockfileVersion option passed to @npmcli/arborist. (See WIP feat: make lockfileVersion configurable arborist#329 for proposed implementation.) Any time we write a lockfile, we use the lockfileVersion specified, except for the "hidden lockfile" in node_modules/.package-lock.json, which is always version 3.

This means:

  • Anyone using both npm v7 and v6 can set lockfile-version = 1 in their project .npmrc, to avoid churn.
  • Anyone using npm v7 solely can set lockfile-version = 3 in their project .npmrc, to save disk space while benefitting from the increased reliability and performance.
  • Maximally compatible setting (which works for all npm >= v5) is still the default.
  • When we drop support for npm v6 entirely (or when it's just not used enough to be relevant), it's just a matter of changing the default setting, and everyone gets the right thing.
  • You can always npm i --lockfile-version=X to switch versions without any hassle.

Open question: in the absence of a default lockfileVersion setting, should Arborist use a v3 lockfile if it sees one? It seems a bit weird to have it roll back to v2 if you've already pushed forward. (In the PR as it stands now, it'll always use the setting provided, so if you npm i --lockfile-version=3 and then npm i, it'll put the dependencies section back.) I think it should definitely not use lockfileVersion:1 if it sees that there.

@remcohaszing
Copy link
Contributor Author

@isaacs Looks good to me! Just let me know if I need to update the proposal.

Open question: in the absence of a default lockfileVersion setting, should Arborist use a v3 lockfile if it sees one? It seems a bit weird to have it roll back to v2 if you've already pushed forward. (In the PR as it stands now, it'll always use the setting provided, so if you npm i --lockfile-version=3 and then npm i, it'll put the dependencies section back.) I think it should definitely not use lockfileVersion:1 if it sees that there.

I believe Arborist should always default to lockfile v2 for now for backwards compatibility. This should then be reconsidered for npm 8.

@isaacs
Copy link
Contributor

isaacs commented Oct 7, 2021

I believe Arborist should always default to lockfile v2 for now for backwards compatibility. This should then be reconsidered for npm 8.

Right, but the use case I'm imagining is:

npm i --lockfile-version=3 # create a v3 lockfile
npm i foo # now it just reset the lockfile version back to 2

If the lockfile is already v3, maybe we should default to that, rather than downgrading it?

@ljharb
Copy link
Contributor

ljharb commented Oct 7, 2021

It would seem like a bug to me if it downgrades it or upgrades it implicitly.

@bnb
Copy link

bnb commented Oct 7, 2021

If the lockfile is already v3, maybe we should default to that, rather than downgrading it?

Agree. Not only would it be a buggy experience, it would be excessively noisy in git diffs which is both a good way to make people more desensitized to package-lock file changes even more than they already are.

@isaacs
Copy link
Contributor

isaacs commented Oct 7, 2021

It would seem like a bug to me if it downgrades it or upgrades it implicitly.

"Upgrade it implicitly" is what we already do. The v1 lockfile doesn't have all the information we need to fully construct the tree, meaning that installs are slower and less deterministic, don't track changes in link targets, etc.

But downgrading implicitly definitely seems like the wrong thing to do, since you must have opted into the new version explicitly to be in that state.

@ljharb
Copy link
Contributor

ljharb commented Oct 7, 2021

upgrading from v1 to v2, i understand for the reasons you describe; but from v2 to v3?

PR-URL: #434
Credit: @remcohaszing
Close: #434
Reviewed-by: @isaacs

EDIT(@isaacs): updated to reflect where the discussion on this feature
landed in open RFC meetings, and add implementation details.
@isaacs isaacs closed this in 8804b67 Oct 12, 2021
@isaacs isaacs merged commit 8804b67 into npm:main Oct 12, 2021
isaacs added a commit to npm/arborist that referenced this pull request Oct 12, 2021
This will tell Arborist to save an appropriate lockfile based on
the version specified.

Version 3 = packages section only
Version 2 = packages and dependencies sections (default)
Version 1 = dependencies section only (legacy npm v6 support)

Re: npm/rfcs#434
@isaacs
Copy link
Contributor

isaacs commented Oct 12, 2021

upgrading from v1 to v2, i understand for the reasons you describe; but from v2 to v3?

Upgrading from the default version to a higher version should never happen implicitly. But if you already explicitly set it to a higher version, we shouldn't downgrade to the default.

isaacs added a commit to npm/arborist that referenced this pull request Oct 12, 2021
This will tell Arborist to save an appropriate lockfile based on
the version specified.

Version 3 = packages section only
Version 2 = packages and dependencies sections (default)
Version 1 = dependencies section only (legacy npm v6 support)

Re: npm/rfcs#434
isaacs added a commit to npm/cli that referenced this pull request Oct 12, 2021
isaacs added a commit to npm/cli that referenced this pull request Oct 12, 2021
isaacs added a commit to npm/cli that referenced this pull request Oct 12, 2021
Depends on @npmcli/arborist@4.0.0

Re: npm/rfcs#434

PR-URL: #3880
Credit: @isaacs
Close: #3880
Reviewed-by: @wraithgar
isaacs added a commit to npm/cli that referenced this pull request Oct 12, 2021
Depends on @npmcli/arborist@4.0.0

Re: npm/rfcs#434

PR-URL: #3880
Credit: @isaacs
Close: #3880
Reviewed-by: @wraithgar
@xiaoxiyao
Copy link

Which version of npm 7 started to support lockfileVersion v3?

@remcohaszing
Copy link
Contributor Author

The initial support for lockfile version 3 had some bugs. It’s been usable in practice since npm version 8.1.2.

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.

None yet

6 participants