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

[Feature] RFC: Peer dependencies w/ defaults #1001

Closed
arcanis opened this issue Feb 26, 2020 · 15 comments
Closed

[Feature] RFC: Peer dependencies w/ defaults #1001

arcanis opened this issue Feb 26, 2020 · 15 comments
Labels
enhancement New feature or request rfc This issue is an RFC.

Comments

@arcanis
Copy link
Member

arcanis commented Feb 26, 2020

Describe the user story

Various packages want to improve the user experience by including most of the packages that would typically be expected from the top-level (think next or create-react-app, for example, which don't want their users to have to list webpack in their dependencies).

The problem then becomes: what happens to the top-level packages that happen to have peer dependencies on the packages embed within the source package? For example, @zeit/next-css is a package that the end user has to list at the top level, in their own dependencies, but it also has a peer dependency on webpack (through mini-css-extract-plugin).

In most package managers, if the user doesn't list webpack in their dependencies, the webpack from next will likely be hoisted to the top-level, so mini-css-extract-plugin will likely be able to access it. However, this is incorrect: since the user doesn't provide any version of webpack themselves, there is no safe way to satisfy the peer dependency.

More than just a theoretical issue, it may cause practical issues: if the user has two workspaces, with one depending on Gatsby and another on Next (or, more generally, if multiple workspaces use different versions of Webpack in any way), there's no telling which version of webpack will be hoisted - and thus mini-css-extract-plugin may end up using a different version than the Webpack server used by next.

At the same time, end users can't list webpack in their dependencies either, because it will likely resolve to a different version than the one used by next (even if the ranges are assumed to resolve to the same version if they overlap, the end user still has to keep the ranges in sync, and that's likely to break at some point).

Describe the solution you'd like

I believe this problem is solvable if we introduce a concept of "Peer dependencies with defaults" - in line with the "Optional peer dependencies" we introduced a year ago. Peer dependencies with default would be defined by using the existing fields:

{
  "dependencies": {
    "webpack": "^4"
  },
  "peerDependencies": {
    "webpack": "^4"
  }
}

So for example, next would have not only a regular dependency on webpack, but also a peer dependency. This pattern, currently invalid because without any semantic meaning, would be defined as such:

A package X listing a package Y as both peer dependency and regular dependency will be guaranteed to access the version of Y provided by its parent if available, or the one listed in its dependencies otherwise.

Going back to the initial story, end users would be able to add webpack to their dependencies when using @zeit/next-css, thus fulfilling the peer dependency. Additionally, because next would list a default peer dependency, it would be guaranteed to use the version provided by its parent, but without requiring it to be provided.

Describe the drawbacks of your solution

This proposal doesn't have obvious drawbacks - not only does it merely give a semantic meaning to a pattern that didn't have any, it's also completely forward compatible: older versions of our package managers will downgrade gracefully by installing the regular dependency and ignoring the peer dependency (without printing any warning).

Describe alternatives you've considered

We could try to provide a way for next to directly share its dependencies with its siblings, but I'm worried this would be a very drastic change that would be much more controversial. It would require specific syntax, wouldn't be forward compatible, and would be technically challenging to get right, if even possible (it would break the assumption that the dependency tree only goes in one direction).

Npm has a proposal to make peer dependencies installed automatically if not provided. Regardless of my general doubts about this approach, I don't think this would solve the issue described here, as in this case webpack would be resolved independently on the next and @zeit/next-css branches. Even if the package manager was able to ensure they end up resolving to the same version, it wouldn't necessarily be the same instance as another version of webpack could end up being hoisted to the top level (especially in the multi-workspace setup I described earlier). This isn't a problem for the proposal here, because the user will explicitly "pick" the version they want to make available to both next and @zeit/next-css, avoiding any hoisting conflict.

@arcanis arcanis added the enhancement New feature or request label Feb 26, 2020
@arcanis
Copy link
Member Author

arcanis commented Feb 26, 2020

Pinging @zkochan (pnpm), @isaacs (npm).

Also pinging @timneutkens since you're the example I've picked 😄

