adding react-dom to addon peer dependencies

[danielduan]

Issue: There could be different versions of running in Storybook because some addons don't rely on peer dependencies to get the correct version number.

It could be related to this:
#986

What I did

Updated some package.json

How to test

Please pull the branch and help me test this in your own storybook to see if there are any issues.

[codecov]

Codecov Report

Merging #1179 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #1179   +/-   ##
=======================================
  Coverage   14.07%   14.07%           
=======================================
  Files         200      200           
  Lines        4497     4497           
  Branches      506      501    -5     
=======================================
  Hits          633      633           
- Misses       3436     3440    +4     
+ Partials      428      424    -4

Impacted Files	Coverage Δ
lib/ui/src/modules/ui/components/layout/usplit.js	38.7% <0%> (ø)	⬆️
...codemod/src/transforms/update-organisation-name.js	40.62% <0%> (ø)	⬆️
addons/knobs/src/components/types/Object.js	5.81% <0%> (ø)	⬆️
lib/ui/src/modules/ui/containers/left_panel.js	20% <0%> (ø)	⬆️
lib/ui/src/modules/ui/configs/init_panels.js	25% <0%> (ø)	⬆️
lib/ui/src/modules/shortcuts/actions/shortcuts.js	6.25% <0%> (ø)	⬆️
addons/info/src/components/Node.js	0% <0%> (ø)	⬆️
.../src/manager/containers/CommentsPanel/dataStore.js	34.78% <0%> (ø)	⬆️
addons/info/src/components/markdown/code.js	0% <0%> (ø)	⬆️
addons/knobs/src/components/types/Select.js	7.93% <0%> (ø)	⬆️
... and 12 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 67d6af3...578ade4. Read the comment docs.

[tmeasday]

I don't understand this @danielduan -- if we aren't removing it from other dependencies, what exactly will it do? Also, do all those packages use react-test-renderer?

[danielduan]

react-test-renderer has been specified by those packages already. I'm not sure if there are actual implications of running two different version in the same project so whether we need it as a peer dependency could be up for debate.

As for adding react and react-dom to the peer dependencies, it will help us keep the react and related package versions throughout the project consistent when the user downloads it.

Say someone's project is on [email protected] and [email protected] and the main @storybook/react would use [email protected] and [email protected]. Since the addons have their own package.json, the peer dependency [email protected] could be used with the dependency [email protected] and cause a version mismatch. Also, there would be two react-dom versions running alongside each other.

[tmeasday]

react-test-renderer has been specified by those packages already. I'm not sure if there are actual implications of running two different version in the same project so whether we need it as a peer dependency could be up for debate.

I think if it's in devDependencies it's fine to leave it out of peerDependencies, if it is only used for test purposes.

As for adding react and react-dom to the peer dependencies, it will help us keep the react and related package versions throughout the project consistent when the user downloads it.

Can we talk through the exact scenario this helps with? The thing is that peer deps in npm are just pretty much broken (especially when you npm link), and I'm not sure adding a react: "*" peer dep actually achieves anything. Maybe it's worth doing as I'll discuss in a sec, but I suspect it won't help with the problem of multiple versions of react.

So as a illustrative example: The actions addon currently lists react in the devDependencies but nowhere else. This is bad because the addon imports react and theoretically the app it is in may not include react (although I guess this never happens). Also as a secondary badness there could be a version mis-match between react versions (i.e the app could be using a version of react the addon doesn't like).

1. When you npm install inside an app including the @storybook/addon-actions package, it (A) doesn't influence the version of react your app gets (if any) and (B) doesn't get its own version inside node_modules (this is the problem you are concerned about), because npm won't look at devDependencies in this case.

2. When you npm install in a checked out version of the package it does get a copy of react, and when you npm link it into an app, you'll now have 2 reacts, which is why npm link is broken, and @ndelangen created the test-cra app which uses file:// references. [<- an aside]. (Also a linked package can't require the react from the app anyway, even if you cheated and had no dep on react in the package. Good times).

If we add the peerDependencies lines, then case 1 will change in the sense that now you'll get an error if your app doesn't use some version of react. This is good I suppose, but won't actually help in any real world scenarios (people are using react anyway, right?) . Restricting the version would be more interesting, if we are concerned about that. (In practice I suspect most users ignore peer dep warnings anyway).

Case 2 will be unaffected by the peer dep thing, sadly.

I'm not quite sure what my point is, probably just ranting about npm. I think I am saying (1) let's not add the react-test-renderer lines. (2) we can add the react and react-dom lines for correctness, but they won't do anything (3) if you were seeing an issue with multiple react versions, we should investigate further because I don't think this change will solve that problem.

[danielduan]

Thanks for the really detailed analysis, really appreciate it. I'm not an expert in how npm resolves modules and this definitely helped my understanding.

I updated the branch to just include react-dom for formality and consistency's sake.

[ndelangen]

But do these package actually need react-dom? They don't import or require it anywhere?

[danielduan]

Yeah, seems like they're extraneous. Should have checked to see if they actually used it first. Closing.