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

Inherit peer dependency ranges from your own dependencies. #3

Closed
arcanis opened this issue Feb 19, 2019 · 13 comments
Closed

Inherit peer dependency ranges from your own dependencies. #3

arcanis opened this issue Feb 19, 2019 · 13 comments
Labels
ecosystem This feature affects the whole ecosystem enhancement New feature or request

Comments

@arcanis
Copy link
Member

arcanis commented Feb 19, 2019

Describe the user story

I'm a package author, and my package depends on another package which has a peer dependency on react. Since I don't want to provide this package I must indicate that my own package has a peer dependency on react, but I also need to convey that the required react version simply is the same one has the one requested by my dependency. I currently have to manually copy it, which is error-prone since it won't be automatically updated when my dependency is upgraded and starts using a different range.

Describe the solution you'd like

Yarn should support a new special range specifier for peer dependencies: inherit. When inherit is specified, the peer dependency will automatically become the union of all the ranges of the matching peer dependencies of dependencies of the package.

Describe the drawbacks of your solution

From a technical standpoint it can be seen as making a parent depend on its children (since the exact peer dependency will only be resolvable if the children are available). In practice this shouldn't be a problem though, because the peer dependencies aren't taken into account until after the dependency tree has been computed (because they are a non-binding suggestion that the user is responsible for getting right).

The inherit range keyword will not be properly understood by lower Yarn versions, which might cause some warnings to appear in such cases. Given that only the packages that deemed this feature useful enough to warrant the potential warning will be affected I don't think it's a blocker.

Describe alternatives you've considered

Instead of an inherit keyword the * range could be repurposed. I'm afraid this would be a surprising change, and it would wildly break the semver expectations.

Instead of encoding the inherit status into the range, it could be moved into the peerDependenciesMeta field. This would mean that the range described in the peerDependencies field would have no actual impact, which would be quite unexpected. I believe the range is where this feature has the most sense semantically speaking.

@arcanis arcanis added the enhancement New feature or request label Feb 19, 2019
@arcanis arcanis added the ecosystem This feature affects the whole ecosystem label Feb 28, 2019
@Jessidhia
Copy link

Jessidhia commented Mar 7, 2019

This is essentially how DefinitelyTyped "implements" its peer dependencies. It actually generates dependencies using a * version to make it a kind of "weak version" -- it is supposed to resolve to whatever other version is requested by the user, and only if there is no stronger version requested does it fall back to latest.

Yarn currently just always treats * as an alias to latest which is a cause of a great many bug reports in DefinitelyTyped that are actually just packages not getting deduplicated between updates, either because yarn upgrade-interactive --latest won't touch transitive dependencies if they still satisfy the version range (which * currently does), or because you have requested a stricter version (say, tilde or even exact) that isn't satisfied by whatever is latest.


I do think that these should be specified as peerDependencies instead, as duplicates are definitely not acceptable and break builds. This is just how a difference in behavior related to this between yarn and npm causes problems.

@bradleyayers
Copy link
Contributor

What's the recommendation here, when I encounter a package that hasn't re-declared peerDependencies of its dependencies, should I raise an issue with the author to add those peerDependencies in? Should they copy the version across, or just use *? What's going to have the best compatibility with NPM and Yarn 1?

Also, is there a way to use resolutions in Yarn 2 to work around the problem temporarily while waiting for upstream packages to be patched and released?

@eps1lon
Copy link
Member

eps1lon commented Jun 18, 2019

which is error-prone since it won't be automatically updated when my dependency is upgraded and starts using a different range.

On the flipside if that dependency changes it's peer dependency version it's a breaking change for my library as well. So if I bump the major of that dependency I would have to release a major as well. If the peer is just "inherit" it's not obvious what the breaking change is because a new major of a dependency doesn't necessarily mean it's a breaking change for my library (e.g. when it's only used internally).

While this is a bit annoying now because it adds churn to the ecosystem it might actually be a healthy change in the long term with regard to SemVer.

@arcanis
Copy link
Member Author

arcanis commented Jun 18, 2019

Yarn currently just always treats * as an alias to latest which is a cause of a great many bug reports in DefinitelyTyped that are actually just packages not getting deduplicated between updates, either because yarn upgrade-interactive --latest won't touch transitive dependencies if they still satisfy the version range (which * currently does), or because you have requested a stricter version (say, tilde or even exact) that isn't satisfied by whatever is latest.

