Improper hoisting of peerDependencies: webpack, ajv, ajv-keywords

[CrabDude]

Do you want to request a feature or report a bug?
bug

What is the current behavior?

1. (Top-level) foo lists dependencies:
   • bar@5
   • baz
2. baz lists peerDependencies bar@5.
3. (Top-level) qux & quux list dependencies bar@4
   • Causes bar@4 to hoist, invalidating baz's peerDependency of bar@5)

The result is the following tree:

├─foo
│   └─bar@5
├─baz // (Why is this hoisted? Causes invalid bar peerDependency)
├─bar@4 // (due to qux & quux)
├─qux
└─quux

If the current behavior is a bug, please provide the steps to reproduce.

// In empty directory
$ yarn add  babel-cli@6.24.1 stylelint@7.9.0 webpack@3.1
$ yarn check
yarn check v0.28.1
[...]
error "webpack#ajv-keywords#ajv@>=5.0.0" doesn't satisfy found match of "ajv@4.11.8"
[...]
error Found 1 errors.

What is the expected behavior?

Don't hoist dependencies (i.e., baz & ajv-keywords) w/o hoisting peerDependencies (i.e., bar & ajv).

In the example above, ajv-keywords should not be hoisted, and left as a sub-dependency of webpack.

Please mention your node.js, yarn and operating system version.

yarn@0.28.1
node@4.8.2

Related:

#3465
#2688

Dupes:

#3916
webpack/webpack#5262

[CrabDude]

For posterity, this can be circumvented by adding ajv@5 to package.json to squat on the otherwise hoisted ajv@4's tree position.

[kaylie-alexa]

This seems like the intended behavior to me. baz is a the top level because it's the only dependency, and bar@4 is at the top because hoisting prioritizes the number of occurrences (both qux & quux requires it). The idea of peerDependency is that you'd install it at top level as a peer. So if you're using baz, which requires bar@5 as a peer, then you should install bar@5 at top level (as you did), and bar@4 will fall under the dependency that required it as a dependency.

[CrabDude]

@kaylieEB Your argument definitely has some merit for shallow dependency trees like the original example, but there're some problematic implications of thinking about peerDependencies that way...

First, yarn's current implementation is in contradiction to the design of peerDependencies (npm/npm#1400). The webpack package authors ensured in their dependencies that ajv-keywords' peerDependency ajv@5 is satisfied per npm's peerDependencies requirements:

Each entry must be installed in the family (at the same level or higher) as the package. (If the package is the top target, and not global, then install as normal deps, maybe, just for testing/development purposes?)

ajv@5 is not installed "at the same level or higher" by yarn in disregard of webpack's, ajv-keywords' and npm's intentions and npm's implementation. Apart from the fact that yarn's current implementation makes it impossible to successfully install said dependencies, confirmed by yarn itself via yarn check, it's also impossible for the package author to ensure yarn does not do this, as it's package.json is already in compliance with the peerDependencies spec.

Further, it is impossible to install multiple versions of the same peerDependency at the top-level, so it's easy to see if we take this example a step further...

├ bar@6 // top-level
├ baz@2 // top-level
└ depA
      ├─foo
      │   └─bar@5
      ├─baz@1 // foo
      ├─bar@4 // qux & quux
      ├─qux
      └─quux

As we can see in the above example, if the top-level package.json depends on the same dependencies, but at different versions, yarn's default hoisting mechanics reported here would make it impossible to install depA. No amount of top-level peerDependencies can ensure this, and even if depA forced an install of bar@5 resulting in the following:

├ bar@6 // top-level
├ baz@2 // top-level
└ depA
      ├─foo
      ├─bar@5 // depA
      ├─baz@1 // foo
      ├─qux
      │   └─ bar@4
      └─quux
          └─ bar@4

If the top-level project no longer has a need for bar@2 and removes it, baz@1 would be further hoisted breaking the peerDependency yet again and making depA's bar@5 wasted/extraneous.

├ bar@6 // top-level & inviable b/c baz@1 requires bar@5
├ baz@1 // foo
└ depA
      ├─foo
      ├─bar@5 // Completely pointless install
      ├─qux
      │   └─ bar@4
      └─quux
          └─ bar@4