@zkochan
Copy link
Contributor

zkochan commented Feb 27, 2020

So why adding webpack as a dependency isn't enough? Why should it also be a peer dependency?

The problem then becomes: what happens to the top-level packages that happen to have peer dependencies on the packages embed within the source package?

As far as I remember from the docs of npm, peer dependencies are only resolved from dependencies of parent packages. next is not a parent package of @zeit/next-css so its webpack dependency should not be used to resolve the peer dependency of @zeit/next-css

@arcanis
Copy link
Member Author

arcanis commented Feb 27, 2020

So why adding webpack as a dependency isn't enough? Why should it also be a peer dependency?

If webpack is only a dependency of next, next-css isn't guaranteed to access the same version if it calls require('webpack'), even if it lists it as a peer dependency. For example:

. -> my-pkg -> [email protected]
  -> next -> [email protected]
  -> next-css ~> webpack

Even with hoisting, there's something guaranteeing that [email protected] will get hoisted rather than, say, [email protected]. If you replace the webpack peer dependency by a regular dependency:

. -> my-pkg -> [email protected]
  -> next -> [email protected]
  -> next-css -> [email protected]

Then, again, there's no way for next and next-css to be sure that:

  1. they use the same version. It may be true, but maybe not (for example if next-css got added in the dependencies months after next, the package manager may choose to resolve its deps to more modern versions).

  2. they use the same instance (again because of this hoisting problem - maybe [email protected] is the one that will end up being hoisted, thus preventing next and next-css to share the same instance without symlink magic. This is important, because a few packages (for example I remember graphql, react-dom, etc) keep singletons.

As far as I remember from the docs of npm, peer dependencies are only resolved from dependencies of parent packages

Yep exactly - the problem is that some library authors have this ambivalent need where they want their projects to be battery included (so they ship their whole dependency tree), but also want to allow external plugins (which will need to work with the embed dependencies).

This proposal aims to preserve the "works out of the box" workflow that they want while still giving users the ability to take back control to fulfil the requirements of the plugins they use. It also has other interesting applications, allowing to "override" dependencies if the tool allows it (similar to the resolutions field Yarn already supports). For example, I could keep using the same Next release with a more modern Webpack just by upgrading it inside my own dependencies.

@zkochan
Copy link
Contributor

zkochan commented Feb 27, 2020

But this doesn't solve the singleton problem. It only solves it for some cases. When the version ranges overlap. So I am not sure why this new complexity is needed if it doesn't even solve the problem in 100% of cases. Maybe something completely new is needed. Peer dependencies are already super complex.

@arcanis
Copy link
Member Author

arcanis commented Feb 27, 2020

But this doesn't solve the singleton problem. It only solves it for some cases. When the version ranges overlap.

No, for example (-> means regular dependency, ~> means peer dependency, and +> means peer dependency with default):

. -> A -> X@^1.1.0
  -> B -> X@^1.0.0

In this setup (which is what we currently have), A and B have no guarantee to use the same instance of X. In order to do that, they'd have to use peer dependencies:

. -> A ~> X
  -> B ~> X
  -> X@^1.0.0

However, this is problematic when you only use A and not B, because you're still expected to fulfil X yourself:

. -> A ~> X
  -> X@^1.0.0

With defaulted peer dependencies, we would be able to describe that A can access a version if the user doesn't list them:

. -> A +> X(@^1.0.0)

And when we add back B, the user can then explicitly add X to its dependencies so that both A and B are ensured to access the exact same instance:

. -> A +> X(@^1.0.0, ignored)
  -> B ~> X
  -> X@^1.0.0

Maybe something completely new is needed. Peer dependencies are already super complex.

Maybe. I'm concerned it would take a very long time to get agreement and adoption for any new field - especially given that there are other ones that could follow, such as typeDependencies or similar. It would end up a much bigger proposal than the initial scope, and I wouldn't have the bandwidth to tract it.

@isaacs
Copy link

isaacs commented Feb 27, 2020

Some questions:

(1) What happens if the ranges don't match? Ie, if you did this:

{
  "dependencies": {
    "webpack": "4.1.x"
  },
  "peerDependencies": {
    "webpack": "4.x"
  }
}

or even:

{
  "dependencies": {
    "webpack": "3.x"
  },
  "peerDependencies": {
    "webpack": "4.x"
  }
}

(2) I'm not sure I follow how automatically including peer deps won't solve this problem. Couldn't mini-css-extract-plugin list webpack as a peer dep, and solve this whole problem in that scenario? And likewise, if next wanted to ensure that a specific version of wepack was available to all its peers and children, it could list it as a peer dep as well.

Of course, that only works if peer deps are installed by the resolver. npm has working code slated for inclusion in v7 to do this already (with support for --omit=peer if you want them excluded), and I'm not fully understanding where the problem would arise.

(3) It'd be really helpful to outline in more explicit detail what trees would be generated today in a single specific instance: (a) with peer deps (not installed, but warning on conflicts), (b) with regular deps, (c) with auto-installed peer deps, and (d) with this proposal, just so that we can more clearly see what it offers and the scenarios in which the resulting tree is different, and in which ways.

(4) Since we already have a peerDependenciesMeta, and if we say that (1) is an error condition, then it strikes me that it might be simpler to do "peerDependenciesMeta": {"webpack": {default: true}} or something. (If (1) is not an error condition, then let's dig into what it means for those specs to be mismatched.)

(5) I'd like to dig into the status quo behavior of yarn, berry, pnpm, npm v6, and npm v7, when faced with this type of metadata, where a dep appears in both deps and peerDeps, and the ramifications of what that behavior will mean when faced with the situations where defaulted peer deps would be desirable.

npm v7 will ignore the peer dep, since arborist loads prod deps before peer deps (somewhat arbitrarily -- this could be changed easily), and ignores any dep when it already has an edge by that name. (It's quite common, for legacy reasons, to have a dep in both optionalDependencies and dependencies, and in this case, the dep is semantically an optional dep, and not a production dep. So the order isn't entirely arbitrary, but I don't know of any reason why prod deps should override peer deps, or vice versa.)

The advantage of using peerDependenciesMeta rather than listing the same dep in both places, is that the dependency would be ignored and treated as a plain old peerDep by any package managers that don't know about this new semantic. (Assuming that is an advantage, of course; it may be that treating it as a production dep is a better fallback.)

Since there won't be any major changes coming to either yarn or npm v6, and they'll both be with us for some time, it is important to consider the fallback cases if we provide a reason for people to start publishing packages with this structure.

That's all I've got for now, hope it's helpful :)

@arcanis
Copy link
Member Author

arcanis commented Feb 28, 2020

(1) What happens if the ranges don't match? Ie, if you did this:

The best action would imo be to emit a warning at publish time that the package is likely malformed. On the consumer side however those instructions would have a well-defined behavior (if no dep is provided, use 3.x, but if one is provided it has to match 4.x. Weird, but well-defined).

We could make it lead to an install-time warning, but I'm not sure it would be a good idea - end users wouldn't be able to act upon it anyway (increasing the warning fatigue for little reason), and since the behavior can be well-defined it isn't a soundness problem - just a probable mistake by the package author.

Couldn't mini-css-extract-plugin list webpack as a peer dep, and solve this whole problem in that scenario?

It's already the case, because that doesn't help because the final dependency tree ends up like this:

. -> [...]
  -> next -> webpack
  -> next-css -> mini-css-extract-plugin ~> webpack
              ~> webpack

It's impossible for mini-css-extract-plugin to know whether the webpack instance it will access will be the same one as the one used by next. Whether it'll be true or not will depend on the packages in the [...] set, and how webpack will be hoisted.

(3) It'd be really helpful to outline in more explicit detail what trees would be generated today in a single specific instance: (a) with peer deps (not installed, but warning on conflicts), (b) with regular deps, (c) with auto-installed peer deps, and (d) with this proposal, just so that we can more clearly see what it offers and the scenarios in which the resulting tree is different, and in which ways.

Requirements:

  1. next-css is able to access webpack
  2. next-css and next get the exact same instance of webpack
  3. The end user doesn't have to depend on webpack if they don't use next-css

With peer deps: It doesn't satisfy the point 3: the end-user will have to specify webpack in their dependency even if we remove next-css from the equation.

. -> next ~> webpack
  -> next-css ~> webpack
  -> webpack

With regular deps: It doesn't satisfy the point 2: next and next-css may require different instances of webpack (or even worse, different versions) depending on how the hoisting works. For example Yarn 1.x prefers hoisting packages that are depended upon by a lot of packages, so a webpack with more dependents could be hoisted to the top-level and prevent deduping the next / next-css webpack instances.

. -> next -> webpack
  -> next-css -> webpack

With auto-installed peer deps: Thinking more about it it would actually work since the package manager would be assumed to prevent webpack from being hoisted to the top-level. I still stand by the general objections I raised against the design auto-installed peer dependencies, though.

Interestingly, I think peer dependencies w/ default are quite similar to "auto-installed" peer dependencies but with one key difference: they require an explicit opt-in (in the form of the dual definition), meaning that there is very little chance to break existing packages. Package authors will be able to start using them at their own pace, only when they feel comfortable doing so.

. -> next ~> webpack
  -> next-css ~> webpack

With this proposal: All three requirements are satisfied: next-css and next will access the same webpack instance, and if I remove next-css I can remove webpack as well and next will simply get back to use its default definition.

. -> next +> webpack
  -> next-css ~> webpack
  -> webpack

And likewise, if next wanted to ensure that a specific version of wepack was available to all its peers and children, it could list it as a peer dep as well.

That would require the end user to always list webpack in their dependencies. Such frameworks want to avoid having to require new users to run a large command line such as yarn add next webpack, so it wouldn't satisfy their requirements. It's very close though, hence why I think it could be interesting to find a way to semantically unify both concerns (hence defaulted peer dependencies).

I'd like to dig into the status quo behavior of yarn, berry, pnpm, npm v6, and npm v7, when faced with this type of metadata, where a dep appears in both deps and peerDeps, and the ramifications of what that behavior will mean when faced with the situations where defaulted peer deps would be desirable.

At the moment I think most package managers (I've checked Yarn and npm, not yet pnpm) will just ignore the peer dependency (which is a good thing; it makes the feature degrade gracefully). I don't think the behavior has been defined anywhere, hence why I think there's an opportunity to make possible something that wasn't before with little cost syntax-wise.

The advantage of using peerDependenciesMeta rather than listing the same dep in both places, is that the dependency would be ignored and treated as a plain old peerDep by any package managers that don't know about this new semantic. (Assuming that is an advantage, of course; it may be that treating it as a production dep is a better fallback.)

In the case of Next, for example, it would be harmful for webpack to be listed exclusively in the peerDependencies field, as all their users using older package managers would have to explicitly add webpack to their dependencies in order to satisfy the new peer dependency (which wasn't one before). On the other hand, making the resolution favor prod dependencies has this graceful degradation characteristic I mentioned (older package managers will simply behave as if webpack wasn't provided, which isn't correct but is likely correct enough to prevent most backward incompatible changes).

@isaacs
Copy link

isaacs commented Feb 28, 2020

Thanks for going through all that. I have a much clearer understanding of what you're getting at now.

With auto-installed peer deps: Thinking more about it it would actually work since the package manager would be assumed to prevent webpack from being hoisted to the top-level.

Yeah, that's what was confusing me, I think, because just listing webpack as a peer dep of the things that want to use it (iiuc, next, next-css, and mini-css-extract-plugin, in this example) would result in a single instance of webpack being installed at the same level as next (and causing a resolution collision if that could not be done), without the user explicitly having to know about webpack or depend on it.

they require an explicit opt-in (in the form of the dual definition), meaning that there is very little chance to break existing packages. Package authors will be able to start using them at their own pace, only when they feel comfortable doing so.

What if we added a peerDependenciesMeta field to tell the installer to automatically include the peer dep or not? It seems like that would satisfy a lot of what you're going for here. We could then also interpret the dual-definition in the same way, if users want to do it that way to get the benefits of graceful degradation (ie, treating it as an auto-installed prod dep rather than a peer dep), but we could treat that as a somewhat clumsy kludge and favor the peerDepsMeta approach.

{
  "peerDependencies": {
    "webpack": "4.x"
  },
  "peerDependenciesMeta": {
    "webpack": {
      "autoinstall": true
    }
  }
}

The added advantage of a meta field here is that yarn/berry could default to false, and npm v7 could default to true, but we could respect the user wishes if they explicitly opt one way or the other.

@arcanis
Copy link
Member Author

arcanis commented Feb 28, 2020

What if we added a peerDependenciesMeta field to tell the installer to automatically include the peer dep or not?

I don't think it would be possible because of the peer dependencies format. Peer dependencies are necessarily semver versions (matched against whatever their parents provide), and have no way to express the location where the package should be extracted from.

For example, given the following: what would you put to describe that the peer dependency is satisfied by versions 15.0 to 16.0, but by default it uses the version 16.0 from Github?

{
  "peerDependencies": {
    "myPkg": "???"
  },
  "peerDependenciesMeta": {
    "myPkg": {
      "autoinstall": true
    }
  }
}

The closest would be to make autoinstall accept a string:

{
  "peerDependencies": {
    "myPkg": "15.x || 16.x"
  },
  "peerDependenciesMeta": {
    "myPkg": {
      "autoinstall": "arcanis/myPkg#16.0.0"
    }
  }
}

But in this case it's basically just a different way to write the dual definition syntax:

{
  "dependencies": {
    "myPkg": "arcanis/myPkg#16.0.0"
  },
  "peerDependencies": {
    "myPkg": "15.x || 16.x"
  }
}

Except that in the first case older package managers would default to not install myPkg, whereas in the second they would at least end up using the version from GitHub (I know you mentioned supporting both to solve that).

The added advantage of a meta field here is that yarn/berry could default to false, and npm v7 could default to true

The problem with this approach is that I see only two possibilities:

  • Package authors make due diligence to make sure their packages are portable, and will use the dual definition (which would potentially be understood by all tools)

  • Or they rely on the default behaviour of the tool they're developing with, and in the case of npm it'd lead them to not use the dual definitions. When their package is consumed by Yarn / pnpm users, the assumption won't hold and the package won't be portable.

We both have opinionated user bases, and I'm not looking forward to the first instance of "but npm is doing it so it's a bug if you don't do it too" that we would receive 🙂 We would be able to explain that using the dual definition is the way to go, but by which point I guess they'll wonder why you have the install-by-default if they still need to add the dual definitions be portable.

@arcanis
Copy link
Member Author

arcanis commented Mar 12, 2020

Any more opinion?

@timneutkens
Copy link

timneutkens commented Mar 12, 2020

It's unlikely that we'll adopt this for Next.js as webpack is an implementation detail and could be swapped out eventually. The mentioned plugins are deprecated btw as they're now built into Next.js.

@arcanis
Copy link
Member Author

arcanis commented Mar 12, 2020

That's a good concern 🤔

I think it could still work, because people upgrading next-css to a version that use something else than Webpack would get unmet peer dependency warnings at install time. Combined with the optional peer dependencies that will help drive down the number of warnings, maybe that would be enough?

@SampsonCrowley
Copy link

SampsonCrowley commented Sep 28, 2021

I still don't understand why we can't just install peerdependencies if and only if no dependency is given by the user to guarantee a flat graph. peer dependencies are required by end packages, why arent the ranges there good enough on their own if a package isn't provided by the user themselves?

Always install unmet peer dependencies by default if the package isn't provided whatsoever, but do not hoist whatsoever

@SampsonCrowley
Copy link

if a package isn't dependent on a peer dependency it wouldn't be listing it. it would just be checking *if * it's defined

@merceyz
Copy link
Member

merceyz commented Jul 16, 2022

Closing as the described solution was implemented in #628 and #2915

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request rfc This issue is an RFC.
Projects
None yet
Development

No branches or pull requests

7 participants