Implements peerDependenciesMeta

[arcanis]

This diff implements the peerDependenciesMeta field specified in the following rfc.

What is it?

This field provides a way to package authors to disable the peer dependency warnings for packages that may be needed for some integrations, but only affect a subset of the users and aren't worth printing a warning to all.

Why implement it this way?

The rfc can be read for more details on why this syntax and not another; it's worth noting that both Yarn and pnpm (cc @zkochan) have been supporting this field for about half a year now.

Why is it important?

With npm still being roughly half the Javascript ecosystem, some maintainers are found omitting to declare their peer dependencies altogether just to avoid the peer dependency warning to their users. It generates invalid dependency trees that may work in the best cases, but break when an unrelated package is added to the dependency tree and changes the way the hoisting works.

Why is it broken not to list peer dependencies?

It's convoluted, click to expand 🙂

Proof by example; imagine the following setup:

• A depends on B@*, and has an unlisted peer dependency on X
• your app depends on A and X@1
• B@1 depends on nothing

In this situation, we get the following tree:

. - A - B@1
  \
    X@1

The package A is able to access X@1 (assuming a node_modules install), everything is ok. Now let's say that B gets a new release, B@2, which includes a dependency on X@2:

. - A - B@2 - X@2
  \
    X@1

But because of the hoisting, and because there is no conflict doing this, X will be moved from B to its parent folder. We'll now get the following:

. - A - B@2
  \   \ 
    X   X@2

Because of this hoisting, when A makes a require call to obtain X, it will no longer get the X@1 provided by its parent. Instead it will get X@2 provided by its child, breaking the original contract in a very subtle way.

If X had been listed as peer dependency of A, even optional, then the package manager would have been able to detect that hoisting X into A would have caused a conflict, and this problem would have been avoided.

Where are the tests?

This diff was manually tested by patching my local npm and trying it out. I also don't see how it could have any adverse effect, although I'm not familiar with the codebase.

Regarding automated tests, given that the fixtures are stored in a separate repository I figured you might want to integrate them yourself - although I can do it if you prefer. From a cursory glance, the relevant files to modify are:

• https://github.com/npm/cli/blob/latest/test/tap/peer-deps.js
  This is a trap, this file should not be modified 😃 It's just the test that matters.

• https://github.com/npm/npm-registry-mock/blob/master/fixtures/npm-test-peer-deps.json
  An optional unmet peer dependency should be added.

[isaacs]

This seems fine to me. In an ideal world, we would've maybe done dependenciesMeta for devDependencies, optionalDependencies, and bundleDependencies, or even supported something like dependencies:{<name>:{spec,optional}} but those ships sailed a long time ago. (The argument against is that it adds complexity, but it's a close call, imo.)

I think we can get this into 6.11. There'll probably be at least one or two more bug fix releases on 6.10 before then. If you feel motivated to send a PR to npm-registry-mock to add the optional peer dep fixture, that'd be great, but if not, I'll get to it when it's time to land.

This section of the code is all going away in v7, but I'll make sure to work the peerDependenciesMeta support into arborist. (In npm v7, peerDeps will be installed, and it'll enforce the constraint that they are not allowed nested under the package that peer depends on them, so hopefully we'll see fewer cases where they're unlisted due to the warnings.)

[arcanis]

Sounds good - I've opened a PR at npm/npm-registry-mock#34 🙂

we would've maybe done dependenciesMeta for devDependencies, optionalDependencies, and bundleDependencies

For the record, we also have a dependenciesMeta field that can be, for example, used to prevent a package from being built. The optionality is also one of those settings, but mostly for internal code consistency: we still support optionalDependencies (and we would encourage using it over dependenciesMeta[].optional), but under the hood it gets converted to the new syntax.

In npm v7, peerDeps will be installed, and it'll enforce the constraint that they are not allowed nested under the package that peer depends on them

Do you have an rfc / design document? It sounds like a significant change - many packages have what are essentially optional peer dependencies, and them being automatically installed may have adverse size or behavioural effects. Maybe it would make sense to instead make this opt-in by adding a peerDependenciesMeta[].auto flag?

[isaacs]

Implementation for npm v7: npm/arborist@5288b21

[zkochan]

I still see warnings when installing deps with optional peers (using npm v6.11).

[arcanis]

I quickly checked - it seems that the warning is now properly hidden but only on subsequent installs. I'm not sure why, but the initial install seems to follow a different codepath with regard to the peer dependency validation 🤔

[arcanis]

Further debug seem to indicate that tree.package doesn't contain all the fields during the initial install. Maybe the peerDependenciesMeta field needs to be explicitly forwarded when using the metadata from the registry?

[isaacs]

Ahhh, duh. I think I know what's happening here. The peerDependenciesMeta field needs to be added to the trimmed down manifest that the registry sends when fetching with the application/vnd.npm.install-v1+json accept header.

I think the cli is doing the right thing, and this is a server-side fix.

Do you have an example handy of a package with this field set, so I can point them to it?

[arcanis]

https://www.npmjs.com/package/ts-pnp has an optional peer dependency on typescript