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

Support optional peerDependencies #6487

Closed
edmorley opened this issue Oct 4, 2018 · 13 comments
Closed

Support optional peerDependencies #6487

edmorley opened this issue Oct 4, 2018 · 13 comments
Assignees

Comments

@edmorley
Copy link
Contributor

edmorley commented Oct 4, 2018

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

What is the current behavior?
With Yarn's new Plug and Play feature, if a package uses another package, it must include that other package either in dependencies or peerDependencies otherwise there will be a runtime resolution failure.

However some projects intentionally omit the dependency if it's for an optional feature. For example in:
waysact/webpack-subresource-integrity#90

At the moment such projects have a choice between:

  • not adding the optional peer dep, and Yarn PnP not working
  • adding the peer dep and making everyone install a potentially unnecessary dep to silence the "missing peer dependency" warnings during yarn install

What is the expected behavior?
To have a (backwards-compatible) way to declare a peer dependency as optional, which would allow the PnP resolver to still work, but if the optional peer dep package was not installed, there would not be a "missing peer dependency" warning shown during yarn install.

In waysact/webpack-subresource-integrity#90 (comment) this form was proposed:

{
  "peerDependencies": {
    "html-webpack-plugin": "optional:^x.y.z"
  }
}

And in waysact/webpack-subresource-integrity#90 (comment), this:

{
  "optionalPeerDependencies": {
    "html-webpack-plugin": "^x.y.z"
  }
}

Please mention your node.js, yarn and operating system version.
Yarn 1.12.0, all OSes.

CC @arcanis, @jscheid

@jscheid
Copy link

jscheid commented Oct 4, 2018

Thanks @edmorley for picking this up, it would be great to have a feature like this.

(Also, regardless of PnP, it would provide a way to express "I don't need this package, but when present it has to be version range XYZ").

I was actually thinking optionalPeerDependencies to just be a list of names, as in "optionalPeerDependencies": ["html-webpack-plugin"]. Admittedly it's potentially confusing that peerDependencies is an object and this an array, perhaps calling it optionalPeerDependencyPackages (or Names) would alleviate that?

Just thinking out loud, I don't really have the full picture of all the implications with regard to backward compatibility and compat with other package managers.

@arcanis
Copy link
Member

arcanis commented Oct 4, 2018

Regarding optional:, imo the best argument in favor of it is that it would work regardless of the package manager (worst case there would be a warning). It also has a slightly better story for composition (we could imagine stacking the tags: optional+dev:*). Downside is that it wouldn't allow more complex schemes with parameters, but we don't have any and I don't see us doing such drastic changes.

Regarding optionalPeerDependencies, it would work a bit by chance on package managers that wouldn't support it (just like when the dependency wasn't listed at all in the peerDependencies). Since even now npm still doesn't support link: dependencies, it seems important to keep that in mind. It also wouldn't compose well with other tags (optionalDevPeerDependencies when? :p).

Another option, maybe more radical, is to make the value type of peerDependencies accept an object with metadata. I'm not sure how well package managers (including Yarn's past releases) would support it though. Maybe they would just crash or ignore the peer dependency 😐

{
  "peerDependencies": {
    "html-webpack-plugin": {
      "range": "^x.y.z",
      "optional": true
    }
  }
}

@jscheid
Copy link

jscheid commented Oct 4, 2018

Regarding optionalPeerDependencies, it would work a bit by chance on package managers that wouldn't support it (just like when the dependency wasn't listed at all in the peerDependencies).

I'm not arguing against your approach at all, looks great. But just to clarify, what I was suggesting would be backward compatible not just by chance and would compose well. It would look something like this:

{
  "peerDependencies": {
    "html-webpack-plugin": "^x.y.z",
    "webpack": "^x.y.z"
  },
  "optionalPackages": ["html-webpack-plugin"]
}

(Edited to rename optionalPeers to optionalPackages)

@arcanis
Copy link
Member

arcanis commented Oct 4, 2018

Oh I see, sorry! Yep, would work indeed. optionalPeers would actually be a better name since we already have optionalDependencies (which are a weird design by themselves, and mainly used nowadays for fsevents).

