Skip to content
This repository has been archived by the owner on Jan 20, 2022. It is now read-only.

[BUG] peerOptionals can cause ERESOLVE in peerSet, but aren't included in tree #223

Closed
isaacs opened this issue Feb 9, 2021 · 1 comment · Fixed by #227
Closed

[BUG] peerOptionals can cause ERESOLVE in peerSet, but aren't included in tree #223

isaacs opened this issue Feb 9, 2021 · 1 comment · Fixed by #227
Assignees
Labels
Bug thing that needs fixing

Comments

@isaacs
Copy link
Contributor

isaacs commented Feb 9, 2021

For these dependency graphs:

# case a
root -> (x, y@1)
x -> PEEROPTIONAL(z)
z -> PEER(y@2)

# case b
root -> (x) PEEROPTIONAL(y@1)
x -> PEEROPTIONAL(y@2)

# case c
root -> (x) PEEROPTIONAL(y@1)
x -> PEER(z)
z -> PEEROPTIONAL(y@2)

The peerOptional dependency is included in the peerSet, raising an ERESOLVE conflict at the peerSet generation stage, even though the peerOptional dependencies will not actually be added to the tree.

In --force mode, the peerSet generation error is ignored, since we produce a better warning by waiting until we conflict at the tree generation stage. Since there is no actual conflict, no warning is generated.

But in default mode, we see the error in the peerSet generation, and raise the error early, assuming that there will be no resolution possible.

We should never have a situation where we crash without --force, but do not raise a warning with --force.

We should never warn about a dependency that we are not going to actually install.

@isaacs
Copy link
Contributor Author

isaacs commented Feb 9, 2021

Examples:

{
  "name": "ng-optional-deps",
  "version": "1.0.0",
  "dependencies": {
    "@angular-devkit/build-angular": "^0.1101.4",
    "@angular/compiler-cli": "11.0"
  }
}
{
  "name": "ng-peer-optional-repro",
  "version": "1.0.0",
  "dependencies": {
    "ng-packagr": "11.1.3"
  }
}

@isaacs isaacs self-assigned this Feb 9, 2021
@isaacs isaacs added the Bug thing that needs fixing label Feb 9, 2021
isaacs added a commit that referenced this issue Feb 11, 2021
For these dependency graphs:

    # case a
    root -> (x, y@1)
    x -> PEEROPTIONAL(z)
    z -> PEER(y@2)

    # case b
    root -> (x) PEEROPTIONAL(y@1)
    x -> PEEROPTIONAL(y@2)

    # case c
    root -> (x) PEEROPTIONAL(y@1)
    x -> PEER(z)
    z -> PEEROPTIONAL(y@2)

The peerOptional dependency is included in the peerSet, which would
raise an ERESOLVE conflict at the peerSet generation stage, even though
the peerOptional dependencies will not actually be added to the tree.

To address this, this commit tracks the nodes which are actually
required in the peerSet generation phase, by virtue of being
non-optionally depended upon by a required node in the peerSet.

If a conflict occurs on a node which is not in the required set during
the peerSet generation phase, we ignore it in much the same way that we
would ignore peerSet errors in metadependencies or when --force is used.

Of course, if the peerOptional dependency is _actually_ required, to
avoid a conflict with an invalid resolution present in the tree already,
and there is no suitable placement for it, then ERESOLVE will still be
raised.

Fix: #223
Fix: npm/cli#2667
isaacs added a commit that referenced this issue Feb 11, 2021
For these dependency graphs:

    # case a
    root -> (x, y@1)
    x -> PEEROPTIONAL(z)
    z -> PEER(y@2)

    # case b
    root -> (x) PEEROPTIONAL(y@1)
    x -> PEEROPTIONAL(y@2)

    # case c
    root -> (x) PEEROPTIONAL(y@1)
    x -> PEER(z)
    z -> PEEROPTIONAL(y@2)

The peerOptional dependency is included in the peerSet, which would
raise an ERESOLVE conflict at the peerSet generation stage, even though
the peerOptional dependencies will not actually be added to the tree.

To address this, this commit tracks the nodes which are actually
required in the peerSet generation phase, by virtue of being
non-optionally depended upon by a required node in the peerSet.

If a conflict occurs on a node which is not in the required set during
the peerSet generation phase, we ignore it in much the same way that we
would ignore peerSet errors in metadependencies or when --force is used.

Of course, if the peerOptional dependency is _actually_ required, to
avoid a conflict with an invalid resolution present in the tree already,
and there is no suitable placement for it, then ERESOLVE will still be
raised.

Fix: #223
Fix: npm/cli#2667
isaacs added a commit that referenced this issue Feb 11, 2021
For these dependency graphs:

    # case a
    root -> (x, y@1)
    x -> PEEROPTIONAL(z)
    z -> PEER(y@2)

    # case b
    root -> (x) PEEROPTIONAL(y@1)
    x -> PEEROPTIONAL(y@2)

    # case c
    root -> (x) PEEROPTIONAL(y@1)
    x -> PEER(z)
    z -> PEEROPTIONAL(y@2)

The peerOptional dependency is included in the peerSet, which would
raise an ERESOLVE conflict at the peerSet generation stage, even though
the peerOptional dependencies will not actually be added to the tree.

To address this, this commit tracks the nodes which are actually
required in the peerSet generation phase, by virtue of being
non-optionally depended upon by a required node in the peerSet.

If a conflict occurs on a node which is not in the required set during
the peerSet generation phase, we ignore it in much the same way that we
would ignore peerSet errors in metadependencies or when --force is used.

Of course, if the peerOptional dependency is _actually_ required, to
avoid a conflict with an invalid resolution present in the tree already,
and there is no suitable placement for it, then ERESOLVE will still be
raised.

Fix: #223
Fix: npm/cli#2667
isaacs added a commit that referenced this issue Feb 11, 2021
For these dependency graphs:

    # case a
    root -> (x, y@1)
    x -> PEEROPTIONAL(z)
    z -> PEER(y@2)

    # case b
    root -> (x) PEEROPTIONAL(y@1)
    x -> PEEROPTIONAL(y@2)

    # case c
    root -> (x) PEEROPTIONAL(y@1)
    x -> PEER(z)
    z -> PEEROPTIONAL(y@2)

The peerOptional dependency is included in the peerSet, which would
raise an ERESOLVE conflict at the peerSet generation stage, even though
the peerOptional dependencies will not actually be added to the tree.

To address this, this commit tracks the nodes which are actually
required in the peerSet generation phase, by virtue of being
non-optionally depended upon by a required node in the peerSet.

If a conflict occurs on a node which is not in the required set during
the peerSet generation phase, we ignore it in much the same way that we
would ignore peerSet errors in metadependencies or when --force is used.

Of course, if the peerOptional dependency is _actually_ required, to
avoid a conflict with an invalid resolution present in the tree already,
and there is no suitable placement for it, then ERESOLVE will still be
raised.

Fix: #223
Fix: npm/cli#2667
@isaacs isaacs closed this as completed in f375873 Feb 12, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Bug thing that needs fixing
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant