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

Optional Peer Dependencies #105

Merged
merged 1 commit into from Dec 17, 2018

Conversation

@arcanis
Copy link
Member

commented Oct 30, 2018

Merged!

https://github.com/yarnpkg/rfcs/blob/master/accepted/0000-optional-peer-dependencies.md

Related discussion: yarnpkg/yarn#6487

cc @edmorley @jscheid @Gudahtt who were part of the initial discussion

Also cc @zkochan (pnpm), @iarna (npm), since you'll likely be affected by this change

@zkochan

This comment has been minimized.

Copy link

commented Oct 30, 2018

It seems to me that the current peerDependencies field behaves in an "optional" way already. A warning is printed but installation does not fail.

(pnpm currently has a config called strict-peer-dependencies that makes installation fail if a peer dependency is not from the wanted range or is missing.)

So I think it may be solved the other way around. We think about the current "peerDependencies" as "optional" (and change the warnings to info messages) + we introduce a strict: protocol or a "strictPeerDependencies" field


IMHO, the use of protocol seems like a bad idea because protocols are currently used for declaring the source: npm:, github:, link:, etc.

@arcanis

This comment has been minimized.

Copy link
Member Author

commented Oct 30, 2018

It seems to me that the current peerDependencies field behaves in an "optional" way already. A warning is printed but installation does not fail.

Yep totally - this is more from a UX perspective. We currently print quite a lot of warning, which:

  1. Makes it impractical for package authors to add integrations with other tools (because it will print non-actionable warnings in their users interfaces)
  2. Causes the users to disregard all warnings because there's so many of them, most of them are unactionable, and they have no easy way to figure which ones are

we introduce a strict: protocol or a "strictPeerDependencies" field

That's an interesting idea - maybe it would be a good thing, yeah. I'm still not fond of the strictPeerDependencies field, though, since older package managers (even Yarn itself) wouldn't know how to interpret them (they would simply ignore them). That seems a bit risky, don't you think?

@zkochan

This comment has been minimized.

Copy link

commented Oct 30, 2018

It may be done in a backward compatible way. For instance, via a strictPeers field that will be just an array of the strict peer dependency names.

{
  "peerDependencies": {
    "foo": "1.x.x",
    "bar": "1.x.x"
  },
  "strictPeers": ["foo"]
}
@arcanis

This comment has been minimized.

Copy link
Member Author

commented Oct 30, 2018

