From 865070cff95a7de2d566428e3197601ee3a40ad6 Mon Sep 17 00:00:00 2001 From: Alon Navon Date: Thu, 23 Nov 2023 22:13:41 +0200 Subject: [PATCH 01/52] Settle overrides conflicts between edges, and propagate changes to the edges out --- workspaces/arborist/lib/edge.js | 4 +- workspaces/arborist/lib/node.js | 90 ++++++++++++++++++++++++++++++++- 2 files changed, 90 insertions(+), 4 deletions(-) diff --git a/workspaces/arborist/lib/edge.js b/workspaces/arborist/lib/edge.js index cc9698ad6cae7..2a8b520509b01 100644 --- a/workspaces/arborist/lib/edge.js +++ b/workspaces/arborist/lib/edge.js @@ -258,7 +258,7 @@ class Edge { const newTo = this.#from.resolve(this.#name) if (newTo !== this.#to) { if (this.#to) { - this.#to.edgesIn.delete(this) + this.#to.deleteEdgeIn(this) } this.#to = newTo this.#error = null @@ -273,7 +273,7 @@ class Edge { detach () { this.#explanation = null if (this.#to) { - this.#to.edgesIn.delete(this) + this.#to.deleteEdgeIn(this) } this.#from.edgesOut.delete(this.#name) this.#to = null diff --git a/workspaces/arborist/lib/node.js b/workspaces/arborist/lib/node.js index bdc021b7c12a9..975bcbad60861 100644 --- a/workspaces/arborist/lib/node.js +++ b/workspaces/arborist/lib/node.js @@ -1344,9 +1344,95 @@ class Node { this.edgesOut.set(edge.name, edge) } - addEdgeIn (edge) { + recalculateOutEdgesOverrides () { + // For each edge out propogate the new overrides through. + for (const edgePair of this.edgesOut) { + const edge = edgePair[1] + edge.reload(true) + if (edge.to) { + edge.to.updateNodeOverrideSet(edge.overrides) + } + } + } + + findSpecificOverrideSet (first, second) { + for (let overrideSet = second; overrideSet; overrideSet = overrideSet.parent) { + if (overrideSet == first) { + return second + } + } + for (let overrideSet = first; overrideSet; overrideSet = overrideSet.parent) { + if (overrideSet == second) { + return first + } + } + return undefined + } + + updateNodeOverrideSetDueToEdgeRemoval (otherOverrideSet) { + // If this edge's overrides isn't equal to this node's overrides, then removing it won't change newOverrideSet later. + if (this.overrides != otherOverrideSet) { + return false + } + let newOverrideSet + for (const edge of this.edgesIn) { + if (newOverrideSet) { + newOverrideSet = this.findSpecificOverrideSet() + } else { + newOverrideSet = edge.overrides + } + } + if (this.overrides == newOverrideSet) { + return false + } + this.overrides = newOverrideSet + this.recalculateOutEdgesOverrides() + return true + } + + // This logic isn't perfect either. When we have two edges in that have different override sets, then we have to decide which set is correct. + // This function assumes the more specific override set is applicable, so if we have dependencies A->B->C and A->C + // and an override set that specifies what happens for C under A->B, this will work even if the new A->C edge comes along and tries to change + // the override set. + // The strictly correct logic is not to allow two edges with different overrides to point to the same node, because even if this node can satisfy + // both, one of its dependencies might need to be different depending on the edge leading to it. + // However, this might cause a lot of duplication, because the conflict in the dependencies might never actually happen. + updateNodeOverrideSet (otherOverrideSet) { + if (this.overrides == otherOverrideSet) { + return false + } + if (!this.overrides) { + this.overrides = otherOverrideSet + this.recalculateOutEdgesOverrides() + return true + } + let newOverrideSet = this.findSpecificOverrideSet(this.overrides, otherOverrideSet) + if (!newOverrideSet) { + // There are conflicting relevant override sets here. We should keep the remaining override set and mark the incoming edge as invalid somehow. + console.log("Overrides are in conflict.") + } else { + if (this.overrides != newOverrideSet) { + this.overrides = newOverrideSet + this.recalculateOutEdgesOverrides() + return true + } + return false + } + } + + deleteEdgeIn (edge) { + this.edgesIn.delete(edge) if (edge.overrides) { - this.overrides = edge.overrides + this.updateNodeOverrideSetDueToEdgeRemoval(edge.overrides) + } + } + + addEdgeIn (edge) { + // We need to handle the case where the new edge in has an overrides field which is different from the current value. + // Assuming there are any overrides at all, the overrides field is never undefined for any node at the end state of the tree. + // So if the new edge's overrides is undefined it will be updated later. So we can wait with updating the node's overrides field. + if (edge.overrides && this.overrides != edge.overrides) { + this.updateNodeOverrideSet(edge.overrides) } this.edgesIn.add(edge) From cd2019675427dd8d8cb5ffac1033447d014d141b Mon Sep 17 00:00:00 2001 From: Alon Navon Date: Sun, 26 Nov 2023 14:25:46 +0200 Subject: [PATCH 02/52] cleanup --- workspaces/arborist/lib/node.js | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/workspaces/arborist/lib/node.js b/workspaces/arborist/lib/node.js index 975bcbad60861..cffd9ed91b379 100644 --- a/workspaces/arborist/lib/node.js +++ b/workspaces/arborist/lib/node.js @@ -1346,8 +1346,7 @@ class Node { recalculateOutEdgesOverrides () { // For each edge out propogate the new overrides through. - for (const edgePair of this.edgesOut) { - const edge = edgePair[1] + for (const [, edge] of this.edgesOut) { edge.reload(true) if (edge.to) { edge.to.updateNodeOverrideSet(edge.overrides) @@ -1366,7 +1365,6 @@ class Node { return first } } - return undefined } updateNodeOverrideSetDueToEdgeRemoval (otherOverrideSet) { @@ -1409,7 +1407,7 @@ class Node { let newOverrideSet = this.findSpecificOverrideSet(this.overrides, otherOverrideSet) if (!newOverrideSet) { // There are conflicting relevant override sets here. We should keep the remaining override set and mark the incoming edge as invalid somehow. - console.log("Overrides are in conflict.") + throw Object.assign(new Error(`Two conflicting override sets for ${this.name}`), { code: 'EOVERRIDE' }) } else { if (this.overrides != newOverrideSet) { this.overrides = newOverrideSet From 0cad4e093ed917dada991eee21c02ae7809bc22b Mon Sep 17 00:00:00 2001 From: Alon Navon Date: Sun, 26 Nov 2023 15:35:55 +0200 Subject: [PATCH 03/52] Optimization --- workspaces/arborist/lib/node.js | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/workspaces/arborist/lib/node.js b/workspaces/arborist/lib/node.js index cffd9ed91b379..148a0f8aa8c91 100644 --- a/workspaces/arborist/lib/node.js +++ b/workspaces/arborist/lib/node.js @@ -1365,6 +1365,7 @@ class Node { return first } } + console.log("Conflicting override sets") } updateNodeOverrideSetDueToEdgeRemoval (otherOverrideSet) { @@ -1375,7 +1376,7 @@ class Node { let newOverrideSet for (const edge of this.edgesIn) { if (newOverrideSet) { - newOverrideSet = this.findSpecificOverrideSet() + newOverrideSet = this.findSpecificOverrideSet(edge.overrides, newOverrideSet) } else { newOverrideSet = edge.overrides } @@ -1384,7 +1385,12 @@ class Node { return false } this.overrides = newOverrideSet - this.recalculateOutEdgesOverrides() + if (this.overrides) { + // Optimization: if there's any override set at all, then no non-extraneous node has an empty override set. So if we temporarily have no + // override set (for example, we removed all the edges in), there's no use updating all the edges out right now. Let's just wait until + // we have an actual override set later. + this.recalculateOutEdgesOverrides() + } return true } @@ -1406,8 +1412,8 @@ class Node { } let newOverrideSet = this.findSpecificOverrideSet(this.overrides, otherOverrideSet) if (!newOverrideSet) { - // There are conflicting relevant override sets here. We should keep the remaining override set and mark the incoming edge as invalid somehow. - throw Object.assign(new Error(`Two conflicting override sets for ${this.name}`), { code: 'EOVERRIDE' }) + // This is an error condition. We can only get here if the new override set is in conflict with the existing. + return false } else { if (this.overrides != newOverrideSet) { this.overrides = newOverrideSet From 6b276f3292ce0731e46e5af1b5d9eda3ed424edb Mon Sep 17 00:00:00 2001 From: Alon Navon Date: Tue, 28 Nov 2023 19:34:12 +0200 Subject: [PATCH 04/52] Fix override sets comparisons --- workspaces/arborist/lib/node.js | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/workspaces/arborist/lib/node.js b/workspaces/arborist/lib/node.js index 148a0f8aa8c91..54895fead4637 100644 --- a/workspaces/arborist/lib/node.js +++ b/workspaces/arborist/lib/node.js @@ -1356,12 +1356,12 @@ class Node { findSpecificOverrideSet (first, second) { for (let overrideSet = second; overrideSet; overrideSet = overrideSet.parent) { - if (overrideSet == first) { + if (overrideSet.isEqual(first)) { return second } } for (let overrideSet = first; overrideSet; overrideSet = overrideSet.parent) { - if (overrideSet == second) { + if (overrideSet.isEqual(second)) { return first } } @@ -1370,7 +1370,7 @@ class Node { updateNodeOverrideSetDueToEdgeRemoval (otherOverrideSet) { // If this edge's overrides isn't equal to this node's overrides, then removing it won't change newOverrideSet later. - if (this.overrides != otherOverrideSet) { + if (!this.overrides.isEqual(otherOverrideSet)) { return false } let newOverrideSet @@ -1381,7 +1381,7 @@ class Node { newOverrideSet = edge.overrides } } - if (this.overrides == newOverrideSet) { + if (this.overrides.isEqual(newOverrideSet)) { return false } this.overrides = newOverrideSet @@ -1402,7 +1402,7 @@ class Node { // both, one of its dependencies might need to be different depending on the edge leading to it. // However, this might cause a lot of duplication, because the conflict in the dependencies might never actually happen. updateNodeOverrideSet (otherOverrideSet) { - if (this.overrides == otherOverrideSet) { + if (this.overrides.isEqual(otherOverrideSet)) { return false } if (!this.overrides) { @@ -1415,7 +1415,7 @@ class Node { // This is an error condition. We can only get here if the new override set is in conflict with the existing. return false } else { - if (this.overrides != newOverrideSet) { + if (!this.overrides.isEqual(newOverrideSet)) { this.overrides = newOverrideSet this.recalculateOutEdgesOverrides() return true @@ -1435,7 +1435,7 @@ class Node { // We need to handle the case where the new edge in has an overrides field which is different from the current value. // Assuming there are any overrides at all, the overrides field is never undefined for any node at the end state of the tree. // So if the new edge's overrides is undefined it will be updated later. So we can wait with updating the node's overrides field. - if (edge.overrides && this.overrides != edge.overrides) { + if (edge.overrides && !this.overrides.isEqual(edge.overrides)) { this.updateNodeOverrideSet(edge.overrides) } From fff30728d47a845647cb8541187e9ed41d1cbb4a Mon Sep 17 00:00:00 2001 From: Alon Navon Date: Tue, 28 Nov 2023 19:35:29 +0200 Subject: [PATCH 05/52] Add comparator --- workspaces/arborist/lib/override-set.js | 28 +++++++++++++++++++++++++ 1 file changed, 28 insertions(+) diff --git a/workspaces/arborist/lib/override-set.js b/workspaces/arborist/lib/override-set.js index bfc5a5d7906ee..b9f9b95293c8e 100644 --- a/workspaces/arborist/lib/override-set.js +++ b/workspaces/arborist/lib/override-set.js @@ -44,6 +44,34 @@ class OverrideSet { } } + childrenAreEqual (other) { + if (this.children.size !== other.children.size) { + return false + } + for (const [key, ] of this.children) { + if (!other.children.has(key)) { + return false + } + if (!this.children[key].value === other.children[key].value) { + return false + } + if (!this.children[key].childrenAreEqual(other.children[key])) { + return false + } + } + return true + } + + isEqual (other) { + if (this === other) { + return true + } + if (this.key !== other.key || this.value !== other.value || !this.childrenAreEqual(other) || !this.parent.isEqual(other.parent)) { + return false + } + return true + } + getEdgeRule (edge) { for (const rule of this.ruleset.values()) { if (rule.name !== edge.name) { From e97176ea3b0ee9bbaacf05c549706005b0289f96 Mon Sep 17 00:00:00 2001 From: Alon Navon Date: Tue, 28 Nov 2023 19:45:18 +0200 Subject: [PATCH 06/52] Check for undefined --- workspaces/arborist/lib/node.js | 13 ++++++++----- workspaces/arborist/lib/override-set.js | 3 +++ 2 files changed, 11 insertions(+), 5 deletions(-) diff --git a/workspaces/arborist/lib/node.js b/workspaces/arborist/lib/node.js index 54895fead4637..e5cdf2044ea4a 100644 --- a/workspaces/arborist/lib/node.js +++ b/workspaces/arborist/lib/node.js @@ -1370,7 +1370,7 @@ class Node { updateNodeOverrideSetDueToEdgeRemoval (otherOverrideSet) { // If this edge's overrides isn't equal to this node's overrides, then removing it won't change newOverrideSet later. - if (!this.overrides.isEqual(otherOverrideSet)) { + if (!this.overrides || !this.overrides.isEqual(otherOverrideSet)) { return false } let newOverrideSet @@ -1402,13 +1402,16 @@ class Node { // both, one of its dependencies might need to be different depending on the edge leading to it. // However, this might cause a lot of duplication, because the conflict in the dependencies might never actually happen. updateNodeOverrideSet (otherOverrideSet) { - if (this.overrides.isEqual(otherOverrideSet)) { - return false - } if (!this.overrides) { + if (!otherOverrideSet) { + return false + } this.overrides = otherOverrideSet this.recalculateOutEdgesOverrides() - return true + return true + } + if (this.overrides.isEqual(otherOverrideSet)) { + return false } let newOverrideSet = this.findSpecificOverrideSet(this.overrides, otherOverrideSet) if (!newOverrideSet) { diff --git a/workspaces/arborist/lib/override-set.js b/workspaces/arborist/lib/override-set.js index b9f9b95293c8e..c6069db9b46e0 100644 --- a/workspaces/arborist/lib/override-set.js +++ b/workspaces/arborist/lib/override-set.js @@ -66,6 +66,9 @@ class OverrideSet { if (this === other) { return true } + if (!other) { + return false + } if (this.key !== other.key || this.value !== other.value || !this.childrenAreEqual(other) || !this.parent.isEqual(other.parent)) { return false } From 19155ed020f5c93a13a29ee4704709b65589cfc9 Mon Sep 17 00:00:00 2001 From: Alon Navon Date: Tue, 28 Nov 2023 19:49:40 +0200 Subject: [PATCH 07/52] Another place --- workspaces/arborist/lib/node.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/workspaces/arborist/lib/node.js b/workspaces/arborist/lib/node.js index e5cdf2044ea4a..967a244b14c56 100644 --- a/workspaces/arborist/lib/node.js +++ b/workspaces/arborist/lib/node.js @@ -1438,7 +1438,7 @@ class Node { // We need to handle the case where the new edge in has an overrides field which is different from the current value. // Assuming there are any overrides at all, the overrides field is never undefined for any node at the end state of the tree. // So if the new edge's overrides is undefined it will be updated later. So we can wait with updating the node's overrides field. - if (edge.overrides && !this.overrides.isEqual(edge.overrides)) { + if (edge.overrides && (!this.overrides || !this.overrides.isEqual(edge.overrides))) { this.updateNodeOverrideSet(edge.overrides) } From dedbccf213d44cca4bab0378651aeede16d7e49d Mon Sep 17 00:00:00 2001 From: Alon Navon Date: Tue, 28 Nov 2023 20:13:09 +0200 Subject: [PATCH 08/52] Fix --- workspaces/arborist/lib/override-set.js | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/workspaces/arborist/lib/override-set.js b/workspaces/arborist/lib/override-set.js index c6069db9b46e0..dcb928b60b216 100644 --- a/workspaces/arborist/lib/override-set.js +++ b/workspaces/arborist/lib/override-set.js @@ -52,10 +52,10 @@ class OverrideSet { if (!other.children.has(key)) { return false } - if (!this.children[key].value === other.children[key].value) { + if (!this.children.get(key).value === other.children.get(key).value) { return false } - if (!this.children[key].childrenAreEqual(other.children[key])) { + if (!this.children.get(key).childrenAreEqual(other.children.get(key))) { return false } } @@ -69,10 +69,16 @@ class OverrideSet { if (!other) { return false } - if (this.key !== other.key || this.value !== other.value || !this.childrenAreEqual(other) || !this.parent.isEqual(other.parent)) { + if (this.key !== other.key || this.value !== other.value) { return false } - return true + if (!this.childrenAreEqual(other)) { + return false + } + if (!this.parent) { + return !other.parent + } + return this.parent.isEqual(other.parent) } getEdgeRule (edge) { From 4a837861b0fd29b27502cfee2cf9dd72148e71c4 Mon Sep 17 00:00:00 2001 From: Alon Navon Date: Wed, 6 Dec 2023 15:48:51 +0200 Subject: [PATCH 09/52] Lint fixes --- workspaces/arborist/lib/node.js | 8 ++++---- workspaces/arborist/lib/override-set.js | 2 +- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/workspaces/arborist/lib/node.js b/workspaces/arborist/lib/node.js index 967a244b14c56..0533c852a67da 100644 --- a/workspaces/arborist/lib/node.js +++ b/workspaces/arborist/lib/node.js @@ -1365,7 +1365,7 @@ class Node { return first } } - console.log("Conflicting override sets") + console.log('Conflicting override sets') } updateNodeOverrideSetDueToEdgeRemoval (otherOverrideSet) { @@ -1408,12 +1408,12 @@ class Node { } this.overrides = otherOverrideSet this.recalculateOutEdgesOverrides() - return true + return true } if (this.overrides.isEqual(otherOverrideSet)) { return false } - let newOverrideSet = this.findSpecificOverrideSet(this.overrides, otherOverrideSet) + const newOverrideSet = this.findSpecificOverrideSet(this.overrides, otherOverrideSet) if (!newOverrideSet) { // This is an error condition. We can only get here if the new override set is in conflict with the existing. return false @@ -1426,7 +1426,7 @@ class Node { return false } } - + deleteEdgeIn (edge) { this.edgesIn.delete(edge) if (edge.overrides) { diff --git a/workspaces/arborist/lib/override-set.js b/workspaces/arborist/lib/override-set.js index dcb928b60b216..988a65d9e6bf8 100644 --- a/workspaces/arborist/lib/override-set.js +++ b/workspaces/arborist/lib/override-set.js @@ -48,7 +48,7 @@ class OverrideSet { if (this.children.size !== other.children.size) { return false } - for (const [key, ] of this.children) { + for (const [key,] of this.children) { if (!other.children.has(key)) { return false } From 7ee2f2e668bd327ba39fcc61bf90ba07f7dae9cf Mon Sep 17 00:00:00 2001 From: Alon Navon Date: Wed, 6 Dec 2023 19:21:28 +0200 Subject: [PATCH 10/52] Added tests --- workspaces/arborist/lib/node.js | 12 +- workspaces/arborist/lib/override-set.js | 4 +- .../tap-snapshots/test/edge.js.test.cjs | 44 +++++++ workspaces/arborist/test/edge.js | 123 ++++++++++++++++++ 4 files changed, 174 insertions(+), 9 deletions(-) diff --git a/workspaces/arborist/lib/node.js b/workspaces/arborist/lib/node.js index 0533c852a67da..d6702bffab388 100644 --- a/workspaces/arborist/lib/node.js +++ b/workspaces/arborist/lib/node.js @@ -1403,6 +1403,8 @@ class Node { // However, this might cause a lot of duplication, because the conflict in the dependencies might never actually happen. updateNodeOverrideSet (otherOverrideSet) { if (!this.overrides) { + // Assuming there are any overrides at all, the overrides field is never undefined for any node at the end state of the tree. + // So if the new edge's overrides is undefined it will be updated later. So we can wait with updating the node's overrides field. if (!otherOverrideSet) { return false } @@ -1414,10 +1416,7 @@ class Node { return false } const newOverrideSet = this.findSpecificOverrideSet(this.overrides, otherOverrideSet) - if (!newOverrideSet) { - // This is an error condition. We can only get here if the new override set is in conflict with the existing. - return false - } else { + if (newOverrideSet) { if (!this.overrides.isEqual(newOverrideSet)) { this.overrides = newOverrideSet this.recalculateOutEdgesOverrides() @@ -1425,6 +1424,7 @@ class Node { } return false } + // This is an error condition. We can only get here if the new override set is in conflict with the existing. } deleteEdgeIn (edge) { @@ -1436,9 +1436,7 @@ class Node { addEdgeIn (edge) { // We need to handle the case where the new edge in has an overrides field which is different from the current value. - // Assuming there are any overrides at all, the overrides field is never undefined for any node at the end state of the tree. - // So if the new edge's overrides is undefined it will be updated later. So we can wait with updating the node's overrides field. - if (edge.overrides && (!this.overrides || !this.overrides.isEqual(edge.overrides))) { + if (!this.overrides || !this.overrides.isEqual(edge.overrides)) { this.updateNodeOverrideSet(edge.overrides) } diff --git a/workspaces/arborist/lib/override-set.js b/workspaces/arborist/lib/override-set.js index 988a65d9e6bf8..a03ed5133aa59 100644 --- a/workspaces/arborist/lib/override-set.js +++ b/workspaces/arborist/lib/override-set.js @@ -48,11 +48,11 @@ class OverrideSet { if (this.children.size !== other.children.size) { return false } - for (const [key,] of this.children) { + for (const [key] of this.children) { if (!other.children.has(key)) { return false } - if (!this.children.get(key).value === other.children.get(key).value) { + if (this.children.get(key).value !== other.children.get(key).value) { return false } if (!this.children.get(key).childrenAreEqual(other.children.get(key))) { diff --git a/workspaces/arborist/tap-snapshots/test/edge.js.test.cjs b/workspaces/arborist/tap-snapshots/test/edge.js.test.cjs index 17dc0b0c9fb0b..7b28779165d56 100644 --- a/workspaces/arborist/tap-snapshots/test/edge.js.test.cjs +++ b/workspaces/arborist/tap-snapshots/test/edge.js.test.cjs @@ -52,6 +52,7 @@ Edge { "from": Object { "addEdgeIn": Function addEdgeIn(edge), "addEdgeOut": Function addEdgeOut(edge), + "deleteEdgeIn": Function deleteEdgeIn(edge), "edgesIn": Set {}, "edgesOut": Map { "b" => Edge { @@ -69,6 +70,7 @@ Edge { "parent": Object { "addEdgeIn": Function addEdgeIn(edge), "addEdgeOut": Function addEdgeOut(edge), + "deleteEdgeIn": Function deleteEdgeIn(edge), "edgesIn": Set {}, "edgesOut": Map { "missing" => Edge { @@ -91,6 +93,7 @@ Edge { "root": Object { "addEdgeIn": Function addEdgeIn(edge), "addEdgeOut": Function addEdgeOut(edge), + "deleteEdgeIn": Function deleteEdgeIn(edge), "edgesIn": Set {}, "edgesOut": Map { "missing" => Edge { @@ -122,6 +125,7 @@ Edge { "to": Object { "addEdgeIn": Function addEdgeIn(edge), "addEdgeOut": Function addEdgeOut(edge), + "deleteEdgeIn": Function deleteEdgeIn(edge), "edgesIn": Set { Edge { "peerConflicted": false, @@ -139,6 +143,7 @@ Edge { "parent": Object { "addEdgeIn": Function addEdgeIn(edge), "addEdgeOut": Function addEdgeOut(edge), + "deleteEdgeIn": Function deleteEdgeIn(edge), "edgesIn": Set {}, "edgesOut": Map { "missing" => Edge { @@ -173,6 +178,7 @@ Edge { "from": Object { "addEdgeIn": Function addEdgeIn(edge), "addEdgeOut": Function addEdgeOut(edge), + "deleteEdgeIn": Function deleteEdgeIn(edge), "edgesIn": Set {}, "edgesOut": Map { "b" => Edge { @@ -190,6 +196,7 @@ Edge { "parent": Object { "addEdgeIn": Function addEdgeIn(edge), "addEdgeOut": Function addEdgeOut(edge), + "deleteEdgeIn": Function deleteEdgeIn(edge), "edgesIn": Set {}, "edgesOut": Map { "missing" => Edge { @@ -212,6 +219,7 @@ Edge { "root": Object { "addEdgeIn": Function addEdgeIn(edge), "addEdgeOut": Function addEdgeOut(edge), + "deleteEdgeIn": Function deleteEdgeIn(edge), "edgesIn": Set {}, "edgesOut": Map { "missing" => Edge { @@ -243,6 +251,7 @@ Edge { "to": Object { "addEdgeIn": Function addEdgeIn(edge), "addEdgeOut": Function addEdgeOut(edge), + "deleteEdgeIn": Function deleteEdgeIn(edge), "edgesIn": Set { Edge { "peerConflicted": false, @@ -260,6 +269,7 @@ Edge { "parent": Object { "addEdgeIn": Function addEdgeIn(edge), "addEdgeOut": Function addEdgeOut(edge), + "deleteEdgeIn": Function deleteEdgeIn(edge), "edgesIn": Set {}, "edgesOut": Map { "missing" => Edge { @@ -294,6 +304,7 @@ Edge { "from": Object { "addEdgeIn": Function addEdgeIn(edge), "addEdgeOut": Function addEdgeOut(edge), + "deleteEdgeIn": Function deleteEdgeIn(edge), "edgesIn": Set {}, "edgesOut": Map { "missing" => Edge { @@ -334,6 +345,7 @@ Edge { "from": Object { "addEdgeIn": Function addEdgeIn(edge), "addEdgeOut": Function addEdgeOut(edge), + "deleteEdgeIn": Function deleteEdgeIn(edge), "edgesIn": Set {}, "edgesOut": Map { "aa" => Edge { @@ -351,6 +363,7 @@ Edge { "parent": Object { "addEdgeIn": Function addEdgeIn(edge), "addEdgeOut": Function addEdgeOut(edge), + "deleteEdgeIn": Function deleteEdgeIn(edge), "edgesIn": Set {}, "edgesOut": Map { "b" => Edge { @@ -368,6 +381,7 @@ Edge { "parent": Object { "addEdgeIn": Function addEdgeIn(edge), "addEdgeOut": Function addEdgeOut(edge), + "deleteEdgeIn": Function deleteEdgeIn(edge), "edgesIn": Set {}, "edgesOut": Map { "missing" => Edge { @@ -390,6 +404,7 @@ Edge { "root": Object { "addEdgeIn": Function addEdgeIn(edge), "addEdgeOut": Function addEdgeOut(edge), + "deleteEdgeIn": Function deleteEdgeIn(edge), "edgesIn": Set {}, "edgesOut": Map { "missing" => Edge { @@ -424,6 +439,7 @@ Edge { "to": Object { "addEdgeIn": Function addEdgeIn(edge), "addEdgeOut": Function addEdgeOut(edge), + "deleteEdgeIn": Function deleteEdgeIn(edge), "edgesIn": Set { Edge { "peerConflicted": false, @@ -441,6 +457,7 @@ Edge { "parent": Object { "addEdgeIn": Function addEdgeIn(edge), "addEdgeOut": Function addEdgeOut(edge), + "deleteEdgeIn": Function deleteEdgeIn(edge), "edgesIn": Set {}, "edgesOut": Map { "b" => Edge { @@ -458,6 +475,7 @@ Edge { "parent": Object { "addEdgeIn": Function addEdgeIn(edge), "addEdgeOut": Function addEdgeOut(edge), + "deleteEdgeIn": Function deleteEdgeIn(edge), "edgesIn": Set {}, "edgesOut": Map { "missing" => Edge { @@ -480,6 +498,7 @@ Edge { "root": Object { "addEdgeIn": Function addEdgeIn(edge), "addEdgeOut": Function addEdgeOut(edge), + "deleteEdgeIn": Function deleteEdgeIn(edge), "edgesIn": Set {}, "edgesOut": Map { "missing" => Edge { @@ -516,6 +535,7 @@ Edge { "from": Object { "addEdgeIn": Function addEdgeIn(edge), "addEdgeOut": Function addEdgeOut(edge), + "deleteEdgeIn": Function deleteEdgeIn(edge), "edgesIn": Set {}, "edgesOut": Map { "aa" => Edge { @@ -533,6 +553,7 @@ Edge { "parent": Object { "addEdgeIn": Function addEdgeIn(edge), "addEdgeOut": Function addEdgeOut(edge), + "deleteEdgeIn": Function deleteEdgeIn(edge), "edgesIn": Set {}, "edgesOut": Map { "missing" => Edge { @@ -576,6 +597,7 @@ Edge { "from": Object { "addEdgeIn": Function addEdgeIn(edge), "addEdgeOut": Function addEdgeOut(edge), + "deleteEdgeIn": Function deleteEdgeIn(edge), "edgesIn": Set {}, "edgesOut": Map { "aa" => Edge { @@ -593,6 +615,7 @@ Edge { "parent": Object { "addEdgeIn": Function addEdgeIn(edge), "addEdgeOut": Function addEdgeOut(edge), + "deleteEdgeIn": Function deleteEdgeIn(edge), "edgesIn": Set { Edge { "peerConflicted": false, @@ -610,6 +633,7 @@ Edge { "parent": Object { "addEdgeIn": Function addEdgeIn(edge), "addEdgeOut": Function addEdgeOut(edge), + "deleteEdgeIn": Function deleteEdgeIn(edge), "edgesIn": Set {}, "edgesOut": Map { "missing" => Edge { @@ -645,6 +669,7 @@ Edge { "to": Object { "addEdgeIn": Function addEdgeIn(edge), "addEdgeOut": Function addEdgeOut(edge), + "deleteEdgeIn": Function deleteEdgeIn(edge), "edgesIn": Set { Edge { "peerConflicted": false, @@ -662,6 +687,7 @@ Edge { "parent": Object { "addEdgeIn": Function addEdgeIn(edge), "addEdgeOut": Function addEdgeOut(edge), + "deleteEdgeIn": Function deleteEdgeIn(edge), "edgesIn": Set {}, "edgesOut": Map { "b" => Edge { @@ -679,6 +705,7 @@ Edge { "parent": Object { "addEdgeIn": Function addEdgeIn(edge), "addEdgeOut": Function addEdgeOut(edge), + "deleteEdgeIn": Function deleteEdgeIn(edge), "edgesIn": Set {}, "edgesOut": Map { "missing" => Edge { @@ -701,6 +728,7 @@ Edge { "root": Object { "addEdgeIn": Function addEdgeIn(edge), "addEdgeOut": Function addEdgeOut(edge), + "deleteEdgeIn": Function deleteEdgeIn(edge), "edgesIn": Set {}, "edgesOut": Map { "missing" => Edge { @@ -737,6 +765,7 @@ Edge { "from": Object { "addEdgeIn": Function addEdgeIn(edge), "addEdgeOut": Function addEdgeOut(edge), + "deleteEdgeIn": Function deleteEdgeIn(edge), "edgesIn": Set {}, "edgesOut": Map { "a" => Edge { @@ -766,6 +795,7 @@ Edge { "to": Object { "addEdgeIn": Function addEdgeIn(edge), "addEdgeOut": Function addEdgeOut(edge), + "deleteEdgeIn": Function deleteEdgeIn(edge), "edgesIn": Set { Edge { "peerConflicted": false, @@ -783,6 +813,7 @@ Edge { "parent": Object { "addEdgeIn": Function addEdgeIn(edge), "addEdgeOut": Function addEdgeOut(edge), + "deleteEdgeIn": Function deleteEdgeIn(edge), "edgesIn": Set {}, "edgesOut": Map { "a" => Edge { @@ -805,6 +836,7 @@ Edge { "root": Object { "addEdgeIn": Function addEdgeIn(edge), "addEdgeOut": Function addEdgeOut(edge), + "deleteEdgeIn": Function deleteEdgeIn(edge), "edgesIn": Set {}, "edgesOut": Map { "a" => Edge { @@ -838,6 +870,7 @@ Edge { "from": Object { "addEdgeIn": Function addEdgeIn(edge), "addEdgeOut": Function addEdgeOut(edge), + "deleteEdgeIn": Function deleteEdgeIn(edge), "edgesIn": Set {}, "edgesOut": Map { "aa" => Edge { @@ -855,6 +888,7 @@ Edge { "parent": Object { "addEdgeIn": Function addEdgeIn(edge), "addEdgeOut": Function addEdgeOut(edge), + "deleteEdgeIn": Function deleteEdgeIn(edge), "edgesIn": Set {}, "edgesOut": Map { "missing" => Edge { @@ -877,6 +911,7 @@ Edge { "root": Object { "addEdgeIn": Function addEdgeIn(edge), "addEdgeOut": Function addEdgeOut(edge), + "deleteEdgeIn": Function deleteEdgeIn(edge), "edgesIn": Set {}, "edgesOut": Map { "missing" => Edge { @@ -908,6 +943,7 @@ Edge { "to": Object { "addEdgeIn": Function addEdgeIn(edge), "addEdgeOut": Function addEdgeOut(edge), + "deleteEdgeIn": Function deleteEdgeIn(edge), "edgesIn": Set { Edge { "peerConflicted": false, @@ -925,6 +961,7 @@ Edge { "parent": Object { "addEdgeIn": Function addEdgeIn(edge), "addEdgeOut": Function addEdgeOut(edge), + "deleteEdgeIn": Function deleteEdgeIn(edge), "edgesIn": Set {}, "edgesOut": Map { "aa" => Edge { @@ -942,6 +979,7 @@ Edge { "parent": Object { "addEdgeIn": Function addEdgeIn(edge), "addEdgeOut": Function addEdgeOut(edge), + "deleteEdgeIn": Function deleteEdgeIn(edge), "edgesIn": Set {}, "edgesOut": Map { "missing" => Edge { @@ -964,6 +1002,7 @@ Edge { "root": Object { "addEdgeIn": Function addEdgeIn(edge), "addEdgeOut": Function addEdgeOut(edge), + "deleteEdgeIn": Function deleteEdgeIn(edge), "edgesIn": Set {}, "edgesOut": Map { "missing" => Edge { @@ -1000,6 +1039,7 @@ Edge { "from": Object { "addEdgeIn": Function addEdgeIn(edge), "addEdgeOut": Function addEdgeOut(edge), + "deleteEdgeIn": Function deleteEdgeIn(edge), "edgesIn": Set {}, "edgesOut": Map { "b" => Edge { @@ -1017,6 +1057,7 @@ Edge { "parent": Object { "addEdgeIn": Function addEdgeIn(edge), "addEdgeOut": Function addEdgeOut(edge), + "deleteEdgeIn": Function deleteEdgeIn(edge), "edgesIn": Set {}, "edgesOut": Map { "missing" => Edge { @@ -1039,6 +1080,7 @@ Edge { "root": Object { "addEdgeIn": Function addEdgeIn(edge), "addEdgeOut": Function addEdgeOut(edge), + "deleteEdgeIn": Function deleteEdgeIn(edge), "edgesIn": Set {}, "edgesOut": Map { "missing" => Edge { @@ -1070,6 +1112,7 @@ Edge { "to": Object { "addEdgeIn": Function addEdgeIn(edge), "addEdgeOut": Function addEdgeOut(edge), + "deleteEdgeIn": Function deleteEdgeIn(edge), "edgesIn": Set { Edge { "peerConflicted": false, @@ -1087,6 +1130,7 @@ Edge { "parent": Object { "addEdgeIn": Function addEdgeIn(edge), "addEdgeOut": Function addEdgeOut(edge), + "deleteEdgeIn": Function deleteEdgeIn(edge), "edgesIn": Set {}, "edgesOut": Map { "missing" => Edge { diff --git a/workspaces/arborist/test/edge.js b/workspaces/arborist/test/edge.js index ab08357ece359..9a2e5791b4de1 100644 --- a/workspaces/arborist/test/edge.js +++ b/workspaces/arborist/test/edge.js @@ -57,6 +57,9 @@ const top = { addEdgeIn (edge) { this.edgesIn.add(edge) }, + deleteEdgeIn (edge) { + this.edgesIn.delete(edge) + }, } const a = { @@ -81,6 +84,9 @@ const a = { addEdgeIn (edge) { this.edgesIn.add(edge) }, + deleteEdgeIn (edge) { + this.edgesIn.delete(edge) + }, } const b = { @@ -104,6 +110,9 @@ const b = { addEdgeIn (edge) { this.edgesIn.add(edge) }, + deleteEdgeIn (edge) { + this.edgesIn.delete(edge) + }, } const bb = { @@ -127,6 +136,9 @@ const bb = { addEdgeIn (edge) { this.edgesIn.add(edge) }, + deleteEdgeIn (edge) { + this.edgesIn.delete(edge) + }, } const aa = { @@ -150,6 +162,9 @@ const aa = { addEdgeIn (edge) { this.edgesIn.add(edge) }, + deleteEdgeIn (edge) { + this.edgesIn.delete(edge) + }, } const c = { @@ -173,6 +188,9 @@ const c = { addEdgeIn (edge) { this.edgesIn.add(edge) }, + deleteEdgeIn (edge) { + this.edgesIn.delete(edge) + }, } t.matchSnapshot(new Edge({ @@ -364,6 +382,9 @@ const referenceTop = { addEdgeIn (edge) { this.edgesIn.add(edge) }, + deleteEdgeIn (edge) { + this.edgesIn.delete(edge) + }, overrides: new OverrideSet({ overrides: { referenceGrandchild: '$referenceChild', @@ -403,6 +424,9 @@ const referenceChild = { this.overrides = edge.overrides this.edgesIn.add(edge) }, + deleteEdgeIn (edge) { + this.edgesIn.delete(edge) + }, } new Edge({ @@ -442,6 +466,9 @@ const referenceGrandchild = { this.overrides = edge.overrides this.edgesIn.add(edge) }, + deleteEdgeIn (edge) { + this.edgesIn.delete(edge) + }, } const referenceGrandchildEdge = new Edge({ @@ -490,6 +517,9 @@ const badOverride = { addEdgeIn (edge) { this.edgesIn.add(edge) }, + deleteEdgeIn (edge) { + this.edgesIn.delete(edge) + }, overrides: new OverrideSet({ overrides: { b: '1.x', @@ -775,6 +805,9 @@ const bundleChild = { addEdgeIn (edge) { this.edgesIn.add(edge) }, + deleteEdgeIn (edge) { + this.edgesIn.delete(edge) + }, } const bundleParent = { @@ -797,6 +830,9 @@ const bundleParent = { addEdgeIn (edge) { this.edgesIn.add(edge) }, + deleteEdgeIn (edge) { + this.edgesIn.delete(edge) + }, } const bundledEdge = new Edge({ @@ -858,6 +894,9 @@ t.test('override references find the correct root', (t) => { addEdgeIn (edge) { this.edgesIn.add(edge) }, + deleteEdgeIn (edge) { + this.edgesIn.delete(edge) + }, } const foo = { @@ -885,6 +924,9 @@ t.test('override references find the correct root', (t) => { addEdgeIn (edge) { this.edgesIn.add(edge) }, + deleteEdgeIn (edge) { + this.edgesIn.delete(edge) + }, } foo.overrides = overrides.getNodeRule(foo) @@ -915,6 +957,9 @@ t.test('override references find the correct root', (t) => { addEdgeIn (edge) { this.edgesIn.add(edge) }, + deleteEdgeIn (edge) { + this.edgesIn.delete(edge) + }, } bar.overrides = foo.overrides.getNodeRule(bar) @@ -946,6 +991,9 @@ t.test('override references find the correct root', (t) => { addEdgeIn (edge) { this.edgesIn.add(edge) }, + deleteEdgeIn (edge) { + this.edgesIn.delete(edge) + }, } virtualBar.overrides = overrides @@ -999,6 +1047,9 @@ t.test('shrinkwrapped and bundled deps are not overridden and remain valid', (t) addEdgeIn (edge) { this.edgesIn.add(edge) }, + deleteEdgeIn (edge) { + this.edgesIn.delete(edge) + }, } const foo = { @@ -1029,6 +1080,9 @@ t.test('shrinkwrapped and bundled deps are not overridden and remain valid', (t) addEdgeIn (edge) { this.edgesIn.add(edge) }, + deleteEdgeIn (edge) { + this.edgesIn.delete(edge) + }, } foo.overrides = overrides.getNodeRule(foo) @@ -1058,6 +1112,9 @@ t.test('shrinkwrapped and bundled deps are not overridden and remain valid', (t) addEdgeIn (edge) { this.edgesIn.add(edge) }, + deleteEdgeIn (edge) { + this.edgesIn.delete(edge) + }, } bar.overrides = foo.overrides.getNodeRule(bar) @@ -1072,3 +1129,69 @@ t.test('shrinkwrapped and bundled deps are not overridden and remain valid', (t) t.ok(edge.valid, 'edge is valid') t.end() }) + +t.test('overrideset comparison logic', (t) => { + const overrides1 = new OverrideSet({ + overrides: { + bar: '^2.0.0', + }, + }) + + const overrides2 = new OverrideSet({ + overrides: { + bar: '^2.0.0', + }, + }) + + const overrides3 = new OverrideSet({ + overrides: { + foo: '^2.0.0', + }, + }) + + const overrides4 = new OverrideSet({ + overrides: { + foo: '^1.0.0', + }, + }) + + const overrides5 = new OverrideSet({ + overrides: { + bar: '^2.0.0', + foo: '^2.0.0', + }, + }) + + const overrides6 = new OverrideSet({ + overrides: { + }, + }) + + const overrides7 = new OverrideSet({ + overrides: { + bar: { + '.': '^2.0.0', + baz: '1.2.3', + }, + }, + }) + + t.ok(overrides1.isEqual(overrides1), 'overridesets are equal') + t.ok(overrides1.isEqual(overrides2), 'overridesets are equal') + t.ok(!overrides1.isEqual(overrides3), 'overridesets are different') + t.ok(!overrides1.isEqual(overrides5), 'overridesets are different') + t.ok(!overrides1.isEqual(overrides6), 'overridesets are different') + t.ok(!overrides1.isEqual(overrides7), 'overridesets are different') + t.ok(!overrides3.isEqual(overrides1), 'overridesets are different') + t.ok(!overrides3.isEqual(overrides4), 'overridesets are different') + t.ok(!overrides3.isEqual(overrides5), 'overridesets are different') + t.ok(!overrides4.isEqual(overrides5), 'overridesets are different') + t.ok(!overrides5.isEqual(overrides1), 'overridesets are different') + t.ok(!overrides5.isEqual(overrides3), 'overridesets are different') + t.ok(!overrides5.isEqual(overrides6), 'overridesets are different') + t.ok(!overrides6.isEqual(overrides1), 'overridesets are different') + t.ok(!overrides6.isEqual(overrides3), 'overridesets are different') + t.ok(overrides6.isEqual(overrides6), 'overridesets are equal') + t.ok(!overrides7.isEqual(overrides1), 'overridesets are different') + t.end() +}) From 091742184d3be69694ce93bbbbc3a5077202f019 Mon Sep 17 00:00:00 2001 From: Alon Navon Date: Sun, 27 Oct 2024 21:29:38 +0200 Subject: [PATCH 11/52] Overrides aren't inherited from parent, but from to nodes --- workspaces/arborist/lib/node.js | 4 ---- 1 file changed, 4 deletions(-) diff --git a/workspaces/arborist/lib/node.js b/workspaces/arborist/lib/node.js index d6702bffab388..ed43b50e4bc9b 100644 --- a/workspaces/arborist/lib/node.js +++ b/workspaces/arborist/lib/node.js @@ -1247,10 +1247,6 @@ class Node { this[_changePath](newPath) } - if (parent.overrides) { - this.overrides = parent.overrides.getNodeRule(this) - } - // clobbers anything at that path, resets all appropriate references this.root = parent.root } From 6d0ad42d5320871d2708cdb80ba00b263a923f9d Mon Sep 17 00:00:00 2001 From: Alon Navon Date: Mon, 28 Oct 2024 23:26:43 +0200 Subject: [PATCH 12/52] Always use raw spec when analyzing overrides --- workspaces/arborist/lib/override-set.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/workspaces/arborist/lib/override-set.js b/workspaces/arborist/lib/override-set.js index a03ed5133aa59..8a64333204f9a 100644 --- a/workspaces/arborist/lib/override-set.js +++ b/workspaces/arborist/lib/override-set.js @@ -92,7 +92,7 @@ class OverrideSet { return rule } - let spec = npa(`${edge.name}@${edge.spec}`) + let spec = npa(`${edge.name}@${edge.rawSpec}`) if (spec.type === 'alias') { spec = spec.subSpec } From 67d7d5cfecc844ef990ca53c2134a10cbde840d4 Mon Sep 17 00:00:00 2001 From: Alon Navon Date: Mon, 28 Oct 2024 23:28:48 +0200 Subject: [PATCH 13/52] Replaceability --- workspaces/arborist/lib/node.js | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-) diff --git a/workspaces/arborist/lib/node.js b/workspaces/arborist/lib/node.js index ed43b50e4bc9b..8d5c7c7cfdad9 100644 --- a/workspaces/arborist/lib/node.js +++ b/workspaces/arborist/lib/node.js @@ -1004,10 +1004,19 @@ class Node { return false } - // XXX need to check for two root nodes? - if (node.overrides !== this.overrides) { - return false + if (this.edgesOut.size) { + // XXX need to check for two root nodes? + if (node.overrides) { + if (!node.overrides.isEqual(this.overrides)) { + return false + } + } else { + if (this.overrides) { + return false + } + } } + ignorePeers = new Set(ignorePeers) // gather up all the deps of this node and that are only depended From 643cbeac9e7fe47242026af09dfc92902b701943 Mon Sep 17 00:00:00 2001 From: Alon Navon Date: Mon, 28 Oct 2024 23:30:00 +0200 Subject: [PATCH 14/52] Overridden status --- workspaces/arborist/lib/node.js | 19 ++++++++++++++++++- 1 file changed, 18 insertions(+), 1 deletion(-) diff --git a/workspaces/arborist/lib/node.js b/workspaces/arborist/lib/node.js index 8d5c7c7cfdad9..dcb30fbac5b5b 100644 --- a/workspaces/arborist/lib/node.js +++ b/workspaces/arborist/lib/node.js @@ -342,7 +342,24 @@ class Node { } get overridden () { - return !!(this.overrides && this.overrides.value && this.overrides.name === this.name) + if (!this.overrides) { + return false + } + if (!this.overrides.value) { + return false + } + if (this.overrides.name !== this.name) { + return false + } + for (const edge of this.edgesIn) { + if (this.overrides.isEqual(edge.overrides)) { + if (!edge.overrides.isEqual(edge.from.overrides)) { + return true + } + } + } + + return false } get package () { From c5f6e4af79ca8d3d9be1d1bd93f5150c73bfadcb Mon Sep 17 00:00:00 2001 From: Alon Navon Date: Mon, 28 Oct 2024 23:30:42 +0200 Subject: [PATCH 15/52] Fix undefined bugs --- workspaces/arborist/lib/node.js | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/workspaces/arborist/lib/node.js b/workspaces/arborist/lib/node.js index dcb30fbac5b5b..5ec074e8766b1 100644 --- a/workspaces/arborist/lib/node.js +++ b/workspaces/arborist/lib/node.js @@ -1397,7 +1397,7 @@ class Node { } let newOverrideSet for (const edge of this.edgesIn) { - if (newOverrideSet) { + if (newOverrideSet && edge.overrides) { newOverrideSet = this.findSpecificOverrideSet(edge.overrides, newOverrideSet) } else { newOverrideSet = edge.overrides @@ -1424,12 +1424,12 @@ class Node { // both, one of its dependencies might need to be different depending on the edge leading to it. // However, this might cause a lot of duplication, because the conflict in the dependencies might never actually happen. updateNodeOverrideSet (otherOverrideSet) { - if (!this.overrides) { + if (!otherOverrideSet) { // Assuming there are any overrides at all, the overrides field is never undefined for any node at the end state of the tree. // So if the new edge's overrides is undefined it will be updated later. So we can wait with updating the node's overrides field. - if (!otherOverrideSet) { - return false - } + return false + } + if (!this.overrides) { this.overrides = otherOverrideSet this.recalculateOutEdgesOverrides() return true From 6c9bbf9a51743a5897d0240431fd78438605fb35 Mon Sep 17 00:00:00 2001 From: Alon Navon Date: Mon, 28 Oct 2024 23:34:08 +0200 Subject: [PATCH 16/52] Parent doesn't matter to overrides --- workspaces/arborist/lib/node.js | 3 --- 1 file changed, 3 deletions(-) diff --git a/workspaces/arborist/lib/node.js b/workspaces/arborist/lib/node.js index 5ec074e8766b1..3d20cd3f3367f 100644 --- a/workspaces/arborist/lib/node.js +++ b/workspaces/arborist/lib/node.js @@ -837,9 +837,6 @@ class Node { target.root = root } - if (!this.overrides && this.parent && this.parent.overrides) { - this.overrides = this.parent.overrides.getNodeRule(this) - } // tree should always be valid upon root setter completion. treeCheck(this) if (this !== root) { From 63760d976159bf9311b5ef60ec07ded6d955eb02 Mon Sep 17 00:00:00 2001 From: Alon Navon Date: Mon, 28 Oct 2024 23:38:27 +0200 Subject: [PATCH 17/52] Update override set in reload --- workspaces/arborist/lib/edge.js | 18 +++++++++++++++++- 1 file changed, 17 insertions(+), 1 deletion(-) diff --git a/workspaces/arborist/lib/edge.js b/workspaces/arborist/lib/edge.js index 2a8b520509b01..a0d9014b774e0 100644 --- a/workspaces/arborist/lib/edge.js +++ b/workspaces/arborist/lib/edge.js @@ -250,8 +250,20 @@ class Edge { reload (hard = false) { this.#explanation = null + let needToUpdateOverrideSet = false + let newOverrideSet + let oldOverrideSet if (this.#from.overrides) { - this.overrides = this.#from.overrides.getEdgeRule(this) + newOverrideSet = this.#from.overrides.getEdgeRule(this) + if (newOverrideSet && !newOverrideSet.isEqual(this.overrides)) { + needToUpdateOverrideSet = true + oldOverrideSet = this.overrides + this.overrides = newOverrideSet + } +// if (this.overrides !== this.#from.overrides) { +// console.log(this.#from.overrides) +// console.log(this.overrides) +// } } else { delete this.overrides } @@ -268,6 +280,10 @@ class Edge { } else if (hard) { this.#error = null } + else if (needToUpdateOverrideSet) { + this.#to.updateNodeOverrideSetDueToEdgeRemoval(oldOverrideSet) + this.#to.updateNodeOverrideSet(newOverrideSet) + } } detach () { From 61116300ba79fedb222d7793a9ce6f62ed32b33f Mon Sep 17 00:00:00 2001 From: Alon Navon Date: Mon, 28 Oct 2024 23:42:56 +0200 Subject: [PATCH 18/52] Erroneous node --- workspaces/arborist/lib/edge.js | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/workspaces/arborist/lib/edge.js b/workspaces/arborist/lib/edge.js index a0d9014b774e0..86c884eaea8d3 100644 --- a/workspaces/arborist/lib/edge.js +++ b/workspaces/arborist/lib/edge.js @@ -179,6 +179,9 @@ class Edge { get spec () { if (this.overrides?.value && this.overrides.value !== '*' && this.overrides.name === this.#name) { + if (this.overrides === this.#from.overrides) { + return this.#spec + } if (this.overrides.value.startsWith('$')) { const ref = this.overrides.value.slice(1) // we may be a virtual root, if we are we want to resolve reference overrides @@ -238,6 +241,12 @@ class Edge { this.#error = 'PEER LOCAL' } else if (!this.satisfiedBy(this.#to)) { this.#error = 'INVALID' + } else if (this.overrides && this.#to.edgesOut.size) { + if (!this.#to.findSpecificOverrideSet(this.overrides, this.#to.overrides)) { + // In principle any kind of difference here has some potential for problem, + // but we'll say it's invalid only if the override sets are plainly conflicting. + this.#error = 'INVALID' + } } else { this.#error = 'OK' } @@ -260,10 +269,6 @@ class Edge { oldOverrideSet = this.overrides this.overrides = newOverrideSet } -// if (this.overrides !== this.#from.overrides) { -// console.log(this.#from.overrides) -// console.log(this.overrides) -// } } else { delete this.overrides } From b42d4c8b9b8aff4fa0e898f9977887dcfd8e8715 Mon Sep 17 00:00:00 2001 From: Alon Navon Date: Tue, 29 Oct 2024 19:22:49 +0200 Subject: [PATCH 19/52] Add comments --- workspaces/arborist/lib/edge.js | 21 +++++++++++++-------- workspaces/arborist/lib/node.js | 16 +++++++++++----- workspaces/arborist/lib/override-set.js | 1 + 3 files changed, 25 insertions(+), 13 deletions(-) diff --git a/workspaces/arborist/lib/edge.js b/workspaces/arborist/lib/edge.js index 86c884eaea8d3..fd33e986990bd 100644 --- a/workspaces/arborist/lib/edge.js +++ b/workspaces/arborist/lib/edge.js @@ -179,9 +179,11 @@ class Edge { get spec () { if (this.overrides?.value && this.overrides.value !== '*' && this.overrides.name === this.#name) { + // If this edge has the same overrides field as the source, then we're not applying an override for this edge. if (this.overrides === this.#from.overrides) { return this.#spec } + if (this.overrides.value.startsWith('$')) { const ref = this.overrides.value.slice(1) // we may be a virtual root, if we are we want to resolve reference overrides @@ -241,12 +243,11 @@ class Edge { this.#error = 'PEER LOCAL' } else if (!this.satisfiedBy(this.#to)) { this.#error = 'INVALID' - } else if (this.overrides && this.#to.edgesOut.size) { - if (!this.#to.findSpecificOverrideSet(this.overrides, this.#to.overrides)) { - // In principle any kind of difference here has some potential for problem, - // but we'll say it's invalid only if the override sets are plainly conflicting. - this.#error = 'INVALID' - } + } else if (this.overrides && this.#to.edgesOut.size && !this.#to.findSpecificOverrideSet(this.overrides, this.#to.overrides)) { + // Any inconsistency between the edge's override set and the target's override set is potentially problematic. + // But we only say the edge is in error if the override sets are plainly conflicting. + // Note that if the target doesn't have any dependencies of their own, then this inconsistency is irrelevant. + this.#error = 'INVALID' } else { this.#error = 'OK' } @@ -259,12 +260,15 @@ class Edge { reload (hard = false) { this.#explanation = null + let needToUpdateOverrideSet = false let newOverrideSet let oldOverrideSet if (this.#from.overrides) { newOverrideSet = this.#from.overrides.getEdgeRule(this) if (newOverrideSet && !newOverrideSet.isEqual(this.overrides)) { + // If there's a new different override set we need to propagate it to the nodes. + // If we're deleting the override set then there's no point propagating it right now since it will be filled with another value later. needToUpdateOverrideSet = true oldOverrideSet = this.overrides this.overrides = newOverrideSet @@ -286,8 +290,9 @@ class Edge { this.#error = null } else if (needToUpdateOverrideSet) { - this.#to.updateNodeOverrideSetDueToEdgeRemoval(oldOverrideSet) - this.#to.updateNodeOverrideSet(newOverrideSet) + // Propagate the new override set to the target node. + this.#to.updateOverridesEdgeInRemoved(oldOverrideSet) + this.#to.updateOverridesEdgeInAdded(newOverrideSet) } } diff --git a/workspaces/arborist/lib/node.js b/workspaces/arborist/lib/node.js index 3d20cd3f3367f..1f8887c32de18 100644 --- a/workspaces/arborist/lib/node.js +++ b/workspaces/arborist/lib/node.js @@ -351,6 +351,10 @@ class Node { if (this.overrides.name !== this.name) { return false } + + // The overrides rule is for a package with this name, but some override rules only apply to specific + // versions. To make sure this package was actually overridden, we check whether any edge going in + // had the rule applied to it, in which case its overrides set is different than its source node. for (const edge of this.edgesIn) { if (this.overrides.isEqual(edge.overrides)) { if (!edge.overrides.isEqual(edge.from.overrides)) { @@ -1018,6 +1022,8 @@ class Node { return false } + // If this node has no dependencies, then it's irrelevant to check the override + // rules of the replacement node. if (this.edgesOut.size) { // XXX need to check for two root nodes? if (node.overrides) { @@ -1368,7 +1374,7 @@ class Node { for (const [, edge] of this.edgesOut) { edge.reload(true) if (edge.to) { - edge.to.updateNodeOverrideSet(edge.overrides) + edge.to.updateOverridesEdgeInAdded(edge.overrides) } } } @@ -1387,7 +1393,7 @@ class Node { console.log('Conflicting override sets') } - updateNodeOverrideSetDueToEdgeRemoval (otherOverrideSet) { + updateOverridesEdgeInRemoved (otherOverrideSet) { // If this edge's overrides isn't equal to this node's overrides, then removing it won't change newOverrideSet later. if (!this.overrides || !this.overrides.isEqual(otherOverrideSet)) { return false @@ -1420,7 +1426,7 @@ class Node { // The strictly correct logic is not to allow two edges with different overrides to point to the same node, because even if this node can satisfy // both, one of its dependencies might need to be different depending on the edge leading to it. // However, this might cause a lot of duplication, because the conflict in the dependencies might never actually happen. - updateNodeOverrideSet (otherOverrideSet) { + updateOverridesEdgeInAdded (otherOverrideSet) { if (!otherOverrideSet) { // Assuming there are any overrides at all, the overrides field is never undefined for any node at the end state of the tree. // So if the new edge's overrides is undefined it will be updated later. So we can wait with updating the node's overrides field. @@ -1449,14 +1455,14 @@ class Node { deleteEdgeIn (edge) { this.edgesIn.delete(edge) if (edge.overrides) { - this.updateNodeOverrideSetDueToEdgeRemoval(edge.overrides) + this.updateOverridesEdgeInRemoved(edge.overrides) } } addEdgeIn (edge) { // We need to handle the case where the new edge in has an overrides field which is different from the current value. if (!this.overrides || !this.overrides.isEqual(edge.overrides)) { - this.updateNodeOverrideSet(edge.overrides) + this.updateOverridesEdgeInAdded(edge.overrides) } this.edgesIn.add(edge) diff --git a/workspaces/arborist/lib/override-set.js b/workspaces/arborist/lib/override-set.js index 8a64333204f9a..48a43ebbcf6ac 100644 --- a/workspaces/arborist/lib/override-set.js +++ b/workspaces/arborist/lib/override-set.js @@ -92,6 +92,7 @@ class OverrideSet { return rule } + // We need to use the rawSpec here, because the spec has the overrides applied to it already. let spec = npa(`${edge.name}@${edge.rawSpec}`) if (spec.type === 'alias') { spec = spec.subSpec From 7d977263e14949f41febc5662c6235e10570f34c Mon Sep 17 00:00:00 2001 From: Alon Navon Date: Mon, 4 Nov 2024 16:28:31 +0200 Subject: [PATCH 20/52] Code review fixes --- workspaces/arborist/lib/node.js | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/workspaces/arborist/lib/node.js b/workspaces/arborist/lib/node.js index 1f8887c32de18..d028a22790e0a 100644 --- a/workspaces/arborist/lib/node.js +++ b/workspaces/arborist/lib/node.js @@ -65,6 +65,8 @@ const CaseInsensitiveMap = require('./case-insensitive-map.js') const querySelectorAll = require('./query-selector-all.js') +const log = require('proc-log') + class Node { #global #meta @@ -1390,7 +1392,7 @@ class Node { return first } } - console.log('Conflicting override sets') + log.silly('Conflicting override sets', this, first, second) } updateOverridesEdgeInRemoved (otherOverrideSet) { @@ -1450,6 +1452,7 @@ class Node { return false } // This is an error condition. We can only get here if the new override set is in conflict with the existing. + throw Object.assign(new Error(`Conflicting override requirements for node ${this.name}`), { code: 'EOVERRIDE' }) } deleteEdgeIn (edge) { From 35e5790bc6ef0f7bec3aa325f86554f73d7e4b26 Mon Sep 17 00:00:00 2001 From: Alon Navon Date: Mon, 4 Nov 2024 16:32:54 +0200 Subject: [PATCH 21/52] Fix edge satisfaction logic --- workspaces/arborist/lib/dep-valid.js | 2 +- workspaces/arborist/lib/edge.js | 26 +++++++++++++++++++++++++- workspaces/arborist/lib/node.js | 2 +- 3 files changed, 27 insertions(+), 3 deletions(-) diff --git a/workspaces/arborist/lib/dep-valid.js b/workspaces/arborist/lib/dep-valid.js index 4afb5e47cf111..4fab2d3ece835 100644 --- a/workspaces/arborist/lib/dep-valid.js +++ b/workspaces/arborist/lib/dep-valid.js @@ -101,7 +101,7 @@ const depValid = (child, requested, requestor) => { }) } - default: // unpossible, just being cautious + default: // impossible, just being cautious break } diff --git a/workspaces/arborist/lib/edge.js b/workspaces/arborist/lib/edge.js index fd33e986990bd..83c3c5d6eaf7f 100644 --- a/workspaces/arborist/lib/edge.js +++ b/workspaces/arborist/lib/edge.js @@ -112,7 +112,31 @@ class Edge { if (node.hasShrinkwrap || node.inShrinkwrap || node.inBundle) { return depValid(node, this.rawSpec, this.#accept, this.#from) } - return depValid(node, this.spec, this.#accept, this.#from) + + // If there's no override we just use the spec. + if (!this.overridden) { + return depValid(node, this.spec, this.#accept, this.#from) + } + // There's some override. If the target node satisfies the overriding spec + // then it's okay. + if (depValid(node, this.spec, this.#accept, this.#from)) { + return true + } + // If it doesn't, then it should at least satisfy the original spec. + if (!depValid(node, this.rawSpec, this.#accept, this.#from)) { + return false + } + // It satisfies the original spec, not the overriding spec. We need to make + // sure it doesn't use the overridden spec. + // For example: + // we might have an ^8.0.0 rawSpec, and an override that makes + // keySpec=8.23.0 and the override value spec=9.0.0. + // If the node is 9.0.0, then it's okay because it's consistent with spec. + // If the node is 8.24.0, then it's okay because it's consistent with the rawSpec. + // If the node is 8.23.0, then it's not okay because even though it's consistent + // with the rawSpec, it's also consistent with the keySpec. + // So we're looking for ^8.0.0 or 9.0.0 and not 8.23.0. + return !depValid(node, this.overrides.keySpec, this.#accept, this.#from) } // return the edge data, and an explanation of how that edge came to be here diff --git a/workspaces/arborist/lib/node.js b/workspaces/arborist/lib/node.js index d028a22790e0a..f2c0459769ba5 100644 --- a/workspaces/arborist/lib/node.js +++ b/workspaces/arborist/lib/node.js @@ -358,7 +358,7 @@ class Node { // versions. To make sure this package was actually overridden, we check whether any edge going in // had the rule applied to it, in which case its overrides set is different than its source node. for (const edge of this.edgesIn) { - if (this.overrides.isEqual(edge.overrides)) { + if (edge.overrides && edge.overrides.name === this.name && edge.overrides.value === this.version) { if (!edge.overrides.isEqual(edge.from.overrides)) { return true } From 35ce2f743aaa68dda908467843153f46e209cf0a Mon Sep 17 00:00:00 2001 From: Alon Navon Date: Mon, 4 Nov 2024 16:34:28 +0200 Subject: [PATCH 22/52] Fix dedupe logic --- workspaces/arborist/lib/node.js | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/workspaces/arborist/lib/node.js b/workspaces/arborist/lib/node.js index f2c0459769ba5..5d346a246c1cf 100644 --- a/workspaces/arborist/lib/node.js +++ b/workspaces/arborist/lib/node.js @@ -1106,8 +1106,13 @@ class Node { return false } - // if we prefer dedupe, or if the version is greater/equal, take the other - if (preferDedupe || semver.gte(other.version, this.version)) { + // if we prefer dedupe, or if the version is equal, take the other + if (preferDedupe || semver.eq(other.version, this.version)) { + return true + } + + // if our current version isn't the result of an override, then prefer to take the greater version + if (!this.overridden && semver.gt(other.version, this.version)) { return true } From 56309540cd0a153666ca16fec0d550e1d7853c03 Mon Sep 17 00:00:00 2001 From: Alon Navon Date: Mon, 4 Nov 2024 18:46:17 +0200 Subject: [PATCH 23/52] Code review fixes --- workspaces/arborist/lib/edge.js | 12 ++++++++---- workspaces/arborist/lib/node.js | 4 ++-- 2 files changed, 10 insertions(+), 6 deletions(-) diff --git a/workspaces/arborist/lib/edge.js b/workspaces/arborist/lib/edge.js index 83c3c5d6eaf7f..d547fda715dad 100644 --- a/workspaces/arborist/lib/edge.js +++ b/workspaces/arborist/lib/edge.js @@ -204,7 +204,7 @@ class Edge { get spec () { if (this.overrides?.value && this.overrides.value !== '*' && this.overrides.name === this.#name) { // If this edge has the same overrides field as the source, then we're not applying an override for this edge. - if (this.overrides === this.#from.overrides) { + if (this.overrides === this.#from?.overrides) { return this.#spec } @@ -212,9 +212,9 @@ class Edge { const ref = this.overrides.value.slice(1) // we may be a virtual root, if we are we want to resolve reference overrides // from the real root, not the virtual one - const pkg = this.#from.sourceReference - ? this.#from.sourceReference.root.package - : this.#from.root.package + const pkg = this.#from?.sourceReference + ? this.#from?.sourceReference.root.package + : this.#from?.root?.package if (pkg.devDependencies?.[ref]) { return pkg.devDependencies[ref] } @@ -264,6 +264,7 @@ class Edge { this.#error = 'MISSING' } } else if (this.peer && this.#from === this.#to.parent && !this.#from.isTop) { + } else if (this.peer && this.#from === this.#to.parent && !this.#from?.isTop) { this.#error = 'PEER LOCAL' } else if (!this.satisfiedBy(this.#to)) { this.#error = 'INVALID' @@ -289,6 +290,7 @@ class Edge { let newOverrideSet let oldOverrideSet if (this.#from.overrides) { + if (this.#from?.overrides) { newOverrideSet = this.#from.overrides.getEdgeRule(this) if (newOverrideSet && !newOverrideSet.isEqual(this.overrides)) { // If there's a new different override set we need to propagate it to the nodes. @@ -301,6 +303,7 @@ class Edge { delete this.overrides } const newTo = this.#from.resolve(this.#name) + const newTo = this.#from?.resolve(this.#name) if (newTo !== this.#to) { if (this.#to) { this.#to.deleteEdgeIn(this) @@ -326,6 +329,7 @@ class Edge { this.#to.deleteEdgeIn(this) } this.#from.edgesOut.delete(this.#name) + this.#from?.edgesOut.delete(this.#name) this.#to = null this.#error = 'DETACHED' this.#from = null diff --git a/workspaces/arborist/lib/node.js b/workspaces/arborist/lib/node.js index 5d346a246c1cf..7b2c837cfaacd 100644 --- a/workspaces/arborist/lib/node.js +++ b/workspaces/arborist/lib/node.js @@ -1378,7 +1378,7 @@ class Node { recalculateOutEdgesOverrides () { // For each edge out propogate the new overrides through. - for (const [, edge] of this.edgesOut) { + for (const edge of this.edgesOut.values()) { edge.reload(true) if (edge.to) { edge.to.updateOverridesEdgeInAdded(edge.overrides) @@ -1386,7 +1386,7 @@ class Node { } } - findSpecificOverrideSet (first, second) { + static findSpecificOverrideSet (first, second) { for (let overrideSet = second; overrideSet; overrideSet = overrideSet.parent) { if (overrideSet.isEqual(first)) { return second From f859e926f26f521bd8fd089a135aa1bfc6600b41 Mon Sep 17 00:00:00 2001 From: Alon Navon Date: Mon, 4 Nov 2024 19:43:48 +0200 Subject: [PATCH 24/52] Oops --- workspaces/arborist/lib/edge.js | 4 ---- 1 file changed, 4 deletions(-) diff --git a/workspaces/arborist/lib/edge.js b/workspaces/arborist/lib/edge.js index d547fda715dad..8fab290e5c174 100644 --- a/workspaces/arborist/lib/edge.js +++ b/workspaces/arborist/lib/edge.js @@ -263,7 +263,6 @@ class Edge { } else { this.#error = 'MISSING' } - } else if (this.peer && this.#from === this.#to.parent && !this.#from.isTop) { } else if (this.peer && this.#from === this.#to.parent && !this.#from?.isTop) { this.#error = 'PEER LOCAL' } else if (!this.satisfiedBy(this.#to)) { @@ -289,7 +288,6 @@ class Edge { let needToUpdateOverrideSet = false let newOverrideSet let oldOverrideSet - if (this.#from.overrides) { if (this.#from?.overrides) { newOverrideSet = this.#from.overrides.getEdgeRule(this) if (newOverrideSet && !newOverrideSet.isEqual(this.overrides)) { @@ -302,7 +300,6 @@ class Edge { } else { delete this.overrides } - const newTo = this.#from.resolve(this.#name) const newTo = this.#from?.resolve(this.#name) if (newTo !== this.#to) { if (this.#to) { @@ -328,7 +325,6 @@ class Edge { if (this.#to) { this.#to.deleteEdgeIn(this) } - this.#from.edgesOut.delete(this.#name) this.#from?.edgesOut.delete(this.#name) this.#to = null this.#error = 'DETACHED' From 29f07506bc4c19ea16705d8a76ecee0a082129e1 Mon Sep 17 00:00:00 2001 From: Alon Navon Date: Tue, 5 Nov 2024 14:54:54 +0200 Subject: [PATCH 25/52] Static function --- workspaces/arborist/lib/edge.js | 2 +- workspaces/arborist/lib/node.js | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/workspaces/arborist/lib/edge.js b/workspaces/arborist/lib/edge.js index 8fab290e5c174..9b404d2028e5d 100644 --- a/workspaces/arborist/lib/edge.js +++ b/workspaces/arborist/lib/edge.js @@ -267,7 +267,7 @@ class Edge { this.#error = 'PEER LOCAL' } else if (!this.satisfiedBy(this.#to)) { this.#error = 'INVALID' - } else if (this.overrides && this.#to.edgesOut.size && !this.#to.findSpecificOverrideSet(this.overrides, this.#to.overrides)) { + } else if (this.overrides && this.#to.edgesOut.size && !this.#to.constructor.findSpecificOverrideSet(this.overrides, this.#to.overrides)) { // Any inconsistency between the edge's override set and the target's override set is potentially problematic. // But we only say the edge is in error if the override sets are plainly conflicting. // Note that if the target doesn't have any dependencies of their own, then this inconsistency is irrelevant. diff --git a/workspaces/arborist/lib/node.js b/workspaces/arborist/lib/node.js index 7b2c837cfaacd..a7a6304a4ce34 100644 --- a/workspaces/arborist/lib/node.js +++ b/workspaces/arborist/lib/node.js @@ -1408,7 +1408,7 @@ class Node { let newOverrideSet for (const edge of this.edgesIn) { if (newOverrideSet && edge.overrides) { - newOverrideSet = this.findSpecificOverrideSet(edge.overrides, newOverrideSet) + newOverrideSet = Node.findSpecificOverrideSet(edge.overrides, newOverrideSet) } else { newOverrideSet = edge.overrides } @@ -1447,7 +1447,7 @@ class Node { if (this.overrides.isEqual(otherOverrideSet)) { return false } - const newOverrideSet = this.findSpecificOverrideSet(this.overrides, otherOverrideSet) + const newOverrideSet = Node.findSpecificOverrideSet(this.overrides, otherOverrideSet) if (newOverrideSet) { if (!this.overrides.isEqual(newOverrideSet)) { this.overrides = newOverrideSet From 17ec4571f1ba9e8db3b4f9fce1115e3c2568d105 Mon Sep 17 00:00:00 2001 From: Alon Navon Date: Tue, 5 Nov 2024 19:26:38 +0200 Subject: [PATCH 26/52] Typo --- workspaces/arborist/lib/edge.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/workspaces/arborist/lib/edge.js b/workspaces/arborist/lib/edge.js index 9b404d2028e5d..2ab3dd9227187 100644 --- a/workspaces/arborist/lib/edge.js +++ b/workspaces/arborist/lib/edge.js @@ -114,7 +114,7 @@ class Edge { } // If there's no override we just use the spec. - if (!this.overridden) { + if (!this.overrides) { return depValid(node, this.spec, this.#accept, this.#from) } // There's some override. If the target node satisfies the overriding spec From 4cde831b7f2be489bf9f8576b977a4719774bc37 Mon Sep 17 00:00:00 2001 From: Alon Navon Date: Thu, 7 Nov 2024 20:13:27 +0200 Subject: [PATCH 27/52] Shouldn't throw --- workspaces/arborist/lib/edge.js | 2 +- workspaces/arborist/lib/node.js | 6 +++++- 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/workspaces/arborist/lib/edge.js b/workspaces/arborist/lib/edge.js index 2ab3dd9227187..55468bba15b3c 100644 --- a/workspaces/arborist/lib/edge.js +++ b/workspaces/arborist/lib/edge.js @@ -267,7 +267,7 @@ class Edge { this.#error = 'PEER LOCAL' } else if (!this.satisfiedBy(this.#to)) { this.#error = 'INVALID' - } else if (this.overrides && this.#to.edgesOut.size && !this.#to.constructor.findSpecificOverrideSet(this.overrides, this.#to.overrides)) { + } else if (this.overrides && this.#to.edgesOut.size && this.#to.constructor.doOverrideSetsConflict(this.overrides, this.#to.overrides)) { // Any inconsistency between the edge's override set and the target's override set is potentially problematic. // But we only say the edge is in error if the override sets are plainly conflicting. // Note that if the target doesn't have any dependencies of their own, then this inconsistency is irrelevant. diff --git a/workspaces/arborist/lib/node.js b/workspaces/arborist/lib/node.js index a7a6304a4ce34..836cc0dd6c69f 100644 --- a/workspaces/arborist/lib/node.js +++ b/workspaces/arborist/lib/node.js @@ -1400,6 +1400,10 @@ class Node { log.silly('Conflicting override sets', this, first, second) } + static doOverrideSetsConflict (first, second) { + return (this.findSpecificOverrideSet(first, second) === undefined) + } + updateOverridesEdgeInRemoved (otherOverrideSet) { // If this edge's overrides isn't equal to this node's overrides, then removing it won't change newOverrideSet later. if (!this.overrides || !this.overrides.isEqual(otherOverrideSet)) { @@ -1457,7 +1461,7 @@ class Node { return false } // This is an error condition. We can only get here if the new override set is in conflict with the existing. - throw Object.assign(new Error(`Conflicting override requirements for node ${this.name}`), { code: 'EOVERRIDE' }) + log.silly('Conflicting override sets', this.name) } deleteEdgeIn (edge) { From 701b125b9ae3b02d7132646fe5ccede86ddabe52 Mon Sep 17 00:00:00 2001 From: Alon Navon Date: Thu, 7 Nov 2024 20:23:47 +0200 Subject: [PATCH 28/52] CR fixes --- workspaces/arborist/lib/edge.js | 2 ++ workspaces/arborist/lib/node.js | 2 +- 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/workspaces/arborist/lib/edge.js b/workspaces/arborist/lib/edge.js index 55468bba15b3c..b27100e42c15e 100644 --- a/workspaces/arborist/lib/edge.js +++ b/workspaces/arborist/lib/edge.js @@ -104,6 +104,7 @@ class Edge { satisfiedBy (node) { if (node.name !== this.#name) { + if (node.name !== this.#name || !this.#from) { return false } @@ -115,6 +116,7 @@ class Edge { // If there's no override we just use the spec. if (!this.overrides) { + if (!this.overrides?.keySpec) { return depValid(node, this.spec, this.#accept, this.#from) } // There's some override. If the target node satisfies the overriding spec diff --git a/workspaces/arborist/lib/node.js b/workspaces/arborist/lib/node.js index 836cc0dd6c69f..0779d9eab6b72 100644 --- a/workspaces/arborist/lib/node.js +++ b/workspaces/arborist/lib/node.js @@ -1397,7 +1397,7 @@ class Node { return first } } - log.silly('Conflicting override sets', this, first, second) + log.silly('Conflicting override sets', first, second) } static doOverrideSetsConflict (first, second) { From b119f67abc640edcafaeffbed7b2caf99d2660f6 Mon Sep 17 00:00:00 2001 From: Alon Navon Date: Thu, 7 Nov 2024 20:35:08 +0200 Subject: [PATCH 29/52] CR fixes --- workspaces/arborist/lib/edge.js | 2 -- 1 file changed, 2 deletions(-) diff --git a/workspaces/arborist/lib/edge.js b/workspaces/arborist/lib/edge.js index b27100e42c15e..1f42c2e8ea209 100644 --- a/workspaces/arborist/lib/edge.js +++ b/workspaces/arborist/lib/edge.js @@ -103,7 +103,6 @@ class Edge { } satisfiedBy (node) { - if (node.name !== this.#name) { if (node.name !== this.#name || !this.#from) { return false } @@ -115,7 +114,6 @@ class Edge { } // If there's no override we just use the spec. - if (!this.overrides) { if (!this.overrides?.keySpec) { return depValid(node, this.spec, this.#accept, this.#from) } From 3136b644c27d62060ea0741129ec6de7cf6fdf9a Mon Sep 17 00:00:00 2001 From: Alon Navon Date: Sun, 10 Nov 2024 16:39:08 +0200 Subject: [PATCH 30/52] Move functions --- workspaces/arborist/lib/edge.js | 3 ++- workspaces/arborist/lib/node.js | 22 ++-------------------- workspaces/arborist/lib/override-set.js | 19 +++++++++++++++++++ 3 files changed, 23 insertions(+), 21 deletions(-) diff --git a/workspaces/arborist/lib/edge.js b/workspaces/arborist/lib/edge.js index 1f42c2e8ea209..77f351ed45789 100644 --- a/workspaces/arborist/lib/edge.js +++ b/workspaces/arborist/lib/edge.js @@ -4,6 +4,7 @@ const util = require('util') const npa = require('npm-package-arg') const depValid = require('./dep-valid.js') +const OverrideSet = require('./override-set.js') class ArboristEdge { constructor (edge) { @@ -267,7 +268,7 @@ class Edge { this.#error = 'PEER LOCAL' } else if (!this.satisfiedBy(this.#to)) { this.#error = 'INVALID' - } else if (this.overrides && this.#to.edgesOut.size && this.#to.constructor.doOverrideSetsConflict(this.overrides, this.#to.overrides)) { + } else if (this.overrides && this.#to.edgesOut.size && OverrideSet.doOverrideSetsConflict(this.overrides, this.#to.overrides)) { // Any inconsistency between the edge's override set and the target's override set is potentially problematic. // But we only say the edge is in error if the override sets are plainly conflicting. // Note that if the target doesn't have any dependencies of their own, then this inconsistency is irrelevant. diff --git a/workspaces/arborist/lib/node.js b/workspaces/arborist/lib/node.js index 0779d9eab6b72..0640c3c0f441d 100644 --- a/workspaces/arborist/lib/node.js +++ b/workspaces/arborist/lib/node.js @@ -1386,24 +1386,6 @@ class Node { } } - static findSpecificOverrideSet (first, second) { - for (let overrideSet = second; overrideSet; overrideSet = overrideSet.parent) { - if (overrideSet.isEqual(first)) { - return second - } - } - for (let overrideSet = first; overrideSet; overrideSet = overrideSet.parent) { - if (overrideSet.isEqual(second)) { - return first - } - } - log.silly('Conflicting override sets', first, second) - } - - static doOverrideSetsConflict (first, second) { - return (this.findSpecificOverrideSet(first, second) === undefined) - } - updateOverridesEdgeInRemoved (otherOverrideSet) { // If this edge's overrides isn't equal to this node's overrides, then removing it won't change newOverrideSet later. if (!this.overrides || !this.overrides.isEqual(otherOverrideSet)) { @@ -1412,7 +1394,7 @@ class Node { let newOverrideSet for (const edge of this.edgesIn) { if (newOverrideSet && edge.overrides) { - newOverrideSet = Node.findSpecificOverrideSet(edge.overrides, newOverrideSet) + newOverrideSet = OverrideSet.findSpecificOverrideSet(edge.overrides, newOverrideSet) } else { newOverrideSet = edge.overrides } @@ -1451,7 +1433,7 @@ class Node { if (this.overrides.isEqual(otherOverrideSet)) { return false } - const newOverrideSet = Node.findSpecificOverrideSet(this.overrides, otherOverrideSet) + const newOverrideSet = OverrideSet.findSpecificOverrideSet(this.overrides, otherOverrideSet) if (newOverrideSet) { if (!this.overrides.isEqual(newOverrideSet)) { this.overrides = newOverrideSet diff --git a/workspaces/arborist/lib/override-set.js b/workspaces/arborist/lib/override-set.js index 48a43ebbcf6ac..9c8dd6ea72a40 100644 --- a/workspaces/arborist/lib/override-set.js +++ b/workspaces/arborist/lib/override-set.js @@ -1,5 +1,6 @@ const npa = require('npm-package-arg') const semver = require('semver') +const log = require('proc-log') class OverrideSet { constructor ({ overrides, key, parent }) { @@ -180,6 +181,24 @@ class OverrideSet { return ruleset } + + static findSpecificOverrideSet (first, second) { + for (let overrideSet = second; overrideSet; overrideSet = overrideSet.parent) { + if (overrideSet.isEqual(first)) { + return second + } + } + for (let overrideSet = first; overrideSet; overrideSet = overrideSet.parent) { + if (overrideSet.isEqual(second)) { + return first + } + } + log.silly('Conflicting override sets', first, second) + } + + static doOverrideSetsConflict (first, second) { + return (this.findSpecificOverrideSet(first, second) === undefined) + } } module.exports = OverrideSet From 80126c764f377d9136c79dd5b45b65e718ea91e8 Mon Sep 17 00:00:00 2001 From: Alon Navon Date: Mon, 11 Nov 2024 17:47:55 +0200 Subject: [PATCH 31/52] Comments --- workspaces/arborist/lib/override-set.js | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/workspaces/arborist/lib/override-set.js b/workspaces/arborist/lib/override-set.js index 9c8dd6ea72a40..82607e32c6ddc 100644 --- a/workspaces/arborist/lib/override-set.js +++ b/workspaces/arborist/lib/override-set.js @@ -193,10 +193,15 @@ class OverrideSet { return first } } + + // The override sets are incomparable. Neither one contains the other. log.silly('Conflicting override sets', first, second) } static doOverrideSetsConflict (first, second) { + // If override sets contain one another then we can try to use the more specific one. + // This is imperfect, since we may lose some of the context of the less specific override + // set. But the more specific override set is more important to propagate, so we go with it. return (this.findSpecificOverrideSet(first, second) === undefined) } } From c4b5338f44362a7400e0612ca164e4572ea989a6 Mon Sep 17 00:00:00 2001 From: Alon Navon Date: Mon, 11 Nov 2024 19:24:28 +0200 Subject: [PATCH 32/52] Override sets unit tests --- workspaces/arborist/test/override-set.js | 117 +++++++++++++++++++++++ 1 file changed, 117 insertions(+) diff --git a/workspaces/arborist/test/override-set.js b/workspaces/arborist/test/override-set.js index 705996b443b22..d34535c1dbbaa 100644 --- a/workspaces/arborist/test/override-set.js +++ b/workspaces/arborist/test/override-set.js @@ -271,4 +271,121 @@ t.test('constructor', async (t) => { const outOfRangeRule = bazEdgeRule.getEdgeRule({ name: 'buzz', spec: 'github:baz/buzz#semver:^2.0.0' }) t.equal(outOfRangeRule.name, 'baz', 'no match - returned parent') }) + + t.test('isequal and findspecificoverrideset tests', async (t) => { + const overrides1 = new OverrideSet({ + overrides: { + foo: { + bar: { + '.': '2.0.0', + baz: '3.0.0', + }, + baz: '2.0.0', + }, + bar: '1.0.0', + baz: '1.0.0', + }, + }) + const overrides2 = new OverrideSet({ + overrides: { + foo: { + bar: { + '.': '2.0.0', + baz: '3.0.0', + }, + baz: '2.0.0', + }, + bar: '1.0.0', + baz: '1.0.0', + }, + }) + const overrides3 = new OverrideSet({ + overrides: { + foo: { + bar: { + '.': '2.0.0', + baz: '3.1.0', + }, + baz: '2.0.0', + }, + bar: '1.0.0', + baz: '1.0.0', + }, + }) + const overrides4 = new OverrideSet({ + overrides: { + foo: { + bar: { + '.': '2.0.0', + }, + baz: '2.0.0', + }, + bar: '1.0.0', + baz: '1.0.0', + }, + }) + const overrides5 = new OverrideSet({ + overrides: { + foo: { + bar: { + '.': '2.0.0', + }, + bat: '2.0.0', + }, + bar: '1.0.0', + baz: '1.0.0', + }, + }) + const overrides6 = new OverrideSet({ + overrides: { + bar: { + '.': '2.0.0', + }, + bat: '2.0.0', + }, + }) + const overrides7 = new OverrideSet({ + overrides: { + bat: '2.0.0', + }, + }) + const overrides8 = new OverrideSet({ + overrides: { + bat: '1.2.0', + }, + }) + const overrides9 = new OverrideSet({ + overrides: { + 'bat@3.0.0': '1.2.0', + }, + }) + + t.ok(overrides1.isEqual(overrides1), 'override set is equal to itself') + t.ok(overrides1.isEqual(overrides2), 'two identical override sets are equal') + t.ok(!overrides1.isEqual(overrides3), 'two different override sets are not equal') + t.ok(!overrides2.isEqual(overrides3), 'two different override sets are not equal') + t.ok(!overrides3.isEqual(overrides1), 'two different override sets are not equal') + t.ok(!overrides3.isEqual(overrides2), 'two different override sets are not equal') + t.ok(!overrides4.isEqual(overrides1), 'two different override sets are not equal') + t.ok(!overrides4.isEqual(overrides2), 'two different override sets are not equal') + t.ok(!overrides4.isEqual(overrides3), 'two different override sets are not equal') + t.ok(!overrides4.isEqual(overrides5), 'two override sets that differ only by package name are not equal') + t.ok(!overrides5.isEqual(overrides4), 'two override sets that differ only by package name are not equal') + t.equal(OverrideSet.findSpecificOverrideSet(overrides5, overrides5), overrides5, "find more specific override set when the sets are identical") + t.equal(OverrideSet.findSpecificOverrideSet(overrides5, overrides6), overrides6, "find more specific override set when it's the second") + t.equal(OverrideSet.findSpecificOverrideSet(overrides6, overrides5), overrides6, "find more specific override set when it's the first") + t.ok(!OverrideSet.doOverrideSetsConflict(overrides1, overrides2), "override sets are equal") + t.ok(!OverrideSet.doOverrideSetsConflict(overrides5, overrides5), "override sets are the same object") + t.ok(!OverrideSet.doOverrideSetsConflict(overrides5, overrides6), "one override set is the specific version of the other") + t.ok(!OverrideSet.doOverrideSetsConflict(overrides6, overrides5), "one override set is the specific version of the other") + t.ok(OverrideSet.doOverrideSetsConflict(overrides5, overrides7), "no override set is the specific version of the other") + t.ok(OverrideSet.doOverrideSetsConflict(overrides7, overrides5), "no override set is the specific version of the other") + t.ok(!overrides7.isEqual(overrides8), 'two override sets that differ in the version are not equal') + t.ok(!overrides8.isEqual(overrides9), 'two override sets that differ in the range are not equal') + t.ok(!overrides7.isEqual(overrides9), 'two override sets that differ in both version and range are not equal') + t.ok(OverrideSet.doOverrideSetsConflict(overrides7, overrides8), "override sets are incomparable due to version") + t.ok(OverrideSet.doOverrideSetsConflict(overrides7, overrides9), "override sets are incomparable due to version and range") + t.ok(OverrideSet.doOverrideSetsConflict(overrides8, overrides9), "override sets are incomparable due to range") + + }) }) From 9db123265e34037a2175780868670a7c0f68c619 Mon Sep 17 00:00:00 2001 From: Alon Navon Date: Mon, 11 Nov 2024 19:48:13 +0200 Subject: [PATCH 33/52] Clarify comment --- workspaces/arborist/lib/override-set.js | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/workspaces/arborist/lib/override-set.js b/workspaces/arborist/lib/override-set.js index 82607e32c6ddc..13c5982b44414 100644 --- a/workspaces/arborist/lib/override-set.js +++ b/workspaces/arborist/lib/override-set.js @@ -200,8 +200,7 @@ class OverrideSet { static doOverrideSetsConflict (first, second) { // If override sets contain one another then we can try to use the more specific one. - // This is imperfect, since we may lose some of the context of the less specific override - // set. But the more specific override set is more important to propagate, so we go with it. + // If neither one is more specific, then we consider them to be in conflict. return (this.findSpecificOverrideSet(first, second) === undefined) } } From 3ba9932d312b50860723d786f65c6293c7cae160 Mon Sep 17 00:00:00 2001 From: Michael Smith Date: Fri, 7 Feb 2025 09:43:32 -0800 Subject: [PATCH 34/52] linter --- workspaces/arborist/lib/edge.js | 3 ++- workspaces/arborist/test/override-set.js | 21 ++++++++++----------- 2 files changed, 12 insertions(+), 12 deletions(-) diff --git a/workspaces/arborist/lib/edge.js b/workspaces/arborist/lib/edge.js index 77f351ed45789..0edd6d5b70386 100644 --- a/workspaces/arborist/lib/edge.js +++ b/workspaces/arborist/lib/edge.js @@ -311,7 +311,8 @@ class Edge { if (this.#to) { this.#to.addEdgeIn(this) } - } else if (hard) { + } + else if (hard) { this.#error = null } else if (needToUpdateOverrideSet) { diff --git a/workspaces/arborist/test/override-set.js b/workspaces/arborist/test/override-set.js index d34535c1dbbaa..8f426bf919a57 100644 --- a/workspaces/arborist/test/override-set.js +++ b/workspaces/arborist/test/override-set.js @@ -371,21 +371,20 @@ t.test('constructor', async (t) => { t.ok(!overrides4.isEqual(overrides3), 'two different override sets are not equal') t.ok(!overrides4.isEqual(overrides5), 'two override sets that differ only by package name are not equal') t.ok(!overrides5.isEqual(overrides4), 'two override sets that differ only by package name are not equal') - t.equal(OverrideSet.findSpecificOverrideSet(overrides5, overrides5), overrides5, "find more specific override set when the sets are identical") + t.equal(OverrideSet.findSpecificOverrideSet(overrides5, overrides5), overrides5, 'find more specific override set when the sets are identical') t.equal(OverrideSet.findSpecificOverrideSet(overrides5, overrides6), overrides6, "find more specific override set when it's the second") t.equal(OverrideSet.findSpecificOverrideSet(overrides6, overrides5), overrides6, "find more specific override set when it's the first") - t.ok(!OverrideSet.doOverrideSetsConflict(overrides1, overrides2), "override sets are equal") - t.ok(!OverrideSet.doOverrideSetsConflict(overrides5, overrides5), "override sets are the same object") - t.ok(!OverrideSet.doOverrideSetsConflict(overrides5, overrides6), "one override set is the specific version of the other") - t.ok(!OverrideSet.doOverrideSetsConflict(overrides6, overrides5), "one override set is the specific version of the other") - t.ok(OverrideSet.doOverrideSetsConflict(overrides5, overrides7), "no override set is the specific version of the other") - t.ok(OverrideSet.doOverrideSetsConflict(overrides7, overrides5), "no override set is the specific version of the other") + t.ok(!OverrideSet.doOverrideSetsConflict(overrides1, overrides2), 'override sets are equal') + t.ok(!OverrideSet.doOverrideSetsConflict(overrides5, overrides5), 'override sets are the same object') + t.ok(!OverrideSet.doOverrideSetsConflict(overrides5, overrides6), 'one override set is the specific version of the other') + t.ok(!OverrideSet.doOverrideSetsConflict(overrides6, overrides5), 'one override set is the specific version of the other') + t.ok(OverrideSet.doOverrideSetsConflict(overrides5, overrides7), 'no override set is the specific version of the other') + t.ok(OverrideSet.doOverrideSetsConflict(overrides7, overrides5), 'no override set is the specific version of the other') t.ok(!overrides7.isEqual(overrides8), 'two override sets that differ in the version are not equal') t.ok(!overrides8.isEqual(overrides9), 'two override sets that differ in the range are not equal') t.ok(!overrides7.isEqual(overrides9), 'two override sets that differ in both version and range are not equal') - t.ok(OverrideSet.doOverrideSetsConflict(overrides7, overrides8), "override sets are incomparable due to version") - t.ok(OverrideSet.doOverrideSetsConflict(overrides7, overrides9), "override sets are incomparable due to version and range") - t.ok(OverrideSet.doOverrideSetsConflict(overrides8, overrides9), "override sets are incomparable due to range") - + t.ok(OverrideSet.doOverrideSetsConflict(overrides7, overrides8), 'override sets are incomparable due to version') + t.ok(OverrideSet.doOverrideSetsConflict(overrides7, overrides9), 'override sets are incomparable due to version and range') + t.ok(OverrideSet.doOverrideSetsConflict(overrides8, overrides9), 'override sets are incomparable due to range') }) }) From d1b0b402774108ecab7ab64c910a05cfb72252c1 Mon Sep 17 00:00:00 2001 From: Michael Smith Date: Fri, 7 Feb 2025 09:50:05 -0800 Subject: [PATCH 35/52] linter --- workspaces/arborist/lib/edge.js | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/workspaces/arborist/lib/edge.js b/workspaces/arborist/lib/edge.js index 0edd6d5b70386..c4aafdafb6e4e 100644 --- a/workspaces/arborist/lib/edge.js +++ b/workspaces/arborist/lib/edge.js @@ -311,11 +311,9 @@ class Edge { if (this.#to) { this.#to.addEdgeIn(this) } - } - else if (hard) { + } else if (hard) { this.#error = null - } - else if (needToUpdateOverrideSet) { + } else if (needToUpdateOverrideSet) { // Propagate the new override set to the target node. this.#to.updateOverridesEdgeInRemoved(oldOverrideSet) this.#to.updateOverridesEdgeInAdded(newOverrideSet) From 3e726264038e218734d34d6738a96ee4323660d3 Mon Sep 17 00:00:00 2001 From: Michael Smith Date: Fri, 7 Feb 2025 09:53:41 -0800 Subject: [PATCH 36/52] linter --- workspaces/arborist/lib/override-set.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/workspaces/arborist/lib/override-set.js b/workspaces/arborist/lib/override-set.js index 13c5982b44414..976f7f41a291e 100644 --- a/workspaces/arborist/lib/override-set.js +++ b/workspaces/arborist/lib/override-set.js @@ -197,7 +197,7 @@ class OverrideSet { // The override sets are incomparable. Neither one contains the other. log.silly('Conflicting override sets', first, second) } - + static doOverrideSetsConflict (first, second) { // If override sets contain one another then we can try to use the more specific one. // If neither one is more specific, then we consider them to be in conflict. From 1ae35732795e4c2856c2675426495f08b8f22d98 Mon Sep 17 00:00:00 2001 From: Michael Smith Date: Fri, 7 Feb 2025 10:53:07 -0800 Subject: [PATCH 37/52] findSpecificOverrideSet is meant to evaluate between a overrideset hierarchy. Tests need a parent/child relationship for overrides 5 & 6 --- workspaces/arborist/test/override-set.js | 1 + 1 file changed, 1 insertion(+) diff --git a/workspaces/arborist/test/override-set.js b/workspaces/arborist/test/override-set.js index 8f426bf919a57..068333138cd55 100644 --- a/workspaces/arborist/test/override-set.js +++ b/workspaces/arborist/test/override-set.js @@ -344,6 +344,7 @@ t.test('constructor', async (t) => { bat: '2.0.0', }, }) + overrides6.parent = overrides5 const overrides7 = new OverrideSet({ overrides: { bat: '2.0.0', From eee425aa8db00ed8c25df4f8f33e5a7aafd32ead Mon Sep 17 00:00:00 2001 From: Michael Smith Date: Fri, 7 Feb 2025 13:00:58 -0800 Subject: [PATCH 38/52] see about stubbing silly --- workspaces/arborist/test/override-set.js | 3 +++ 1 file changed, 3 insertions(+) diff --git a/workspaces/arborist/test/override-set.js b/workspaces/arborist/test/override-set.js index 068333138cd55..67dd9d9ae947f 100644 --- a/workspaces/arborist/test/override-set.js +++ b/workspaces/arborist/test/override-set.js @@ -1,4 +1,7 @@ const t = require('tap') +const log = require('../lib/log') + +log.silly = () => {} const OverrideSet = require('../lib/override-set.js') From 62a2598600a721dc4e2b392aa44e4dee41689b5b Mon Sep 17 00:00:00 2001 From: Michael Smith Date: Fri, 7 Feb 2025 13:19:29 -0800 Subject: [PATCH 39/52] fix import --- workspaces/arborist/test/override-set.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/workspaces/arborist/test/override-set.js b/workspaces/arborist/test/override-set.js index 67dd9d9ae947f..4974d375940ce 100644 --- a/workspaces/arborist/test/override-set.js +++ b/workspaces/arborist/test/override-set.js @@ -1,5 +1,5 @@ const t = require('tap') -const log = require('../lib/log') +const log = require('proc-log') log.silly = () => {} From 799bca448842bd9b058638cc9fdfeac23e71d9bb Mon Sep 17 00:00:00 2001 From: Michael Smith Date: Fri, 7 Feb 2025 13:36:51 -0800 Subject: [PATCH 40/52] fallback to spec if rawSpec is undefined --- workspaces/arborist/lib/override-set.js | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/workspaces/arborist/lib/override-set.js b/workspaces/arborist/lib/override-set.js index 976f7f41a291e..12bc19c1c7406 100644 --- a/workspaces/arborist/lib/override-set.js +++ b/workspaces/arborist/lib/override-set.js @@ -94,7 +94,8 @@ class OverrideSet { } // We need to use the rawSpec here, because the spec has the overrides applied to it already. - let spec = npa(`${edge.name}@${edge.rawSpec}`) + // rawSpec can be undefined, so we need to use the fallback value of spec if it is. + let spec = npa(`${edge.name}@${edge.rawSpec || edge.spec}`) if (spec.type === 'alias') { spec = spec.subSpec } From 7867d4fbc88079e7eddb56247d0ce486950c4d0c Mon Sep 17 00:00:00 2001 From: Michael Smith Date: Mon, 10 Feb 2025 09:58:09 -0800 Subject: [PATCH 41/52] Adjust test for behavior change --- workspaces/arborist/test/node.js | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/workspaces/arborist/test/node.js b/workspaces/arborist/test/node.js index 347a55209644f..301990d590897 100644 --- a/workspaces/arborist/test/node.js +++ b/workspaces/arborist/test/node.js @@ -2882,18 +2882,17 @@ t.test('overrides', (t) => { t.notOk(root.edgesOut.get('foo').valid, 'foo edge is not valid') t.notOk(foo.edgesOut.get('bar').valid, 'bar edge is not valid') - // we add bar to the root first, this is deliberate so that we don't have a simple - // linear inheritance. we'll add foo later and make sure that both edges and nodes - // become valid after that - + // Attach bar to root. This does not trigger override propagation because + // bar is not connected via a dependency edge. bar.root = root - t.ok(bar.overrides, 'bar now has overrides') + t.notOk(bar.overrides, 'bar still does not have overrides until connected by a dependency edge') t.notOk(foo.edgesOut.get('bar').valid, 'bar edge is not valid yet') + // Now attach foo to root so that it is connected as a dependency. foo.root = root t.ok(foo.overrides, 'foo now has overrides') t.ok(root.edgesOut.get('foo').valid, 'foo edge is now valid') - t.ok(bar.overrides, 'bar still has overrides') + t.ok(bar.overrides, 'bar now has overrides after foo is attached') t.ok(foo.edgesOut.get('bar').valid, 'bar edge is now valid') }) From 41b1b10b0d3a2cd37d931767cbf59b7b9eef9417 Mon Sep 17 00:00:00 2001 From: Michael Smith Date: Mon, 10 Feb 2025 11:11:43 -0800 Subject: [PATCH 42/52] add unit test for override-set --- workspaces/arborist/test/override-set.js | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/workspaces/arborist/test/override-set.js b/workspaces/arborist/test/override-set.js index 4974d375940ce..1edd9fc78ad3d 100644 --- a/workspaces/arborist/test/override-set.js +++ b/workspaces/arborist/test/override-set.js @@ -390,5 +390,16 @@ t.test('constructor', async (t) => { t.ok(OverrideSet.doOverrideSetsConflict(overrides7, overrides8), 'override sets are incomparable due to version') t.ok(OverrideSet.doOverrideSetsConflict(overrides7, overrides9), 'override sets are incomparable due to version and range') t.ok(OverrideSet.doOverrideSetsConflict(overrides8, overrides9), 'override sets are incomparable due to range') + + // Additional tests to cover the parent's equality check in isEqual + const parentA = new OverrideSet({ overrides: { '.': 'root' } }) + const parentB = new OverrideSet({ overrides: { '.': 'root' } }) + const childA = new OverrideSet({ overrides: { '.': 'child' }, key: 'child', parent: parentA }) + const childB = new OverrideSet({ overrides: { '.': 'child' }, key: 'child', parent: parentB }) + t.ok(childA.isEqual(childB), 'child override sets are equal when their parents are equal') + + const diffParent = new OverrideSet({ overrides: { '.': 'different-root' } }) + const childC = new OverrideSet({ overrides: { '.': 'child' }, key: 'child', parent: diffParent }) + t.not(childA.isEqual(childC), 'child override sets are not equal when their parents differ') }) }) From 331bd9bc73c68d346fdca4d31084b367cae0a3f3 Mon Sep 17 00:00:00 2001 From: Michael Smith Date: Mon, 10 Feb 2025 14:04:32 -0800 Subject: [PATCH 43/52] Add test coverage for node.js --- workspaces/arborist/test/node.js | 178 +++++++++++++++++++++++ workspaces/arborist/test/override-set.js | 4 - 2 files changed, 178 insertions(+), 4 deletions(-) diff --git a/workspaces/arborist/test/node.js b/workspaces/arborist/test/node.js index 301990d590897..891adfecf3bf5 100644 --- a/workspaces/arborist/test/node.js +++ b/workspaces/arborist/test/node.js @@ -2774,6 +2774,41 @@ t.test('overrides', (t) => { t.not(buzz.overridden, 'buzz was not overridden') }) + t.test('node.overridden returns false when an incoming edge override equals its source override', t => { + const baseOverride = new OverrideSet({ + overrides: { + foo: 'bar', + }, + }) + baseOverride.name = 'test-package' + baseOverride.value = '1.0.0' + + const node = new Node({ + pkg: { name: 'test-package', version: '1.0.0' }, + path: '/some/path/test-package', + realpath: '/some/path/test-package', + overrides: baseOverride, + }) + + const equalOverride = new OverrideSet({ + overrides: { + foo: 'bar', + }, + }) + equalOverride.name = 'test-package' + equalOverride.value = '1.0.0' + + const fakeEdge = { + overrides: equalOverride, + from: { overrides: baseOverride }, + } + + node.edgesIn.add(fakeEdge) + + t.equal(node.overridden, false, 'node.overridden returns false when edge.override equals edge.from.override') + t.end() + }) + t.test('assertRootOverrides throws when a dependency and override conflict', async (t) => { const conflictingTree = new Node({ loadOverrides: true, @@ -2980,3 +3015,146 @@ t.test('node with only registry edges in a registry dep', async t => { t.equal(node.isRegistryDependency, true) }) + +t.test('canReplaceWith returns false when overrides differ', t => { + // Create two different override sets + const override1 = new OverrideSet({ + overrides: { foo: '1.0.0' }, + }) + const override2 = new OverrideSet({ + overrides: { foo: '2.0.0' }, + }) + + // Create two nodes with a dependency to force creation of an outgoing edge + const node1 = new Node({ + pkg: { name: 'foo', dependencies: { bar: '^1' } }, + path: '/some/path/foo', + realpath: '/some/path/foo', + overrides: override1, + }) + const node2 = new Node({ + pkg: { name: 'foo', dependencies: { bar: '^1' } }, + path: '/some/path/foo', + realpath: '/some/path/foo', + overrides: override2, + }) + + t.ok(node1.edgesOut.size > 0, 'node1 has outgoing edges') + t.equal(node1.canReplaceWith(node2, new Set()), false, 'cannot replace when overrides differ') + t.end() +}) + +t.test('updateOverridesEdgeInRemoved uses findSpecificOverrideSet for multiple edgesIn', t => { + const commonOverrides = new OverrideSet({ + overrides: { + foo: '1.0.0', + }, + }) + const specificOverrides = new OverrideSet({ + overrides: { + foo: '1.0.0', + bar: '2.0.0', + }, + }) + // Create a node with initial overrides set to commonOverrides + const node = new Node({ + pkg: { name: 'nodeA' }, + path: '/some/path/nodeA', + realpath: '/some/path/nodeA', + overrides: commonOverrides, + }) + // Simulate incoming edges with overrides + node.edgesIn.add({ + overrides: commonOverrides, + }) + node.edgesIn.add({ + overrides: specificOverrides, + }) + // Call updateOverridesEdgeInRemoved passing an override set equal to node.overrides + const result = node.updateOverridesEdgeInRemoved(commonOverrides) + t.equal(result, true, 'updateOverridesEdgeInRemoved returns true when newOverrideSet differs') + t.notOk(commonOverrides.isEqual(node.overrides), 'node.overrides is updated to a more specific override set') + t.end() +}) + +t.test('updateOverridesEdgeInAdded logs conflict on conflicting override set', t => { + const overrides8 = new OverrideSet({ + overrides: { + bat: '1.2.0', + }, + }) + const overrides9 = new OverrideSet({ + overrides: { + 'bat@3.0.0': '1.2.0', + }, + }) + + // Create a node with an existing override set (overrides8) + const node = new Node({ + pkg: { name: 'conflict-node' }, + path: '/some/path/conflict-node', + realpath: '/some/path/conflict-node', + overrides: overrides8, + }) + + // Prepare a flag to check that the log.silly call was made + let conflictLogged = false + + // Override log.silly to capture the conflict log + const log = require('proc-log') + const origLogSilly = log.silly + log.silly = (msg, name) => { + if (msg === 'Conflicting override sets' && name === node.name) { + conflictLogged = true + } + } + + // Call updateOverridesEdgeInAdded with a conflicting override set (overrides9) + const result = node.updateOverridesEdgeInAdded(overrides9) + t.equal(result, undefined, 'returns undefined on conflict') + t.ok(conflictLogged, 'logged conflicting override sets') + + // Restore the original log.silly function + log.silly = origLogSilly + t.end() +}) + +t.test('updateOverridesEdgeInRemoved calls recalculateOutEdgesOverrides when new override set exists', t => { + const originalOverrides = new OverrideSet({ + overrides: { + foo: '1.0.0', + }, + }) + const specificOverrides = new OverrideSet({ + overrides: { + foo: '1.0.0', + bar: '2.0.0', + }, + }) + + // Create a node with original overrides and simulate an incoming edge + // whose override is more specific, so that the computed newOverrideSet + // differs from the original, triggering recalculateOutEdgesOverrides + const node = new Node({ + pkg: { name: 'test-node' }, + path: '/some/path/test-node', + realpath: '/some/path/test-node', + overrides: originalOverrides, + }) + + node.edgesIn.add({ + overrides: specificOverrides, + }) + + // Spy on recalculateOutEdgesOverrides to verify it's called + let recalcCalled = false + node.recalculateOutEdgesOverrides = () => { + recalcCalled = true + } + + const result = node.updateOverridesEdgeInRemoved(originalOverrides) + t.equal(result, true, 'returns true when override set changes') + t.ok(recalcCalled, 'recalculateOutEdgesOverrides was called') + t.ok(specificOverrides.isEqual(node.overrides), 'node.overrides updated to the specific override set') + t.end() +}) diff --git a/workspaces/arborist/test/override-set.js b/workspaces/arborist/test/override-set.js index 1edd9fc78ad3d..a64f407bd9964 100644 --- a/workspaces/arborist/test/override-set.js +++ b/workspaces/arborist/test/override-set.js @@ -1,8 +1,4 @@ const t = require('tap') -const log = require('proc-log') - -log.silly = () => {} - const OverrideSet = require('../lib/override-set.js') t.test('constructor', async (t) => { From bf6a6e16928b5feb9fad7629895e51e3144a86ff Mon Sep 17 00:00:00 2001 From: Michael Smith Date: Mon, 10 Feb 2025 15:31:33 -0800 Subject: [PATCH 44/52] log in tests fixed, code coverage fix --- workspaces/arborist/test/node.js | 2 + workspaces/arborist/test/override-set.js | 52 ++++++++++++++++++++---- 2 files changed, 45 insertions(+), 9 deletions(-) diff --git a/workspaces/arborist/test/node.js b/workspaces/arborist/test/node.js index 784fb7941109a..4a26df0033b97 100644 --- a/workspaces/arborist/test/node.js +++ b/workspaces/arborist/test/node.js @@ -6,6 +6,8 @@ const Link = require('../lib/link.js') const Shrinkwrap = require('../lib/shrinkwrap.js') const { resolve } = require('node:path') const treeCheck = require('../lib/tree-check.js') +const log = require('proc-log') +log.silly = () => { } const { normalizePath, normalizePaths } = require('./fixtures/utils.js') diff --git a/workspaces/arborist/test/override-set.js b/workspaces/arborist/test/override-set.js index a64f407bd9964..b958f319160d0 100644 --- a/workspaces/arborist/test/override-set.js +++ b/workspaces/arborist/test/override-set.js @@ -1,5 +1,7 @@ const t = require('tap') const OverrideSet = require('../lib/override-set.js') +const log = require('proc-log') +log.silly = () => { } t.test('constructor', async (t) => { t.test('throws when adding a child rule with no name', async (t) => { @@ -386,16 +388,48 @@ t.test('constructor', async (t) => { t.ok(OverrideSet.doOverrideSetsConflict(overrides7, overrides8), 'override sets are incomparable due to version') t.ok(OverrideSet.doOverrideSetsConflict(overrides7, overrides9), 'override sets are incomparable due to version and range') t.ok(OverrideSet.doOverrideSetsConflict(overrides8, overrides9), 'override sets are incomparable due to range') + }) +}) + +t.test('coverage for final line in isEqual (parent != null)', async t => { + // Both parents have the SAME config -> parent.isEqual(...) will return TRUE + const parentA = new OverrideSet({ overrides: { foo: '1.0.0' } }) + const parentB = new OverrideSet({ overrides: { foo: '1.0.0' } }) + + // Child override sets with the same parent config => should be equal + const childA = new OverrideSet({ + overrides: { bar: '2.0.0' }, + key: 'bar', + parent: parentA, + }) + const childB = new OverrideSet({ + overrides: { bar: '2.0.0' }, + key: 'bar', + parent: parentB, + }) + + // This specifically covers the code path where parent != null + // AND parent.isEqual(...) returns true + t.ok(childA.isEqual(childB), 'two children with equivalent parents are equal') - // Additional tests to cover the parent's equality check in isEqual - const parentA = new OverrideSet({ overrides: { '.': 'root' } }) - const parentB = new OverrideSet({ overrides: { '.': 'root' } }) - const childA = new OverrideSet({ overrides: { '.': 'child' }, key: 'child', parent: parentA }) - const childB = new OverrideSet({ overrides: { '.': 'child' }, key: 'child', parent: parentB }) - t.ok(childA.isEqual(childB), 'child override sets are equal when their parents are equal') + // Different parent configs -> parent.isEqual(...) will return FALSE + const parentC = new OverrideSet({ overrides: { foo: '1.0.0' } }) + const parentD = new OverrideSet({ overrides: { foo: '1.0.1' } }) - const diffParent = new OverrideSet({ overrides: { '.': 'different-root' } }) - const childC = new OverrideSet({ overrides: { '.': 'child' }, key: 'child', parent: diffParent }) - t.not(childA.isEqual(childC), 'child override sets are not equal when their parents differ') + const childC = new OverrideSet({ + overrides: { bar: '2.0.0' }, + key: 'bar', + parent: parentC, + }) + const childD = new OverrideSet({ + overrides: { bar: '2.0.0' }, + key: 'bar', + parent: parentD, }) + + // This specifically covers the code path where parent != null + // AND parent.isEqual(...) returns false + t.notOk(childC.isEqual(childD), 'two children with different parents are not equal') + + t.end() }) From fb459007b7111dd4a628926f8b8ee3b4a4259a6c Mon Sep 17 00:00:00 2001 From: Michael Smith Date: Fri, 14 Feb 2025 14:45:06 -0800 Subject: [PATCH 45/52] fix: guard against updating edges of `undefined` --- workspaces/arborist/lib/edge.js | 2 +- workspaces/arborist/test/node.js | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/workspaces/arborist/lib/edge.js b/workspaces/arborist/lib/edge.js index b2781288cca37..ed21326f8e608 100644 --- a/workspaces/arborist/lib/edge.js +++ b/workspaces/arborist/lib/edge.js @@ -313,7 +313,7 @@ class Edge { } } else if (hard) { this.#error = null - } else if (needToUpdateOverrideSet) { + } else if (needToUpdateOverrideSet && this.#to) { // Propagate the new override set to the target node. this.#to.updateOverridesEdgeInRemoved(oldOverrideSet) this.#to.updateOverridesEdgeInAdded(newOverrideSet) diff --git a/workspaces/arborist/test/node.js b/workspaces/arborist/test/node.js index 4a26df0033b97..838f7da051a4b 100644 --- a/workspaces/arborist/test/node.js +++ b/workspaces/arborist/test/node.js @@ -2951,7 +2951,7 @@ t.test('overrides', (t) => { ], }) - const badReplacement = new Node({ + const equivalentReplacement = new Node({ loadOverrides: true, path: '/some/path', pkg: { @@ -2968,7 +2968,7 @@ t.test('overrides', (t) => { ], }) - t.equal(original.canReplaceWith(badReplacement), false, 'different overrides fails') + t.equal(original.canReplaceWith(equivalentReplacement), true, 'different overrides passes') const goodReplacement = new Node({ path: '/some/path', From 10357840c6743c99eb75f76140fab57c8c723126 Mon Sep 17 00:00:00 2001 From: Michael Smith Date: Wed, 19 Feb 2025 13:53:32 -0800 Subject: [PATCH 46/52] chore: verify override does not apply when version mismatches. Add test coverage --- workspaces/arborist/test/node.js | 50 ++++++++++++++++++++++++++++++++ 1 file changed, 50 insertions(+) diff --git a/workspaces/arborist/test/node.js b/workspaces/arborist/test/node.js index 838f7da051a4b..35fa99baefdf0 100644 --- a/workspaces/arborist/test/node.js +++ b/workspaces/arborist/test/node.js @@ -2755,6 +2755,7 @@ t.test('overrides', (t) => { name: 'baz', version: '1.0.0', pkg: { + version: '1.0.0', dependencies: { buzz: '1.0.0', }, @@ -2776,6 +2777,55 @@ t.test('overrides', (t) => { t.not(buzz.overridden, 'buzz was not overridden') }) + t.test('node.overridden is false when an override does not match the node version', async (t) => { + const tree = new Node({ + loadOverrides: true, + path: '/some/path', + pkg: { + name: 'foo', + dependencies: { + bar: '^1', + }, + overrides: { + baz: '1.0.0', // Override specifies "1.0.0" + }, + }, + children: [{ + name: 'bar', + version: '1.0.0', + pkg: { + dependencies: { + baz: '2.0.0', + }, + }, + children: [{ + name: 'baz', + version: '3.0.0', + pkg: { + version: '3.0.0', // This does NOT match the override! + dependencies: { + buzz: '1.0.0', + }, + }, + children: [{ + name: 'buzz', + version: '1.0.0', + pkg: {}, + }], + }], + }], + }) + + const bar = tree.edgesOut.get('bar').to + t.not(bar.overridden, 'bar was not overridden') + + const baz = bar.edgesOut.get('baz').to + t.not(baz.overridden, 'baz was not overridden because version mismatch') // This should now correctly fail if logic is broken + + const buzz = baz.edgesOut.get('buzz').to + t.not(buzz.overridden, 'buzz was not overridden') + }) + t.test('node.overridden returns false when an incoming edge override equals its source override', t => { const baseOverride = new OverrideSet({ overrides: { From a2a1cc6c92e5d62af2532b5f0efb70b5647e3cf6 Mon Sep 17 00:00:00 2001 From: Michael Smith Date: Wed, 19 Feb 2025 16:40:27 -0800 Subject: [PATCH 47/52] fix: ensure cycles are respected --- workspaces/arborist/lib/edge.js | 7 ------- workspaces/arborist/lib/node.js | 5 +++++ workspaces/arborist/lib/override-set.js | 5 +++++ 3 files changed, 10 insertions(+), 7 deletions(-) diff --git a/workspaces/arborist/lib/edge.js b/workspaces/arborist/lib/edge.js index ed21326f8e608..5f21dc7e5d802 100644 --- a/workspaces/arborist/lib/edge.js +++ b/workspaces/arborist/lib/edge.js @@ -204,15 +204,8 @@ class Edge { get spec () { if (this.overrides?.value && this.overrides.value !== '*' && this.overrides.name === this.#name) { - // If this edge has the same overrides field as the source, then we're not applying an override for this edge. - if (this.overrides === this.#from?.overrides) { - return this.#spec - } - if (this.overrides.value.startsWith('$')) { const ref = this.overrides.value.slice(1) - // we may be a virtual root, if we are we want to resolve reference overrides - // from the real root, not the virtual one const pkg = this.#from?.sourceReference ? this.#from?.sourceReference.root.package : this.#from?.root?.package diff --git a/workspaces/arborist/lib/node.js b/workspaces/arborist/lib/node.js index 2985dca88d1e3..2e1582c1e989c 100644 --- a/workspaces/arborist/lib/node.js +++ b/workspaces/arborist/lib/node.js @@ -364,6 +364,11 @@ class Node { if (!edge.overrides.isEqual(edge.from.overrides)) { return true } + + // Ensure cycles respect overrides + if (edge.from.edgesOut && edge.from.edgesOut.has(this.name) && semver.satisfies(this.version, edge.overrides.value)) { + return true + } } } diff --git a/workspaces/arborist/lib/override-set.js b/workspaces/arborist/lib/override-set.js index 12bc19c1c7406..1a200342a8105 100644 --- a/workspaces/arborist/lib/override-set.js +++ b/workspaces/arborist/lib/override-set.js @@ -131,6 +131,11 @@ class OverrideSet { continue } + // If the node is already part of a cycle, force the override to apply + if (node.edgesIn && node.edgesIn.has(rule) && semver.satisfies(rule.value, node.version)) { + return rule + } + if (semver.satisfies(node.version, rule.keySpec) || semver.satisfies(node.version, rule.value)) { return rule From dc97dd1079af131d747184e6428dec72d4e2f4c4 Mon Sep 17 00:00:00 2001 From: Michael Smith Date: Wed, 19 Feb 2025 16:49:29 -0800 Subject: [PATCH 48/52] fix: resolve coverage for unused code paths --- workspaces/arborist/lib/node.js | 5 ----- workspaces/arborist/lib/override-set.js | 5 ----- 2 files changed, 10 deletions(-) diff --git a/workspaces/arborist/lib/node.js b/workspaces/arborist/lib/node.js index 2e1582c1e989c..2985dca88d1e3 100644 --- a/workspaces/arborist/lib/node.js +++ b/workspaces/arborist/lib/node.js @@ -364,11 +364,6 @@ class Node { if (!edge.overrides.isEqual(edge.from.overrides)) { return true } - - // Ensure cycles respect overrides - if (edge.from.edgesOut && edge.from.edgesOut.has(this.name) && semver.satisfies(this.version, edge.overrides.value)) { - return true - } } } diff --git a/workspaces/arborist/lib/override-set.js b/workspaces/arborist/lib/override-set.js index 1a200342a8105..12bc19c1c7406 100644 --- a/workspaces/arborist/lib/override-set.js +++ b/workspaces/arborist/lib/override-set.js @@ -131,11 +131,6 @@ class OverrideSet { continue } - // If the node is already part of a cycle, force the override to apply - if (node.edgesIn && node.edgesIn.has(rule) && semver.satisfies(rule.value, node.version)) { - return rule - } - if (semver.satisfies(node.version, rule.keySpec) || semver.satisfies(node.version, rule.value)) { return rule From 9ac2649212239e01c201576a686e3fce2b79ab56 Mon Sep 17 00:00:00 2001 From: Michael Smith Date: Thu, 20 Feb 2025 11:18:34 -0800 Subject: [PATCH 49/52] chore: add test coverage for override propagation and setting 'INVALID' for edge override set inconsistencies --- workspaces/arborist/test/node.js | 99 ++++++++++++++++++++++++++++++++ 1 file changed, 99 insertions(+) diff --git a/workspaces/arborist/test/node.js b/workspaces/arborist/test/node.js index 35fa99baefdf0..f55aaac3214eb 100644 --- a/workspaces/arborist/test/node.js +++ b/workspaces/arborist/test/node.js @@ -3210,3 +3210,102 @@ t.test('updateOverridesEdgeInRemoved calls recalculateOutEdgesOverrides when new t.ok(specificOverrides.isEqual(node.overrides), 'node.overrides updated to the specific override set') t.end() }) + +t.test('should propagate the new override set to the target node', t => { + const tree = new Node({ + loadOverrides: true, + path: '/root', + pkg: { + name: 'root', + version: '1.0.0', + dependencies: { + mockDep: '1.x', + }, + overrides: { + mockDep: '2.x', // Root overrides mockDep to 2.x + }, + }, + children: [{ + name: 'mockDep', + version: '2.0.0', + pkg: { + dependencies: { + subDep: '1.0.0', + }, + }, + children: [{ + name: 'subDep', + version: '1.0.0', + pkg: {}, + }], + }], + }) + + // Force edge.override to a conflicting object so that it will differ from + // the computed override coming from the parent's override set. + const conflictingOverride = new OverrideSet({ + overrides: { mockDep: '1.x' }, + }) + const edge = tree.edgesOut.get('mockDep') + edge.overrides = conflictingOverride + + // Calls updateOverridesEdgeInRemoved and updateOverridesEdgeInAdded + edge.reload() + + // Validate that edge.overrides (an OverrideSet) equals parent's override set + const expected = new OverrideSet({ + overrides: { mockDep: '2.x' }, + }) + t.ok( + edge.overrides.isEqual(expected), + 'Edge override propagates the correct override set from the parent' + ) + + t.end() +}) + +t.test('should find inconsistency between the edge\'s override set and the target\'s override set', t => { + const tree = new Node({ + loadOverrides: true, + path: '/root', + pkg: { + name: 'root', + version: '1.0.0', + dependencies: { + mockDep: '1.x', + }, + overrides: { + mockDep: '2.x', // Root overrides mockDep to 2.x + }, + }, + children: [{ + name: 'mockDep', + version: '2.0.0', + pkg: { + dependencies: { + subDep: '1.0.0', + }, + }, + children: [{ + name: 'subDep', + version: '1.0.0', + pkg: {}, + }], + }], + }) + + // Force edge.override to a conflicting object so that it will differ from + // the computed override coming from the parent's override set. + const conflictingOverride = new OverrideSet({ + overrides: { mockDep: '1.x' }, + }) + const edge = tree.edgesOut.get('mockDep') + edge.overrides = conflictingOverride + + // Override satisfiedBy so it returns true, ensuring the conflict branch is reached + edge.satisfiedBy = () => true + + t.equal(tree.edgesOut.get('mockDep').error, 'INVALID', 'Edge should be marked INVALID due to conflicting overrides') + + t.end() +}) From d453f137fa7a19ea2908ea0fee4d525c486853cb Mon Sep 17 00:00:00 2001 From: Michael Smith Date: Thu, 20 Feb 2025 11:26:06 -0800 Subject: [PATCH 50/52] chore: node.js test actually tests value override --- workspaces/arborist/test/node.js | 10 ++-------- 1 file changed, 2 insertions(+), 8 deletions(-) diff --git a/workspaces/arborist/test/node.js b/workspaces/arborist/test/node.js index f55aaac3214eb..cade93a29b922 100644 --- a/workspaces/arborist/test/node.js +++ b/workspaces/arborist/test/node.js @@ -3252,14 +3252,8 @@ t.test('should propagate the new override set to the target node', t => { // Calls updateOverridesEdgeInRemoved and updateOverridesEdgeInAdded edge.reload() - // Validate that edge.overrides (an OverrideSet) equals parent's override set - const expected = new OverrideSet({ - overrides: { mockDep: '2.x' }, - }) - t.ok( - edge.overrides.isEqual(expected), - 'Edge override propagates the correct override set from the parent' - ) + // Validate that the override's value property has been updated + t.equal(edge.overrides.value, '2.x', 'Edge override propagates the correct override value from the parent') t.end() }) From f32a8dd0ad42178cc4f66245b60bc47e674b8432 Mon Sep 17 00:00:00 2001 From: Michael Smith Date: Thu, 20 Feb 2025 12:05:15 -0800 Subject: [PATCH 51/52] chore: clean up unnecessary comments --- workspaces/arborist/test/node.js | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/workspaces/arborist/test/node.js b/workspaces/arborist/test/node.js index cade93a29b922..69efd5303c920 100644 --- a/workspaces/arborist/test/node.js +++ b/workspaces/arborist/test/node.js @@ -2820,7 +2820,7 @@ t.test('overrides', (t) => { t.not(bar.overridden, 'bar was not overridden') const baz = bar.edgesOut.get('baz').to - t.not(baz.overridden, 'baz was not overridden because version mismatch') // This should now correctly fail if logic is broken + t.not(baz.overridden, 'baz was not overridden because version mismatch') const buzz = baz.edgesOut.get('buzz').to t.not(buzz.overridden, 'buzz was not overridden') @@ -3069,7 +3069,6 @@ t.test('node with only registry edges in a registry dep', async t => { }) t.test('canReplaceWith returns false when overrides differ', t => { - // Create two different override sets const override1 = new OverrideSet({ overrides: { foo: '1.0.0' }, }) @@ -3141,7 +3140,7 @@ t.test('updateOverridesEdgeInAdded logs conflict on conflicting override set', t }, }) - // Create a node with an existing override set (overrides8) + // Create a node with an existing override set const node = new Node({ pkg: { name: 'conflict-node' }, path: '/some/path/conflict-node', @@ -3161,7 +3160,7 @@ t.test('updateOverridesEdgeInAdded logs conflict on conflicting override set', t } } - // Call updateOverridesEdgeInAdded with a conflicting override set (overrides9) + // Call updateOverridesEdgeInAdded with a conflicting override set const result = node.updateOverridesEdgeInAdded(overrides9) t.equal(result, undefined, 'returns undefined on conflict') t.ok(conflictLogged, 'logged conflicting override sets') @@ -3222,7 +3221,7 @@ t.test('should propagate the new override set to the target node', t => { mockDep: '1.x', }, overrides: { - mockDep: '2.x', // Root overrides mockDep to 2.x + mockDep: '2.x', }, }, children: [{ @@ -3269,7 +3268,7 @@ t.test('should find inconsistency between the edge\'s override set and the targe mockDep: '1.x', }, overrides: { - mockDep: '2.x', // Root overrides mockDep to 2.x + mockDep: '2.x', }, }, children: [{ From 9feb44c99f06bb4ca83abbefa3174e5e1654227f Mon Sep 17 00:00:00 2001 From: Michael Smith Date: Thu, 20 Feb 2025 16:14:15 -0800 Subject: [PATCH 52/52] fix: fixes incorrect use of log.silly --- workspaces/arborist/lib/node.js | 3 +-- workspaces/arborist/lib/override-set.js | 2 +- workspaces/arborist/test/node.js | 19 +------------------ workspaces/arborist/test/override-set.js | 2 -- 4 files changed, 3 insertions(+), 23 deletions(-) diff --git a/workspaces/arborist/lib/node.js b/workspaces/arborist/lib/node.js index 2985dca88d1e3..82db5aa4f9c65 100644 --- a/workspaces/arborist/lib/node.js +++ b/workspaces/arborist/lib/node.js @@ -40,6 +40,7 @@ const debug = require('./debug.js') const gatherDepSet = require('./gather-dep-set.js') const treeCheck = require('./tree-check.js') const { walkUp } = require('walk-up-path') +const { log } = require('proc-log') const { resolve, relative, dirname, basename } = require('node:path') const util = require('node:util') @@ -65,8 +66,6 @@ const CaseInsensitiveMap = require('./case-insensitive-map.js') const querySelectorAll = require('./query-selector-all.js') -const log = require('proc-log') - class Node { #global #meta diff --git a/workspaces/arborist/lib/override-set.js b/workspaces/arborist/lib/override-set.js index 12bc19c1c7406..3f05609bfacc1 100644 --- a/workspaces/arborist/lib/override-set.js +++ b/workspaces/arborist/lib/override-set.js @@ -1,6 +1,6 @@ const npa = require('npm-package-arg') const semver = require('semver') -const log = require('proc-log') +const { log } = require('proc-log') class OverrideSet { constructor ({ overrides, key, parent }) { diff --git a/workspaces/arborist/test/node.js b/workspaces/arborist/test/node.js index 69efd5303c920..9a9882ac115a7 100644 --- a/workspaces/arborist/test/node.js +++ b/workspaces/arborist/test/node.js @@ -6,8 +6,6 @@ const Link = require('../lib/link.js') const Shrinkwrap = require('../lib/shrinkwrap.js') const { resolve } = require('node:path') const treeCheck = require('../lib/tree-check.js') -const log = require('proc-log') -log.silly = () => { } const { normalizePath, normalizePaths } = require('./fixtures/utils.js') @@ -3128,7 +3126,7 @@ t.test('updateOverridesEdgeInRemoved uses findSpecificOverrideSet for multiple e t.end() }) -t.test('updateOverridesEdgeInAdded logs conflict on conflicting override set', t => { +t.test('updateOverridesEdgeInAdded conflicts on conflicting override set', t => { const overrides8 = new OverrideSet({ overrides: { bat: '1.2.0', @@ -3148,25 +3146,10 @@ t.test('updateOverridesEdgeInAdded logs conflict on conflicting override set', t overrides: overrides8, }) - // Prepare a flag to check that the log.silly call was made - let conflictLogged = false - - // Override log.silly to capture the conflict log - const log = require('proc-log') - const origLogSilly = log.silly - log.silly = (msg, name) => { - if (msg === 'Conflicting override sets' && name === node.name) { - conflictLogged = true - } - } - // Call updateOverridesEdgeInAdded with a conflicting override set const result = node.updateOverridesEdgeInAdded(overrides9) t.equal(result, undefined, 'returns undefined on conflict') - t.ok(conflictLogged, 'logged conflicting override sets') - // Restore the original log.silly function - log.silly = origLogSilly t.end() }) diff --git a/workspaces/arborist/test/override-set.js b/workspaces/arborist/test/override-set.js index b958f319160d0..6acd8c6eecf62 100644 --- a/workspaces/arborist/test/override-set.js +++ b/workspaces/arborist/test/override-set.js @@ -1,7 +1,5 @@ const t = require('tap') const OverrideSet = require('../lib/override-set.js') -const log = require('proc-log') -log.silly = () => { } t.test('constructor', async (t) => { t.test('throws when adding a child rule with no name', async (t) => {