Yarn treats * as "anything goes". The main catch is that (at least for Yarn 2), we never backtrack the resolution. So let's say that you depend on foo@* and bar@1, Yarn will first resolve both foo and bar before considering any of their own dependencies. Let's say that it selected foo@42. Now let's imagine that bar@1 actually depends on foo@21 - theoretically * could be switched to use foo@21 and thus deduplicate foo. But if we do this, then we need to do a potentially exponential amount of backtracking (perfect resolution is NP-Complete), and potentially need to make many more requests. So we don't try to be too intelligent for our own good, and don't come back on our decisions.

What's the recommendation here, when I encounter a package that hasn't re-declared peerDependencies of its dependencies, should I raise an issue with the author to add those peerDependencies in? Should they copy the version across, or just use *? What's going to have the best compatibility with NPM and Yarn 1?

I'd advise the affected packages to simply list the same range as the library they depend on. Worst case scenario they'll get some warnings if their dependency is updated, by which point whether they want to expand their own range or restrict the version of their dependency to keep the peer dependency in sync with their own.

Also, is there a way to use resolutions in Yarn 2 to work around the problem temporarily while waiting for upstream packages to be patched and released?

Not with resolutions, but you can manually edit the yarn.lock file to add the peer dependency to the entry of the affected dependency. It's an advanced use case (if you upgrade the dependency you'll need to apply the changes again), but it's a good way to move forward until the PR gets merged upstream.

While this is a bit annoying now because it adds churn to the ecosystem it might actually be a healthy change in the long term with regard to SemVer.

Yep, maybe 🤔 I think we'll see whether there's an ask for this feature before considering implementing it. So far maintainers I've spoke with were fairly understanding.

@Jessidhia
Copy link

A possible way of dealing with the * problem is to not actually resolve * versions immediately, but add them to a list. Once all other packages are resolved, resolve the * versions by preferring other resolutions instead if they exist, and only looking up the latest version if it's not fulfilled by that second pass.

@bradleyayers
Copy link
Contributor

FWIW what I do in yarn 1 for this is run yarn-deduplicate after every time I install a new package. The yarn docs say yarn dedupe isn't necessary since it's part of yarn install, but I haven't found that to be the case.

It is annoying and would be great if yarn did actually deduplicate by default, but the idea of needing to do a "second pass" afterwards to deduplicate doesn't bother me too much. If it was baked in as a --dedupe flag that would fix the developer experience for me. If this was the path for yarn 2 I'd be satisfied with that.

@arcanis
Copy link
Member Author

arcanis commented Jun 19, 2019

Quick note: I think we're going off-topic here. This issue is about inheritable peer dependency ranges. The * problem you mention is about DefinitelyTyped using dependencies when they should use peer dependencies. I'll still answer, but if you want to pursue the subject please open a "Case Study" thread that summarise the various existing threads.


We may do a dedupe pass later, but DefinitelyTyped is still definitely using incorrect semantics, and I won't act based on their experience alone.

For the record, between this topic, this other topic, this article, and various sub-issues I can't find, I wrote more than 15,000 symbols just about this. Everything is documented, including the proper fix. I don't want to add new semantic constraints to package managers just because one project used them wrong and noone else wants to deal with fixing it.

@lroling8350
Copy link
Contributor

@arcanis Is it possible to consider not listing anything as inheriting? Maybe make this an option in the rc file? I've successfully changed the Project.ts locally to do just this with only a few lines and wanted to get your thoughts before opening a PR.

The only downside is a package may be less explicit. However, does adding a peerDependency entry and making it optional provide that much value? The information has the potential to become stale if the peer dependency is removed (which can possibly be a minor change in semver). Also, the typical use case (for me at least) is to look at a packages dependencies, verify i'm happy with them and do a add/install. If any warnings show, which they would if the peer still isn't satisfied, i handle those appropriately.

@merceyz
Copy link
Member

merceyz commented Dec 24, 2020

I've successfully changed the Project.ts locally to do just this with only a few lines and wanted to get your thoughts before opening a PR.

@lroling8350 sharing is caring

@kherock
Copy link
Contributor

kherock commented Jul 1, 2021

Shouldn't this proposed behavior be automatic when no peer dependency is specified by a parent? Or does this break some expectation of PnP? It seems feasible for Yarn to use the entire ancestor list for satisfying peer dependencies rather than the immediate parent. I think this is what @lroling8350 is proposing.

So far, most of my packageExtensions end up being from dependencies that expect a grandparent (typically my project root) to have a dependency specified. Their parents don't typically have an opinion on the version of the peer since they don't use it at runtime. These particular extensions could be eliminated if Yarn could generate peer dependency unions for ancestors after the dependency tree is computed. They would simply be unions of the peer dependencies of its children minus the dependencies that the parent provides itself.

