Make React a peer dependency?

[roblg]

I ran into a problem late last week where, after I upgraded my node.js + webpack (client-side) app to react 0.12, the flux-router-component NavLink started spitting out gobbledygook. If I copied that file into my source directory, and included it that way, everything worked (with warnings, because NavLink uses react syntax/features that have been deprecated in 0.12). Closer inspection of my JS rollup client-side shows that webpack is including two versions of React in my rollup - 0.12.0, which I include, and 0.11.1, which flux-router-component declares as a dependency. It seems likely that this is because of the way flux-router-component declares its dependency on react.

I think that declaring React a Peer Dependency will alleviate this issue. As I read it, npm will then install react for anyone who uses 'npm install' to install flux-router-component, and it will complain if the versions aren't compatible. I'm also fairly new to node + npm + webpack, so I could be totally off...

[lingyan]

Thanks @roblg!

We saw the same issue and thought about using peer dependency too, except for when we tried using peer dependency on a previous project (not related to React), we had seen issues. @mridgway @redonkulus, do you guys remember what the issues were exactly?

[redonkulus]

I don't recall the exact issues, they were always hard to debug. I did some research and seems like other projects are following a similar approach.

[brigand]

It makes little difference, the important part is declaring compatible React versions for each release, so people know when they can upgrade flux-router-component and when they can upgrade React.

npm doesn't do a great job of helping with this (there's no way to say 'give me flux-router-component latest compatible with 0.12.1')

[peleteiro]

+1

It's a good pratice for a react library to have it as peer dependency. It saves us a lot of time and bugs.

facebook/react#2402

[brigand]

To clarify my previous comment, it makes little difference whether you make it a peerDependency or just don't list it in the dependencies.

If you put in the readme a mapping of flux-router-component releases to react releases it'd be much more useful than a half-baked feature like peerDependencies.

react	flux-router-component
0.12.x	0.5.x
0.11.x	<0.5
<0.11	not compatible

^{numbers above made up for example purposes}

[lingyan]

@brigand Agree with peerDependencies. I can add a table like you proposed to the README. The source of truth is always in the package.json.

[lingyan]

@brigand Added, https://github.com/yahoo/flux-router-component/blob/master/README.md#compatible-react-versions

[peleteiro]

It makes a lot diference. flux-router-component cannot use a different React that my app uses, it should use the exactly same one. It makes it a peerDependency.

If you, like me, uses flux-router-component on the server AND client; You gonna have trouble if you update react and not reinstall flux-router-component. Like I just did today and lost 4 hours. It runs a different version on the server (the one npm installed just for flux-router-component) and on the client uses the new you installed for your project. Making it a peerDependency (with version boundaries and so) solves it.

It's a recommendation from React.js guys for libraries and every other lib/component that I'm aware of uses React this way.

Please, take in consideration this issue. It's not about peerDependencies being a good feature, but about being the best one available.

[lingyan]

@peleteiro I am digging more into the issues we might've had with peer dependencies in the past, to see if those were significant enough to stop this change. I hope they are not. Will get back to you soon.

[lingyan]

@peleteiro

Ok. We have seen issue with npm shrinkwrap, when there is a peer dependency declared (say react), but its parent module does not have it as dependency. Then the issue is npm install still installs react as an extraneous package (which is actually a problem). But then when you run npm shrinkwrap, it will fail on extraneous packages.

This might not be a big issue in our case, as very likely the app will include react as dependency. But then there will be more hairy issues when it comes to npm link and other things :( Check out this super-long debate npm issue about deprecating peerDependencies: npm/npm#5080 One of the comments in there (npm/npm#5080 (comment)) makes most sense to me. But not sure where that thread will eventually go and whether there will be any action item coming out of it.

There are suggestions about using dependencies and npm dedupe in that thread as well. But I am not 100% sure if that is the magic bullet either.

[peleteiro]

I updated my npm modules today and I started to get an error: Cannot read property 'firstChild' of undefined.

It was flux-router-component having it's own version of React again.

http://stackoverflow.com/questions/27153166/typeerror-when-using-react-cannot-read-property-firstchild-of-undefined

rm -rf node_modules/flux-router-component/node_modules/react will fix it.

[lingyan]

Urgh... that is frustrating. Lets bite the bullet, see what other pandora's box it will open :) I will file a PR.

[brigand]

I still think just leaving it out of the dependencies is the smartest move from a technical standpoint. There's no gain to putting it in peerDependencies or dependencies, other than correctness and readability of package.json.

When in doubt, don't break things imo.

[jedireza]

other than correctness and readability of package.json

I think that's important.

Also, even if we put it in peerDependencies it'll probably end up in devDependencies so during continuous integration/deployment (for tests), it'll get installed. peerDependencies are not installed via $ npm install.

[brigand]

Understood, I guess we'll have to agree to disagree and hope for the best.

[lingyan]

0.5.8 released for addressing this issue: https://github.com/yahoo/flux-router-component/releases/tag/v0.5.8

We had to add react also to devDependencies to get unit tests to pass with npm 1.x, because npm 1.x does not install peerDependencies, while npm 2.x does.