fix: List optional peer dependencies

[eps1lon]

react-dom is required from the main bundle (not really optional I suppose) while react-native is required when using it with react-native and their batched updates.

Additional context:

• optional peer dependencies (implemented in [email protected], [email protected] but not yet bundled with node and pnpm@?

I'm currently not using this feature myself until node bundles [email protected]. Just creating this PR now so that we can discuss any concerns you have.

[eps1lon]

Never used react-native. Just use the note from the readme:

As of React Native 0.18, React Redux 5.x should work with React Native.

[netlify]

Deploy preview for react-redux-docs ready!

Built with commit 4fde80f

https://deploy-preview-1390--react-redux-docs.netlify.com

[markerikson]

This has been brought up several times before. The current behavior is quirky but correct, and there's no need to change.

[timdorr]

Actually, this is correct, but only once npm >= 6.11 is in greater circulation. It won't show the error (the reason for this extension to the package.json format), but only for those versions.

Let's leave this one open until npm >= 6.11 has been bundled with Node for a while.

[timdorr]

BTW, npm releases are delayed 2 weeks before being bundled into node. nodejs/node#29273 is just about ready to go and nodejs/node#29430 still has 12 days to go. But likely the next 12.x release will have it. I'm not sure what the LTS schedule is.

[eps1lon]

The current behavior is quirky but correct, and there's no need to change.

I'm not sure how you define "correct" in this context but the dependency trees created from it are incorrect. All I could find about "react dom peer" in the issues is an old statement from dan (#130 (comment)) which

1. no longer applies with peerDependenciesMeta
2. is in todays ecosystem a bit of an overstatement since a missing peer dependency warning is only a warning for a reason. Nobody is forcing you to install them and from my experience most packages seem to be ok with having them anyway.

[markerikson]

@eps1lon : previous PRs:

#1261
#1282
#1371
#1366

And I don't care about "most packages". I care that this package would end up forcing everyone to see at least one warning - RN for RD users, and RD for RN users.

[timdorr]

If they are marked optional with peerDependenciesMeta, they won't warn at all.

[arcanis]

And I don't care about "most packages". I care that this package would end up forcing everyone to see at least one warning - RN for RD users, and RD for RN users.

This is incorrect. With the optional peer dependencies, peer dep warnings will only be printed if:

• you use an outdated npm version or,
• you use a recentish npm version AND npm hasn't fixed their server AND only on initial installs (subsequent ones don't print the warning even now).

Anyway, I've just suggested what I think is an reasonable compromise: if you don't list peer dependencies, please at least put the peerDependenciesMeta field. Yarn will understand that it implies a peer dependencies, even if unlisted, and everyone will be happy.

[markerikson]

@arcanis : my statement about wanting to avoid unnecessary warnings still stands, but no, this is the first time I've heard of "optional peer dependencies", so I'm not familiar with the distinction here in how they behave.

[timdorr]

I'll add the optional stuff to the package.json now. It doesn't hurt anything to add it. And if the package managers adopt it as working without the peerDeps entry, that's great.

[arcanis]

@markerikson Fwiw documentation is here:
https://next.yarnpkg.com/configuration/manifest#peerDependenciesMeta

Note that it's already implemented in both Yarn 1 (initial rfc) and pnpm. Npm has merged my PR too, but they have to do a fix on their server to return the field on the initial installs (they use a special endpoint which removes most fields from the manifest, including the peerDependenciesMeta one).

The distinction is that a package manager will simply discard the warning if the peer dependency is marked optional, but will be able to leverage the information to properly hoist packages (unlisted peer dependencies aren't having a visual effect - they have an actual technical impact regarding how the package managers will setup the tree, hence why not listing them is dangerous for your users).

The change I've just landed makes it possible to list peerDependenciesMeta without listing peerDependencies. That will at least fix Yarn, and npm will benefit from it as well once they'll have fixed their servers (since it'll help them layout the packages correctly).

@timdorr Awesome, thanks! 🙏

[markerikson]

Looks like this was resolved by 388d9e4 .