I guess .. it seems a bit too convoluted imo (especially since it doesn't match the other keys names - for example bundleDependencies has a similar design but a totally different name).

Plus, what if we need other such "flags" later on? I'd like to avoid devPeers (just an example on the top of my head) or similar 😕

@zkochan

This comment has been minimized.

Copy link

commented Oct 30, 2018

probably a stupid idea... but this would also work: checking if the version spec starts with ||. It would be backward compatible unlike a new optional: protocol

{
  "peerDependencies": {
    "foo": "|| 1.x.x",
    "bar": "1.x.x"
  }
}
@arcanis

This comment has been minimized.

Copy link
Member Author

commented Oct 30, 2018

Hm could work - we should check that it doesn't make current package managers crash with "invalid semver pattern" or something like that 🤔

@zkochan

This comment has been minimized.

Copy link

commented Oct 30, 2018

It should work because semver handles it fine: https://runkit.com/embed/qjynj6em684l

@ljharb

This comment has been minimized.

Copy link

commented Oct 30, 2018

They're not optional because npm ls exits nonzero; the only reason they don't fail installation is that they're not installed by default. In npm < 3, they would install by default, and an installation failure would fail installation.

@ljharb

This comment has been minimized.

Copy link

commented Oct 30, 2018

In other words, "required" doesn't mean "ensures the installation succeeds", it means "produces a valid dependency graph". Missing peer deps is an invalid graph.

@arcanis

This comment has been minimized.

Copy link
Member Author

commented Oct 31, 2018

@ljharb I think you've also made this point here, right (to provide more context and discussion on the subject)?

As before, I don't really see npm ls as anything else than any one command from any other package manager, so I would tend to rather look at the user expectations rather than take it as a gospel. Ideally we would look at the spec, but there's none, which is in part why we're here: to clearly spec the expectations of a peer dependency being missing.

From what I saw on various threads, most users don't expect missing peer dependencies to cause their installs to fail, so I would be quite strongly opposed to making it an error. Keeping it a warning if missing seems to me the best solution, especially since it's trivial to any of us to add a flag that turns warnings into errors (think -Werror).

@ljharb

This comment has been minimized.

Copy link

commented Oct 31, 2018

Most users also wouldn't expect a package's "engines" declaration having a nonmatching version of node to make their installs fail, but yarn does that.

@arcanis

This comment has been minimized.

Copy link
Member Author

commented Oct 31, 2018

That's a fair point and something we can discuss in a separate RFC if you feel strongly about it 🙂

@ljharb

This comment has been minimized.

Copy link

commented Oct 31, 2018

I would argue that npm install not running npm ls afterwards is an omission that masks many real problems in developer's graphs; I've caught tons of bugs in packages I maintain by running it in CI after an install.

In other words, even though it would make more installs fail, I think that would, in the long run, be a much better experience for developer - because they'd end up with valid dependency graphs that meet the requirements package authors declare for their packages.

@arcanis

This comment has been minimized.

Copy link
Member Author

commented Oct 31, 2018

I would argue that npm install not running npm ls afterwards is an omission that masks many real problems in developer's graphs; I've caught tons of bugs in packages I maintain by running it in CI after an install.

Coincidentally I've opened a separate RFC this morning to get rid of yarn check in Yarn 2 (the main rational behind it being that if our users have to rely on yarn check, it's that we've done something wrong in yarn install) 😄

because they'd end up with valid dependency graphs that meet the requirements package authors declare for their packages.

The problem I have with this is that you're making assumptions regarding the intents package authors had when writing their packages (those that have already been published). It seems dangerous to me for a very little gain. Think of the worst case: a peer dependency from a transitive dependency is missing and causes the whole install to fail in a very non-actionable way.

I prefer to be a bit more conservative on this topic (even though you should know by this point that I love enforcing strictness, after my work on PnP) and have a forgiving behavior. After all it's not entirely the users fault if they weren't properly taught what behavior was to be expected.

@ljharb

This comment has been minimized.

Copy link

commented Oct 31, 2018

That's intentional. An unmet peer dep in a transitive dependency should fail the entire install; it's an error in whatever intermediate dep included the one with the peer, and failed to either redeclare the peer, or declare a dep. It's quite actionable for the top-level user tho - peer dep requirements hoist, so all they have to do is install the missing peer dep at the top level.

@arcanis

This comment has been minimized.

Copy link
Member Author

commented Oct 31, 2018

An unmet peer dep in a transitive dependency should fail the entire install

You can say it "should" but I'd prefer practical arguments. Whether you (or I) think it should or not isn't the point here; instead we need to find a way to work with the ecosystem in a way that:

  1. can be teached
  2. doesn't require a lot of changes

The fact is that some packages are already using peer dependencies to mark optional dependencies because they don't have the choice. Meaning that if you go your way, you would for sure break packages. In contrast, package managers don't currently fail the install if a peer dependency is missing, meaning that it's 100% sure we wouldn't break anything going this way.

I wouldn't necessarily disagree with you if we were to create a new ecosystem. In this situation, though, we only have so many jokers and I don't think it's wise to use one of them on this when we have clear alternatives.

It's quite actionable for the top-level user tho - peer dep requirements hoist, so all they have to do is install the missing peer dep at the top level.

Eh, this one is fun and would worth a separate discussion. Long story short, this is only by accident. If a package A depends on a package B that has a peer dependency C, then A must provide C in some capacity (it can be through a peer dependency itself - the point is: it has to be provided) for it to be provided at all. It shouldn't be picked up magically across package boundaries.

This semantic is a bit borderline (again, it never got taught one way or the other), but I'm not aware of any popular package using such patterns so I'm not too concerned about enforcing it (note that it's off-topic for this discussion; please open a new thread if you want to discuss it). For those that would do, they would actually be helped a lot if they had access to optional peer dependencies.

@arcanis

This comment has been minimized.

Copy link
Member Author

commented Oct 31, 2018

It should work because semver handles it fine: https://runkit.com/embed/qjynj6em684l

Wow .. I guess it should, yep. Apparently, empty string is a valid semver range 😅 Let's see what it gives with the various syntaxes proposed:

{
    "peerDependencies": {
        "typescript": "optional:^3.0.0",
        "webpack": "^4.0.0"
    }
}
{
    "peerDependencies": {
        "typescript": "^3.0.0",
        "webpack": "strict:^4.0.0"
    }
}
{
    "peerDependencies": {
        "typescript": "| ^3.0.0",
        "webpack": "^4.0.0"
    }
}
{
    "peerDependencies": {
        "typescript": "^3.0.0",
        "webpack": "^4.0.0"
    },
    "optionalPeerDependencies": [
        "typescript"
    ]
}
@ljharb

This comment has been minimized.

Copy link

commented Oct 31, 2018

Most every eslint plugin/config and react ecosystem package uses peer deps in this way - to explicitly declare compatibility with the “main” package (eslint, react). Webpack as well, i believe.

What I’ve never seen is people using peer deps to mean “optional” - in that case (like enzyme 2 did) they do a try/catch require and document the package in their readme. Do you have specific examples of packages that intend this semantic?

@arcanis

This comment has been minimized.

Copy link
Member Author

commented Oct 31, 2018

I can definitely look for a more comprehensive list, but one of them that triggered this issue is waysact/webpack-subresource-integrity#90. Another one is TypeStrong/ts-node#697.

in that case (like enzyme 2 did) they do a try/catch require and document the package in their readme.

It's unfortunately unsafe, they shouldn't do this (hence the issues I've opened). Even disregarding PnP, peer dependencies have specific mechanisms to prevent hoisting from being made in a way that would break the require contract. By not listing the dependency, they run the risk of being hoisted in such a way that they won't be able to access the instance from their parent anymore. For example:

- A
    - B (optional peer dependency on C, omitted)
    - C@1
- C@2

In this context, the package manager will quite likely decide to hoist B to the top-level (which won't be possible for C, since the versions would clash), meaning that the instance of C it will get won't be the same one as the one used by A.

@ljharb

This comment has been minimized.

Copy link

commented Oct 31, 2018

I agree that optional peer deps are a useful feature; what I’m arguing is that in practice they are not optional, and are not used that way.

@arcanis arcanis referenced this pull request Oct 31, 2018
@arcanis

This comment has been minimized.

Copy link
Member Author

commented Nov 2, 2018

So - what syntax do we go with? I'd tend to go with either optional: or strict: since they appear more readable and less confusing to me, but I'm not strongly opposed to | either.

Regarding optional vs strict, I think using optional would be better. I agree with @ljharb that most packages expect peer dependencies to be required (but disagree that all of them do so), and it seems better to me to warn too much than not enough.


Last call for the npm folks (@iarna), if you wish to contribute. It's a simple but important change and it would be better if all package managers were aligned in this. Otherwise the sheer demand will prompt us to implement it nonetheless, but it might make your devx slightly less good 🙂

@rwjblue

This comment has been minimized.

Copy link

commented Nov 2, 2018

Regarding optional vs strict, I think using optional would be better. I agree with @ljharb that most packages expect peer dependencies to be required (but disagree that all of them do so), and it seems better to me to warn too much than not enough.

I strongly agree here.

Last call for the npm folks (@iarna), if you wish to contribute. It's a simple but important change and it would be better if all package managers were aligned in this. Otherwise the sheer demand will prompt us to implement it nonetheless, but it might make your devx slightly less good 🙂

I'm pretty concerned with moving forward without some feedback from the NPM side. Without support from NPM, package authors are really put in a bad spot where they must choose to only support Yarn consumers.

So - what syntax do we go with? I'd tend to go with either optional: or strict: since they appear more readable and less confusing to me, but I'm not strongly opposed to | either.

If we are unable to agree on the semantics amongst package managers, IMHO we should strongly prefer syntax that at the least doesn't break one of them (@zkochan's suggestion of ||)...

@arcanis

This comment has been minimized.

Copy link
Member Author

commented Nov 2, 2018

Without support from NPM package authors are really put in a bad spot where they must choose to only support Yarn consumers.

Note that changes we make here will work on all package managers, even those that don't implement optional:. This is because peer dependency ranges are merely an hint, and even if they don't match the install shouldn't fail (it should warn, though). So even in the worst case scenario, the npm users will only get a warning telling them that optional:^1.0.0 isn't satisfied even when provided 1.0.0. Annoying but not ecosystem-breaking.

I'd still prefer for them to manifest but I can't force them to unless they want to, and given that some of them blocked me on Twitter I'm not overly confident they're in a collaborating mood. Hopefully I'm wrong there 🙂

If we are unable to agree on the semantics amongst package managers, IMHO we should strongly prefer syntax that at the least doesn't break one of them

To be clear about the effect (I made two packages to test: optpeer1 and optpeer2):

package manager dependency not provided provided
yarn optional:^1.0.0 warning: missing warning: incorrect
pnpm optional:^1.0.0 warning: incorrect warning: incorrect
npm optional:^1.0.0 warning: missing warning: missing
yarn l ^1.0.0 warning: missing ok
pnpm l ^1.0.0 warning: incorrect warning: incorrect
npm l ^1.0.0 warning: missing ok

(replace the l with a pipe; it broke the formatting otherwise)

I guess that | something might have the best forward compatibility, but I find it confusing nonetheless.

@rwjblue

This comment has been minimized.

Copy link

commented Nov 2, 2018

Thank you very much @arcanis your breakdown with actual tests was very helpful, sorry for jumping to conclusions RE: using optional: would break in NPM (I'm likely still smarting RE: link: / file: differences between the two 😝).

@ljharb

This comment has been minimized.

Copy link

commented Nov 2, 2018

Instead of syntax, why not a new top level optionalPeerDependencies key? That would both not interfere with npm, and not cause new warnings to show up in npm ls - which many packages run in CI, which means the optional syntax would indeed break CI for non-yarn users.

@arcanis

This comment has been minimized.

Copy link
Member Author

commented Nov 2, 2018

@ljharb cf the rfc. Basically, the biggest issue is that it would actually break the installs for older package managers. Also cf this comment where I explain a case where it would break:

#105 (comment)

@ljharb

This comment has been minimized.

Copy link

commented Nov 2, 2018

I guess I'm confused. If you decide to treat peerDependencies as "required" - all older yarns, and all npms, would continue to treat those as such (with the current state of npm ls warning, or nothing warning, etc). By adding optionalPeerDependencies, all older tools would ignore it - whereas if you add a nonstandard value in the peerDependencies object, it will definitely break all current use cases.

What am I missing?

@iarna

This comment has been minimized.

Copy link

commented Nov 20, 2018

@ljharb I'm suggesting we would still warn on wrong version. Just not on "missing entirely", which tends to happen with things like discord.js that have a bunch of peer deps on modules that it can use, optionally, but does not require.

(The heart of the problem with peer deps is that these two disparate use cases are wrapped up in a single thing. The discord.js use case is far from uncommon.)

@arcanis

This comment has been minimized.

Copy link
Member Author

commented Nov 20, 2018

I don't have super strong feelings regarding whether the default should be "optional" or not 🤔

The cautious side in me would probably slightly go towards making it an option in order to preserve the existing behavior (hence defaulting on "warn if missing"), but both are acceptable to me.

@arcanis

This comment has been minimized.

Copy link
Member Author

commented Nov 22, 2018

It seems we have a reasonable consensus, so the PR will land tomorrow unless you have objections 🙂

@iarna Does npm have an Intent to Support, like pnpm?

@ljharb

This comment has been minimized.

Copy link

commented Nov 22, 2018

Given that today is a US holiday and many people are traveling, perhaps it would be prudent to wait until Monday, so more folks have the chance to weigh in?

@arcanis

This comment has been minimized.

Copy link
Member Author

commented Nov 22, 2018

Oh indeed - sorry, good holidays everyone! 🎉

@arcanis

This comment has been minimized.

Copy link
Member Author

commented Dec 3, 2018

peerDependenciesMeta has been merged into master (yarnpkg/yarn@011a634)

@arcanis arcanis merged commit 7f8f0a1 into yarnpkg:master Dec 17, 2018

@arcanis

This comment has been minimized.

Copy link
Member Author

commented Dec 17, 2018

I think we've reached a consensus, so I've merged the RFC (I forgot to update it to reflect the changes we've discussed, so I've committed it separately: 835c327). Will be part of the soon-to-be-released 1.13! 🙂

@arcanis

This comment has been minimized.

Copy link
Member Author

commented Dec 19, 2018

Released 🎉

@dcleao

This comment has been minimized.

Copy link

commented Dec 21, 2018

Can you guys add support for the following two scenarios that I need?

"peerDependenciesMeta": {
  "@my/runtime": {
     "env": "prod"
  },
  "@my/compiler": {
     "env": "dev", 
     "autoInstall": true
   }
}

Try imagining babel, above.

I think you're on the good track here!

@arcanis

This comment has been minimized.

Copy link
Member Author

commented Dec 21, 2018

I'm not too convinced by this use case, but maybe. Feel free to write a formal rfc to list pros and cons 🙂

@zkochan

This comment has been minimized.

Copy link

commented Jan 5, 2019

I wonder if we should support a reacher syntax. A package may support hundreds of plugins. For pnpm, it would be important to know about these relations. For instance:

"peerDependencies": {
  "foo-plugin-*": "*"
},
"metaPeerDependencies": {
  "foo-plugin-*": {
    "optional": true
  }
}
@arcanis

This comment has been minimized.

Copy link
Member Author

commented Jan 5, 2019

Hm I understand the benefits (would be useful for things like babel and its plugins), but at the same time I think it'd be for the best if we could keep the system simple.

Wouldn't this use case be solved through userland btw? Something like this:

app/index.js

const eslint = require('eslint');
eslint.findPlugins(__dirname);

eslint/index.js

exports.findPlugins = p => {
    const pkgJson = require(path.join(p, `package.json`));
    const peerDependencies = Object.keys(pkgJson.dependencies).filter(name => {
        return name.test(/eslint-plugin-*/);
    }).map(name => {
        return require(name, {paths: [p]});
    });
};
@ljharb

This comment has been minimized.

Copy link

commented Jan 5, 2019

Using any form of "one to many" in dependency lists seems like it would vastly inflate the complexity of determining a dependency graph, including that the packages eligible to be included in the glob could change at any time based on network activity (as opposed to the current situation, where basically only versions could change at any time)

@zkochan

This comment has been minimized.

Copy link

commented Jan 5, 2019

Using any form of "one to many" in dependency lists seems like it would vastly inflate the complexity of determining a dependency graph, including that the packages eligible to be included in the glob could change at any time based on network activity

That's because Yarn/npm create a flat node_modules, right? That randomness cannot happen with pnpm's nested node_modules.

I like the solution proposed by @arcanis.

@ljharb

This comment has been minimized.

Copy link

commented Jan 6, 2019

No, it has nothing to do with node_modules on disk - i mean that you couldn’t look at a package.json and get a list of the top-level dependencies needed. There’s no randomness about yarn/npm that doesn’t exist with pnpm and everything else, when someone newly publishes a package that matches an existing glob.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.