Below is a basic scenario where this automatic behavior is useful (the Storybook repo is full of these)

Consider the following packages @foo/core, @foo/helper, package-a, package-b, and package-with-peers:

@foo/core
├── @foo/helper
└── package-a

@foo/helper
├── package-with-peers
├── package-b
└── computed: package-a (peer)

package-with-peers
├── package-a >=1 (peer)
└── package-b >=1 (peer)

When inspecting @foo/helper as the project root, it's missing package-a requested by package-with-peers, and it wouldn't have a parent to bubble package-a up to. From here, @foo/helper has three options (and ideally Yarn would inform it of each upon adding package-with-peers):

  1. Add package-a as a dependency. This should only be chosen when there is project code importing package-a.
  2. Add package-a as a devDependency and as a peerDependency. Again, this is only recommended if @foo/helper has a direct import somewhere.
  3. Add package-a only as a devDependency. Projects that depend on @foo/helper are warned that package-with-peers has no ancestor satisfying package-a. Imports to package-a from @foo/helper's project code should fail.
    • This would allow package-with-peers to add and remove peerDependencies without it being considered a breaking change for @foo/helper.

Now consider a new project that depends on @foo/core. They run $ yarn add @foo/core, and their project has the following tree:

.
└── @foo/core
    ├── @foo/helper
    │   ├── package-with-peers
    │   └── package-b
    └── package-a

This configuration would allow package-with-peers to resolve its peer dependencies without ambiguity; however, Yarn's current PnP linker stops looking for package-a once it sees that @foo/helper failed to provide it with "package-a": "*". I can't think of a scenario where@foo/helper would be motivated to add a specific version of package-a other than * if it doesn't have any code that imports it. To me it seems like parents should implicitly forward the peer dependencies of its children.

@arcanis
Copy link
Member Author

arcanis commented Jul 1, 2021

No - on top of being algorithmically difficult to implement (it's easier with nm because nm has limitations that make certain scenarios non-problems because they can't be supported in the first place), it would also prevent various very convenient assumptions (like "a package without peer dependencies will only ever be instantiated once"), which would have far-reaching effects.

I understand it may look easy (and I myself made a few attempts to rethink this, trying to see whether it was just me being stubborn), but my final conclusion is that implicit inheritance of peer dependencies is an antipattern that only provides some convenience at the cost of soundness and maintainability (as in, maintenance of a package manager). Given that:

  • enforcing this requirement is perfectly compatible with how other package managers work (in that your packages won't fail elsewhere just because you follow it)

  • there's a decent semantical argument that transitive dependencies are a private part of an API (meaning that in a A->B->C scenario, which deps A has shouldn't affect B's behavior unless it explicitly opt-in via its own peer dependency requirements; if B magically inherited peer deps from C then this contract would be broken)

Given those two parameters I don't feel too bad not implementing this shortcut. Still, if someone wants to prove me wrong I'd certainly review a PR - but I believe this will be a difficult endeavor, and as such it's unlikely I'll spend further time investigating this.

@arcanis
Copy link
Member Author

arcanis commented Jul 1, 2021

I'm also going to close this issue because Yarn now supports range aggregates¹, which solves the initial use case of this thread (ie "explicitly opt-in to a peer dep, but inherit the peer dependency range from the children").

¹ If a package A lists, say, "foo": "^1.0.0" as peer dependency, and one of its transitive dependencies lists a stricter range (such as ^1.1.0), Yarn will refine the range into the stricter range overlap when running its checks. In this case, it means that A's peer dependencies would become ^1.1.0 since that's the overlap of ^1.0.0 and ^1.1.0. In other words, if you set * as your peer dependencies range, Yarn will smartly inherit the range from its children.

@arcanis arcanis closed this as completed Jul 1, 2021
@patrick-mcdougle
Copy link

patrick-mcdougle commented Nov 5, 2021

IMO this doesn't quite solve the problem either. If the package (call it "A") doesn't actually need the peer dependency ("P") at all and is just forwarding it for its dependency ("B") [and puts peerDependency B: * in it's package.json], what happens when the transitive dependency ("B") no longer requires the peer ("P") that it used to ask for? Now package ("A") requests a peer for no reason whatsoever (maybe it bumps versions less often). This isn't very clear or detectable. I'm still quite preferential to yarn defaulting to "forwarding" or setting * as the default for all peer dependencies until it hits an ancestor that does declare it in dependencies OR if it hits root then we should throw the warning / error in the resolve step.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ecosystem This feature affects the whole ecosystem enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

8 participants