Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: npm dedupe property 'path' of null error #3632

Closed
wants to merge 1 commit into from

Conversation

ParadoxInfinite
Copy link

From what I noticed by console logging dep when running npm dedupe, when dep.isLink was true, dep.target was null. Also, dep.realpath exists, so I am not 100% sure, but making an educated guess that we are supposed to use that instead of trying to look for the real path in the target which is null.

Would appreciate feedback, if any!

References

Fixes #3565

@ParadoxInfinite ParadoxInfinite requested a review from a team as a code owner August 8, 2021 19:57
@ljharb
Copy link
Contributor

ljharb commented Aug 8, 2021

This change would need to go directly into arborist - not into the vendored copy in this repo.

@ParadoxInfinite
Copy link
Author

@ljharb But does the change look correct? If yes, I'll raise the PR to the arborist repo by forking it.

@ljharb
Copy link
Contributor

ljharb commented Aug 8, 2021

Seems right but I have no idea.

@ljharb
Copy link
Contributor

ljharb commented Aug 8, 2021

Although you’d need a regression test as well.

@ParadoxInfinite
Copy link
Author

Not sure what that is, any links for me to refer? Also, should I close this PR then?

@ljharb
Copy link
Contributor

ljharb commented Aug 8, 2021

Yes, I'd close this one.

A "regression test" means, add a test case that fails without your fix, but passes with it, so that it prevents the bug from regressing in the future.

@ParadoxInfinite
Copy link
Author

Okay, thank you for the info too. I'll be sure to add a regression test in my PR to arborist!

@ParadoxInfinite ParadoxInfinite deleted the fix-3565 branch August 9, 2021 07:03
isaacs added a commit to npm/arborist that referenced this pull request Aug 12, 2021
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
isaacs added a commit to npm/arborist that referenced this pull request Aug 12, 2021
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

PR-URL: #309
Credit: @isaacs
Close: #309
Reviewed-by: @nlf
wraithgar added a commit that referenced this pull request Aug 12, 2021
  * [#3632] Fix "cannot read property path of null" error in 'npm
    dedupe'
  * fix(shrinkwrap): always set name on the root node
@wraithgar wraithgar mentioned this pull request Aug 12, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] npm dedupe fails with TypeError: Cannot read property 'path' of null
2 participants