@jscheid
Copy link

jscheid commented Oct 4, 2018

What about optional dev dependencies though?

Not to blow up the scope of this ticket, but since you mentioned composability - I remember wanting for that feature once.

@arcanis
Copy link
Member

arcanis commented Oct 10, 2018

Things like optional dev dependencies require a relatively huge investment since we would have to support a "better dependencies" field that would allow meta information to be passed to the package manager on top of the range (or doubling down to the antipattern that are fields such as optionalDevDependencies), so I think they're out of scope for this issue.

Considering the pros/cons, I think adding a syntax to the peerDependencies range is the less intrusive way to solve the problem described in this issue. So what would be the best syntax?:

  1. "react": "optional:^1.0.0"
  2. "react": "?^1.0.0"
  3. "react": "^1.0.0?"

Note that ? is an invalid character per semver, so it should be fine to use it.

@arcanis
Copy link
Member

arcanis commented Oct 10, 2018

Also, ping @iarna and @zkochan - feel free to share your thoughts 🙂

@Gudahtt
Copy link
Member

Gudahtt commented Oct 14, 2018

I was wondering why this problem hasn't come up with npm's tink as well. It looks like tink still looks in the node_modules folder for packages first, so I'm guessing that optional dependencies can be resolved from there even if they aren't listed in package.json anywhere.

So if we add a new field for optionalPeerDependencies (or allow it to be specified on an existing field in a way that doesn't break npm somehow), then library authors can safely adopt this new field to remain compatible with both npm and yarn, including PnP and tink. That sounds promising.

It would be nice to support other types of optional dependencies as well, especially given that these were possible prior to PnP but won't work with it. It is definitely a more difficult problem to solve, and probably outside the scope of this issue. But I think having a long-term goal in mind for other optional dependencies would be helpful in deciding which path to take on optional peer dependencies in the short term.

This discussion should be moved to an RFC. This change is definitely significant enough to warrant one.

@arcanis
Copy link
Member

arcanis commented Oct 26, 2018

I was wondering why this problem hasn't come up with npm's tink as well

Last time I heard about tink they weren't actually getting rid of the node_modules, just virtualizing it. It still exists, but not on the filesystem (basically doing the same thing than a FUSE filesystem would do, except that they trade the ability to run any tool for an increased portability).

It's really quite different from the PnP goals (for example they can't make the guarantees we give regarding how packages are instanciated or optimized, because they're still limited by the fs hierarchy). Compare us with pnpm rather than tink, they're much closer to PnP than tink.

It would be nice to support other types of optional dependencies as well, especially given that these were possible prior to PnP but won't work with it. It is definitely a more difficult problem to solve, and probably outside the scope of this issue. But I think having a long-term goal in mind for other optional dependencies would be helpful in deciding which path to take on optional peer dependencies in the short term.

Yep, it's much more complex - I prefer to first have a short-term plan for optional peer dependencies, and only then think about an overhaul of how dependencies are setup (which would take at least a year before being drafted, let alone implemented and deployed).

This discussion should be moved to an RFC. This change is definitely significant enough to warrant one.

Will submit an RFC early next week (based on the optional: range proposal), and an implementation soon after. I'd like to get this ready for the 1.13 🙂

@ccope
Copy link

ccope commented Feb 8, 2019

yarnpkg/rfcs#105 Has been merged and it is implemented in yarn 1.13. Can this be marked as closed now? (also, @arcanis should the RFC be moved to the implemented directory?)

@Venryx
Copy link

Venryx commented Jun 10, 2021

It looks like the latest versions of Yarn got rid of the optional: protocol, and are now using the peerDependenciesMeta.some-library.optional field?: https://yarnpkg.com/configuration/manifest#peerDependenciesMeta

@arcanis
Copy link
Member

arcanis commented Jun 10, 2021

It never got implemented as the optional: protocol, as we settled on the peerDependenciesMeta field in yarnpkg/rfcs#105

@Venryx
Copy link

Venryx commented Jun 10, 2021

Good. I like the peerDependenciesMeta approach better anyway. (leaves the protocol open for other potential uses, and the root field could be expanded to include other meta information)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants