fix: npm dedupe property path of null error
#308
Conversation
When a dependency graph cycles back on itself incompatibly like this:
```
a@1 -> b@1
b@1 -> a@2
a@2 -> b@2
b@2 -> a@1
```
We would find ourselves unable to handle the conflict via nesting. For
example:
```
root
+-- a@1 -> b@1
+-- b@1 -> a@2
+-- a@2 -> b@2
+-- b@2 -> a@1
+-- a@1 -> b@1
+-- b@1 -> a@2
+-- a@2 -> b@2
+-- b@2 -> a@1 (cycling forever)
```
In order to address this, we create a link when such a cycle is
detected.
```
root
+-- a@1 -> b@1
+-- b@1 -> a@2
+-- a@2 -> b@2
+-- b@2 -> a@1
+-- a@1 -> b@1
+-- b@1 -> link to root/node_modules/b@1
```
Prior to the recent refactor to move much of the dependency placement
logic out of Arborist.buildIdealTree and into the PlaceDep class, this
link was created right at the moment when a new dependency was created
in a temp tree.
However, if we feed that Link object into the PlaceDep flow, it will
(correctly) see that the Link does not match the Node further up the
tree, and attempt to replace it.
Compounding the problem (and why it appeared in `npm dedupe` and not
`npm install`) is the fact that explicitly named updates are _always_
treated as a "problem edge", so that they can be re-evaluated.
So, rather than creating a Node to be tested against the tree, it was
creating a Link object, and then attempting to replace the Link's target
with the Link itself, which caused some havoc.
This patch moves the loop detection and remediating Link creation into
the PlaceDep class, which is the more proper place for it, as that class
owns the "put deps into the tree" logic, and this is clearly a "put deps
into the tree" type of situation.
Via: @ParadoxInfinite
Close: npm/cli#3632
Close: #308
Fix: npm/cli#3565
|
This addresses the surface symptom, and it's a perfectly fine defensive change to make, but doesn't actually make |
I understand, as someone who wasn't very familiar with the codebase, this seemed like a logical change as it resolved the issue (or so I thought). I will go through the PR you've mentioned to see if I can understand the issue and the solution better. Hopefully it makes sense to me. Thank you for taking the time and checking out this PR though! |
|
@ParadoxInfinite You did most of the work of finding it, thanks ;) Happy to have you keep poking at Arborist, let us know if you have questions. |
Related to npm/cli#3632
Copy pasting from the above closed PR as it's the same change.
Would appreciate feedback, if any!
Also, I would appreciate if someone can help me out with writing a regression test for this? I looked into the
test/place-dep.jsfile, but wasn't sure how I could write one for this condition specifically. The way I tested this was using the https://github.com/npm/cli/ repo with the test case provided in the issue below, so any help here is awesome!References
Fixes npm/cli#3565