-
Notifications
You must be signed in to change notification settings - Fork 4.3k
fix: prefer existing tree nodes for peerOptional deps (#9249) #9283
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
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -898,8 +898,18 @@ This is a one-time fix-up, please be patient... | |||||||||||||||||||||
| // be forced to agree on a version of z. | ||||||||||||||||||||||
| const required = new Set([edge.from]) | ||||||||||||||||||||||
| const parent = edge.peer ? virtualRoot : null | ||||||||||||||||||||||
| const dep = vrDep && vrDep.satisfies(edge) ? vrDep | ||||||||||||||||||||||
| : await this.#nodeFromEdge(edge, parent, null, required) | ||||||||||||||||||||||
| let dep = vrDep && vrDep.satisfies(edge) ? vrDep : null | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| // A peerOptional conflict can be resolved by finding an existing | ||||||||||||||||||||||
| // node in the tree that satisfies the edge, avoiding a registry | ||||||||||||||||||||||
| // fetch that may introduce an extraneous package. | ||||||||||||||||||||||
| // See npm/cli#9249. | ||||||||||||||||||||||
| if (!dep && edge.type === 'peerOptional') { | ||||||||||||||||||||||
| dep = this.#findHoistableNode(edge.from.resolveParent || edge.from, edge) | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
I think if we add this check, we get to keep the dedupe behavior for normal installs while still letting npm update, npm audit fix, and explicit installs do their thing.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is only in problemEdges tho, wouldn't there be other chances earlier to apply requested updates? Aside: I noticed there is a test that peerOptionals are installed when explicitly requested, and that still passes Tho I noticed that even before this change the test would fail on re-install, or when package-lock or node_modules was already present. That seems bad, npm's only respecting the explciit request when it's not building from a lock.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh, I see those count as problems and land here. |
||||||||||||||||||||||
| if (!dep) { | ||||||||||||||||||||||
| dep = await this.#nodeFromEdge(edge, parent, null, required) | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| /* istanbul ignore next */ | ||||||||||||||||||||||
| debug(() => { | ||||||||||||||||||||||
|
|
@@ -1029,6 +1039,24 @@ This is a one-time fix-up, please be patient... | |||||||||||||||||||||
| return this.#buildDepStep() | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| // BFS descendants of `root` for a node satisfying `edge`. | ||||||||||||||||||||||
| // Prefers nodes closer to root. Skips bundled nodes. | ||||||||||||||||||||||
| #findHoistableNode (root, edge) { | ||||||||||||||||||||||
| const queue = [...root.children.values()] | ||||||||||||||||||||||
| while (queue.length) { | ||||||||||||||||||||||
| const node = queue.shift() | ||||||||||||||||||||||
| if (node.name === edge.name | ||||||||||||||||||||||
| && !node.inDepBundle | ||||||||||||||||||||||
| && node.satisfies(edge)) { | ||||||||||||||||||||||
| return node | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
| for (const child of node.children.values()) { | ||||||||||||||||||||||
| queue.push(child) | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
| return null | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| // loads a node from an edge, and then loads its peer deps (and their peer deps, on down the line) into a virtual root parent. | ||||||||||||||||||||||
| async #nodeFromEdge (edge, parent_, secondEdge, required) { | ||||||||||||||||||||||
| // create a virtual root node with the same deps as the node that is requesting this one, so that we can get all the peer deps in a context where they're likely to be resolvable. | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we try to break only at natural boundaries for screen reader accessibility