fix(resolvePeers): fix deduplication when version missmatch#6606
Conversation
|
💖 Thanks for opening this pull request! 💖 |
This comment was marked as outdated.
This comment was marked as outdated.
|
You need to run |
This comment was marked as resolved.
This comment was marked as resolved.
|
I don't understand why. But having jest installed globally messed it up. Uninstalling it fixed it. |
3239e9e to
92f805e
Compare
|
The failing tests was due to "nested" peer dependencies. The test I solved it by keeping track of unresolved duplicates and taking several deduplication passes until nothing is deduplicated. There is still one failing test locally; Is this expected? |
|
yes, I think that one breaks for me too. |
| Object.values(depGraph).forEach((node) => { | ||
| node.children = mapValues((childDepPath) => depPathsMap[childDepPath] ?? childDepPath, node.children) | ||
| }) | ||
| for (const { id } of opts.projects) { | ||
| dependenciesByProjectId[id] = mapValues((depPath) => depPathsMap[depPath] ?? depPath, dependenciesByProjectId[id]) | ||
| } |
There was a problem hiding this comment.
do we need to do this on each cycle? Can't we do it one time after the while cycle?
There was a problem hiding this comment.
You're correct. Moved it out.
I was also getting paranoid about the while loop. Added a maxPasses guard. In the worst case scenario, where every duplicated package is dependent on another duplicated package, the max number of passes that can do anything is same as duplicates.length.
When dedupe-peer-dependents is enabled (default), use the path to determine compatibility. When multiple dependency groups can be deduplicated, the latter ones are sorted according to number of peers to allow them to benefit from deduplication.
92f805e to
a834d26
Compare
| Object.values(depGraph).forEach((node) => { | ||
| node.children = mapValues((childDepPath) => depPathsMap[childDepPath] ?? childDepPath, node.children) | ||
| }) |
There was a problem hiding this comment.
what about this one? Can this be moved out from the loop?
There was a problem hiding this comment.
No, that is required for pkg-manager/resolve-dependencies/src/resolvePeers.ts:196-197 to get the dependency path for any peers that have been deduplicated in a previous pass.
There was a problem hiding this comment.
This is a pretty expensive operation. Can at least Object.values(depGraph) be moved to a variable?
|
One could create a map for the affected nodes per duplicated dependency. That way one can replace one the affected keys in the affected nodes. For that I would need a canonical way of translating the pkgId (/abc/1.0.0) to the pkg key in node.children. That way there would only be one pass over the nodes. I can do it on Monday. |
|
Yes, that solution actually sounds better to me. |
|
I'll merge it for now. You can make your optimisations in a separate PR. |
|
Congrats on merging your first pull request! 🎉🎉🎉 |
When dedupe-peer-dependents is enabled (default), use the path to determine compatibility. When multiple dependency groups can be deduplicated, the latter ones are sorted according to number of peers to allow them to benefit from deduplication. close #6605 --------- Co-authored-by: Zoltan Kochan <z@kochan.io>
I got sidetracked. I'll have a look at the optimization, but not sure when it will happen. |
resolves: #6605
When dedupe-peer-dependents is enabled (default), use the path (not id) to
determine compatibility.
When multiple dependency groups can be deduplicated, the
latter ones are sorted according to number of peers to allow them to
benefit from deduplication.