In short, yarn's current implementation requires every package.json in the hierarchy that contains a sub-dependency with peerDependencies to always sort out on their own all the peerDependencies that might get hoisted, and resolve them manually themselves by including every peerDependencies of sub-dependencies in their own package.json, which is subject to change based on not their top-level dependencies, but their sub-dependencies and transitive non-dependencies. Further, subsequent installs, without frozen sub-dependencies, would require every package.json in the hierarchy to be updated every time in order to accommodate yarn's irregular (as compared to npm) tree resolution logic. It effectively makes peerDependencies synonymous with global singletons as opposed to local singletons.

This would effectively make peerDependencies useless because dependencies declaring them would not only not install them (by design), but would also not even be assured that they'll be respected when the parent package properly does install them. Of course, naturally, a top-level package.json also has no need for peerDependencies b/c nobody will consume it. In short, yarn's current implementation would obviate or eliminate any usefulness of peerDependencies through a combination of lack of assurances, non-deterministic non-local (to a packge) resolution via tree-resolution logic, and the singleton (one version per level) nature of top-level dependencies.

Again, the effect of yarn's current implementation would be that every package.json in the hierarchy would have to explicitly declare sub-dependencies peerDependencies (and sub-dependencies' sub-dependencies peerDependencies, ad infinitum), which are of course subject to change without requiring a major or minor semver.

This would be a de facto breaking and (likely) fatal change from npm and its package.json ecosystem, in addition to the node.js' community of package authors and peerDependencies spec.

[kaylie-alexa]

Thanks for the explanation, I can now see the issue in your complex scenario. It seems like peer dependencies need to be aware of its requested context and be placed relative its parent dependency, instead of being deduped and being hoisted like other dependencies. In some scenarios, this may mean that a top level dependency somehow needs to be nested elsewhere since there's only 1 global peer, even though there may just be one of that top level dependency, because the position of its peer dependency is already taken up by another. This seems like a pretty significant change to the current hoisting logic. Thoughts on this @BYK @arcanis?

[BYK]

I'm not entirely convinced that what Yarn does is incorrect here. The scenario explained to me looks more like a dependency clash where different dependencies require different versions of their peer dependencies, causing a clash during hoisting.

@CrabDude instead of explaining this in as prose, would you be okay creating a minimum test case that we can see with some package.json files? Then I'd have an easier time making sure this is

• not intended behavior
• not an unfortunate package class

This would also help us write a test case if/when we fix this.

Finally, I have a feeling that #3868 may help alleviate this issue. What do you think?

[CrabDude]

The scenario explained to me looks more like a dependency clash where different dependencies require different versions of their peer dependencies, causing a clash during hoisting.

@BYK Agreed. This is the problem. My examples demonstrate and explain why it's a problem for yarn and not for the package authors or the package consumers.

In short, there are 2 ways to looks at this:

