feat: add pnpm dedupe command
#5958
Conversation
gluxon
force-pushed
the
dedupe
branch
3 times, most recently
from
f4f4bc9 to
cca24e2
Compare
|
This solution feels a bit hacky. Have you tested this package: pnpm-deduplicate? I guess we could integrate it to our CLI if it works good (or better than the suggested solution). cc @ocavue |
Yup! I've seen
I'm appreciative of the work @ocavue made to create At the moment, the
The There's also an edge case with
This approach relies on the existing broken lockfile code to re-resolve dependencies. Was that the part that looked a bit fishy to you? Admittedly this assumption should be more explicit. I can think of more ways to guarantee the deleted |
ok, add a comment about it to the code |
| @@ -37,6 +37,7 @@ export interface StrictInstallOptions { | |||
| linkWorkspacePackagesDepth: number | |||
| lockfileOnly: boolean | |||
| fixLockfile: boolean | |||
| resetResolutionsWithPreferredVersions: boolean | |||
There was a problem hiding this comment.
Maybe just name it dedupe?
| resetResolutionsWithPreferredVersions: boolean | |
| dedupe: boolean |
Also, you're saying that this works because of the fixLockfile logic but I don't see the fixLockfile set to true anywhere
There was a problem hiding this comment.
Maybe just name it dedupe?
Can do! Wasn't sure if the extra verbose name here was that helpful. Thanks for the check.
Also, you're saying that this works because of the fixLockfile logic but I don't see the
fixLockfileset to true anywhere
I think I could have been more clear in the last comment.
My understanding of fixLockfile is that it can regenerate parts of pnpm-lock.yaml if there are any dangling resolutions. For example, a package refers to foo@1.0.0, but that version doesn't exist in the lockfile anymore.
Since there's no dangling resolutions, fixLockfile isn't needed explicitly since pnpm in general rebuilds the wanted lockfile as it resolves the dependency graph. This should be using the same assumptions the existing forgetResolutionsOfPrevWantedDeps function does for importers. Admittedly, there could be stronger guarantees of this assumption for packages.
I think I could extend the test to make sure a pnpm install after pnpm dedupe doesn't change the lockfile to better capture the assumption packages is regenerated.
There was a problem hiding this comment.
Admittedly, there could be stronger guarantees of this assumption for packages.
Here are the parts of resolve-dependencies that regenerate packages. The toLockfileDependency function is fine with prevSnapshot.dependencies being a partial. This runs regardless of fixLockfile.
pnpm/pkg-manager/resolve-dependencies/src/updateLockfile.ts
Lines 81 to 82 in b78774e
pnpm/pkg-manager/resolve-dependencies/src/updateLockfile.ts
Lines 221 to 223 in b78774e
I'll comment that this is happening in resolve-dependencies.
Can do! I can make that more clear. I also wanted to note that we do something similar here already: pnpm/pkg-manager/core/src/install/index.ts Lines 630 to 632 in cca24e2 Thinking through this again, I think it might be more clear if I move the existing logic into a |
|
a new function sounds good |
gluxon
force-pushed
the
dedupe
branch
2 times, most recently
from
2ce3394 to
5539069
Compare
For pnpm/pkg-manager/core/src/install/index.ts Lines 820 to 823 in 5539069 For pnpm/pkg-manager/core/src/install/index.ts Lines 648 to 650 in 5539069 |
|
Latest push has the following changes:
|
|
Overall it looks OK. I'll review the tests later. |
|
Latest push:
pnpm/pkg-manager/plugin-commands-installation/test/dedupe.ts Lines 54 to 58 in 0586fd4
Thanks for reviewing! |
| --- | ||
| "@pnpm/plugin-commands-installation": minor | ||
| pnpm: minor | ||
| --- |
There was a problem hiding this comment.
@pnpm/core should also be bumped.
There was a problem hiding this comment.
Oops. Thanks for catching! Fixed here: https://github.com/pnpm/pnpm/compare/2686819b8c133ee33bd771b82a75ff86f40ca9fd..7ab4221dcce9aa5fb0f516eba4948f7917e384f1
| "integrity": "sha512-XRsRjdf+j5ml+y/6GKHPZbrF/8p2Yga0JPtdqTIY2Xe5ohJPD9saDJJLPvp9+NSBprVvevdXZybnj2cv8OEd0A==", | ||
| }, | ||
| }, | ||
| "/punycode/2.3.0": { |
There was a problem hiding this comment.
Is it expected that two versions of punycode are present? And both are v2
There was a problem hiding this comment.
Yeah, the other version comes in from the earlier version of uri-js@4.2.2, which also appears twice in the lockfile before deduping.
❯ pnpm view uri-js@4.2.2
# Omitted for brevity
dependencies:
punycode: ^2.1.0
The earlier version of uri-js@4.2.2 is brought in by the earlier version of ajv.
❯ pnpm view ajv@6.10.2
# Omitted for brevity
dependencies:
fast-deep-equal: ^2.0.1
fast-json-stable-stringify: ^2.0.0
json-schema-traverse: ^0.4.1
uri-js: ^4.2.2
Fortunately there's only one version of uri-js and punycode after deduping. 🙂
There was a problem hiding this comment.
It might be helpful to test that if uri-js@4.2.2 and uri-js@4.4.1 are present in the lockfile, that uri-js@4.2.2 dedupes to use punycode@2.3.0. I'll add that test now.
There was a problem hiding this comment.
I could clarify one more thing here: The earlier versions of ajv's dependencies appear because I copied the with-peer fixture.
pnpm/__fixtures__/with-peer/package.json
Lines 2 to 5 in 1072ec1
I then added a new workspace package that depends on the later version of ajv of that same major version. That should simulate what happens in a lot of monorepos, where a developer adds a later version of a dependency that already existed.
There was a problem hiding this comment.
It might be helpful to test that if
uri-js@4.2.2anduri-js@4.4.1are present in the lockfile, thaturi-js@4.2.2dedupes to usepunycode@2.3.0. I'll add that test now.
Done in 8010d61
|
Thanks! Please feel free to ping me if any issues related to this new command come up. I can take responsibility and troubleshoot in any threads. |
|
@gluxon Thank you for implement this! I'm very happy to see that |
|
Thanks again for making |
|
Cool stuff! I wonder if we can also run this command in |
|
@kachkaev I think something in that direction makes sense! Will comment on your issue with some of my thoughts. |
Changes
This PR adds a
pnpm dedupecommand that works similarly toyarn dedupe --strategy highest.Motivation
Today, pnpm is already very good at deduplicating on
pnpm install. If the specifier of a new dependency matches an existing dependency already present inpnpm-lock.yaml, that existing version will be reused. ✅However, the
pnpm-lock.yamlfile can get into a state that requires cleanup. Suppose multiple workspace packages depend onajv@^6.10.2.If a user introduces a new version specifier for
ajvthat doesn't overlap with any existing versions inpnpm-lock.yaml, pnpm will install a new version ofajvalongside the old one. This makes sense and is generally desirable.To avoid unexpected surprises, pnpm only updates the
fooworkspace package's wanted version ofajvsince it's the project that changed. It does not rewrite thebarpackage's wanted version, despite^6.10.2satisfying the newly installed6.12.6version.While the above makes sense for normal
pnpm installbehavior, from time to time it's desirable slim down thepnpm-lock.yamlfile by re-resolving. This was added as a new command since upgrading existing resolutions should be explicitly intentional and generally requires testing afterwards.In a large monorepo on my team, this new command was able to remove hundreds of dependencies correctly.
Other Discussions
Questions
pnpm dedupefail if the wanted lockfile isn't the same as the current lockfile? This is a scenario that would cause new dependencies to be added.Testing
The above example was added as a fixture named
workspace-with-lockfile-dupes. Runningpnpm dedupeon the fixture creates the followingpnpm-lock.yamldiff.