1. peerDependencies are effectively global singletons, so collisions are the concern of the consumer. (yarn's current approach, but incomplete w/o enforcing a single instance of a given package)
2. peerDependencies exist to share a dependency across multiple peer-level dependencies (local singletons), so presuming peers do not require incompatible peerDependencies collisions are not an issue. (npm's current implementation)

yarn doesn't hoist all peerDependencies and (en)force a single global version. Instead, when installing, it ignores their existence and depending on hoisting implementation details, will cause conflicts and breakages sometimes in a unrelated manner.

---

Here are several examples, including the real example in the OI, of package.jsons that break only when using yarn:

EXAMPLE: Original

This package.json has worked since the beginning of peerDependencies (contained in part in the referenced webpack issue), but breaks in yarn.

{
    "dependencies": {
        "babel-cli": "^6.24.1",
        "stylelint": "^7.9.0",
        "webpack": "^3.1"
    }
}

EXAMPLE: 1

{
    "dependencies": {
        "dep-a": "*"
    }
}

{
    "name": "dep-a",
    "dependencies": {
        "babel-cli": "^6.24.1",
        "stylelint": "^7.9.0",
        "webpack": "^3.1"
    }
}

EXAMPLE: 2

{
    "dependencies": {
        "dep-a": "*"
    }
}

{
    "name": "dep-a",
    "dependencies": {
        "babel-cli": "^6.24.1",
        "ajv": "4", // Insufficiently addresses incorrect hoisting
        "stylelint": "^7.9.0",
        "webpack": "^3.1"
    }
}

EXAMPLE: 3

{
    "dependencies": {
        "dep-a": "*",
        "ajv": "4" // Extraneous at the consumer level, but required due to incorrect hoisting
    }
}

{
    "name": "dep-a",
    "dependencies": {
        "babel-cli": "^6.24.1",
        "ajv": "4",
        "stylelint": "^7.9.0",
        "webpack": "^3.1"
    }
}

EXAMPLE: 4

{
    "dependencies": {
        "babel-cli": "^6.24.1",
        "stylelint": "^7.9.0",
        "webpack": "^3.1",
        "ajv": "4" // Extraneous at the consumer level, but required due to incorrect hoisting
    }
}

In this final example, if stylelint or babel-cli added a dependency on ajv-keywords@3 (or any other version incompatible with webpack#ajv-keywords@2), it would be hoisted instead of webpack#ajv-keywords@2, and cause the above package.json to break again b/c the attempt to address webpack#ajv-keywords#ajv peerDependency is now obsolete despite an identical package.json.

Effectively, the same package.json that worked one day, will fail to install the next day no matter how strict the dependencies' semvers.

[BYK]

@CrabDude - I think I understand some of the problems but, your examples are still not clear enough to clearly and reliably reproduce this case. Please provide us the following:

A repo/gist that demonstrates your case, complete with stable package.json files and dependency folders so when someone clones it and then runs yarn install in it, it shows the problem clearly by Yarn failing. Only after that we can strip the facts from opinions and try to come to a consensus with you and all the contributors on the project. After that, we can make a clear statement saying "we believe this is the right way, because" or we can say "oh yeah, this is broken and here's a test case we can reproduce it and then confirm when we fix it."

Right now it is tough to separate what is happening, what should be happening and, what are your personal views about how it should be done.

Makes sense?

[CrabDude]

but, your examples are still not clear enough to clearly and reliably reproduce this case.

Only after that we can strip the facts from opinions and try to come to a consensus with you and all the contributors on the project.

Your statements frustrate me. With each of the examples, you personally can reproduce the described outcomes with 100% consistency, the definition of reliable facts. You're also not responding directly to any of the examples specifically, or their objective 100% reliable/reproducible outcomes.

Right now it is tough to separate what is happening, what should be happening and, what are your personal views about how it should be done.

1. What is happening?

The same package.json may break depending on the peerDependencies of (non immediate or immediate) sub-dependencies due to yarn's 100% reliable hoisting mechanics, and there is nothing any one can do about it:

1. Not the authors of the packages with peerDependencies
2. Not the authors of packages that consume those packages
3. Not the top-level consumer).

This is what my examples demonstrate, that yarn hoisting peerDependencies is breaking, and that it's yarn's issue, not the authors, b/c the authors cannot prevent nor address it.

2. What should be happening?

A constant package.json should not break across installs. If dependency APIs have not changed, but a 5 level deep sub-depdendency adds a peerDependency, it should not cause the same constant package.json to unnecessarily fail to install.

This is objectively different from npm's implementation of peerDependencies, easily demonstrated by the fact that this failure to install a constant package.json when a sub-dependency changes a peerDependency only occurs with yarn.

3. What are your personal views?

This implementation is incompatible with the ecosystem and contrary to the design and canonical implementation of peerDependencies in npm. This will cause numerous packages in the ecosystem to avoidably be uninstallable. The original example demonstrate how trivial it is to create this problem with only 3 (very popular/common) dependencies.

4. How should it be done?

As npm handles peerDependencies. Don't hoist a package w/o considering them.

@sokra @ahmehri @haines @blexrob @guigrpa Thoughts?

[CrabDude]

I'll add that I will put together the repo as requested, despite my concerns about the present effectiveness of my communication.

[arcanis]

Pretty sure @BYK doesn't want to frustrate you, @CrabDude - to give you some context, we're currently very hard at work on closing the critical issues remaining to publish a 1.0 RC, so right now we really need to be spoon-fed on issues requiring brain time 😄

I'll add that I will put together the repo as requested, despite my concerns about the present effectiveness of my communication.

Thanks a lot! We'll look at this and come back to you. I understand this might be frustrating, but please rest assured we'll give this issue all the attention required - we're just having a lot of context switches at the moment!

[haines]

I've closed #3916 which was a duplicate of this.

I agree that this is a Yarn issue, not a package issue. For the package in question, Yarn could install dependencies in a way that doesn't break the peer dependency constraint, but it doesn't, because it ignores that constraint when hoisting.

[arcanis]

Ok, I think I got it, that's definitely a bug. Yarn should not hoist a dependency past the point where it would be able to require its own dependencies, whether they're defined as regular dependencies or peerDependencies.

Here's a repro script based on the first comment - when running this script, the final line should be 2.0.0 (which is the version required in c's peer dependency entry), but since c has been hoisted too much, it instead prints 1.0.0.

rm -rf foo

(
    mkdir -p foo && cd foo
    echo '{"dependencies":{"a":"file:./a", "d":"file:./d", "e":"file:./e"}}' > package.json
    echo 'require("a")' > index.js
)

(
    mkdir -p foo/a && cd foo/a
    echo '{"name":"a", "version":"1.0.0", "dependencies":{"b":"file:../b-2", "c":"file:../c"}}' > package.json
    echo 'require("c")' > index.js
)

(
    mkdir -p foo/b-1 && cd foo/b-1
    echo '{"name":"b", "version":"1.0.0"}' > package.json
)

(
    mkdir -p foo/b-2 && cd foo/b-2
    echo '{"name":"b", "version":"2.0.0"}' > package.json
)

(
    mkdir -p foo/c && cd foo/c
    echo '{"name":"c", "version":"1.0.0", "peerDependencies":{"b":"2.0.0"}}' > package.json
    echo 'console.log(require("b/package.json").version)' > index.js
)

(
    mkdir -p foo/d && cd foo/d
    echo '{"name":"d", "version":"1.0.0", "dependencies":{"b":"file:../b-1"}}' > package.json
)

(
    mkdir -p foo/e && cd foo/e
    echo '{"name":"e", "version":"1.0.0", "dependencies":{"b":"file:../b-1"}}' > package.json
)

(
    cd foo

    yarn

    echo
    tree node_modules
    echo

    for x in $(find node_modules -name package.json); do
        echo $x
        jq .version < $x
    done

    echo
    node index.js
)

[CrabDude]

@arcanis Thanks for putting this together.

Is there a list of outstanding issues for the 1.0 RC? I don't see a Milestone or label for it.

[arcanis]

We try to group everything inside this project

[BYK]

@CrabDude - I certainly didn't want to frustrate you, and I appreciate all the time you've put into this issue and your explanations.

The problem I have is a large volume of issues that we have to verify: is this an actual bug, is this a feature request disguised as a bug, is this a need for parity with npm where we don't have to etc. Having the whole issue described as prose and not as simple as "get the package.json provided, run yarn install, observe the output" or git clone example-repo; cd example-repo; yarn install made it quite hard for me to make that decision.

For this specific example, it required a lot of time to understand, and then manually reproduce the issue. On top of that, the dependencies you use to demonstrate have lots of sub dependencies making the issue harder to isolate.

[CrabDude]

Thanks @BYK & @arcanis. I said "frustrated" to allow for things being as you have described them, in recognition that it may very well be due to my choice of communication. From my perspective, it did not seem the package.jsons would demonstrate anything my examples did not, but that was just my perspective and it was the wrong way to look at this.

Thanks for your patience and work on the project.

[arcanis]

Currently working on a fix - I managed to fix the testcase exposed above, but I still have an error when running yarn add babel-cli@6.24.1 stylelint@7.9.0 webpack@3.1 && yarn check. In progress! :)

[ernani]

Following the issue for updates. Thanks!