React-fontawesome: Remove prop-types from peer dependencies

[cristianrgreco]

My project doesn't use prop-types, is there a reason why it's a peer dependency?

[robmadole]

This project does

[cristianrgreco]

@robmadole Yes OK.. but why is it a peer-dependency and not just a regular dependency?

I'm sure I've just misunderstood something, but to me PropTypes is optional, if FontAwesome needs it then it should be its dependency.

[robmadole]

@cristianrgreco you might have a specific version of PropTypes that you using in your project. If we listed it in dependencies it would install a competing version. It's better to have a peer dependency and let the user of this package decide which version to use.

[lookfirst]

Like @cristianrgreco, I also do not use PropTypes as I'm using TypeScript and don't need it in my project.

I came here wondering what was up and found this issue. This seems to be a wart in npm publishing imho as I should not be forced to depend on it if I'm not using it.

The (ugly) workaround to this issue for me is: yarn add --dev prop-types

This way, my project doesn't actually depend on it and yarn doesn't throw warnings.

[robmadole]

Ok @lookfirst perhaps I'm missing something then. Because _this_ project uses PropTypes:

https://github.com/FortAwesome/react-fontawesome/blob/master/index.es.js#L2

So are you suggesting that we remove this? Why is this an ugly workaround to install peer dependencies for a project that uses it?

[cristianrgreco]

@robmadole I think you just said it, if it's a dependency for fontawesome then it should be listed as such.

To the argument of 'the client may use a different version of a dependency'; can't the same be said of all dependencies? I would assume NPM raises some issue if the user installs an incompatible version.

[robmadole]

@cristianrgreco sure, you could say the same about any dependency. And one of the core benefits of the way that NPM handles dependency resolution is that your 3rd party libraries have guarantees that a compatible version of what they need will be available.

However with our fontawesome and fontawesome-svg-core libraries those behave like plugins. There needs to be a singleton (single instance) of the fontawesome.library that has icons added and retrieved from. If one copy of the library has the icons added to it and a complete separate instance is used for the lookup they will be useless (and we've seen this). Hence, that's where the peer dep comes in.

[lookfirst]

@robmadole

I'm just a clueless yarn user, but if _this project_ uses a dependency, then just list it as a dependency and therefore, you can just let yarn resolve all the things for us.

In this case, it is not only listed as a dependency but _also_ as a peerDependency, which means that not only does _this project_ depend on it, but you also expect _my project_ to as well. Even though _my project_ doesn't actually use it anywhere.

I don't know the absolute right solution here and it really isn't in my interest to argue about it. I found a workaround that is good enough for me.

Cheers.

[panjiesw]

Hi all,
I was also confused to why there is a warning indicating I need to install proper prop-types version while I don't use it in my app.

This confusion is common and happened before in several other projects. Here are some of the note I gather from those occasions:

From the README of prop-types package:

For apps, we recommend putting it in dependencies with a caret range. For example:
...
For libraries, we _also_ recommend leaving it in dependencies:
...

Also in facebook/prop-types#44, it's said:

I think the main difference is the user tends to know which version of React they need, but might not care about prop-types

Also from the same person in cssinjs/react-jss#164:

For PropTypes, the user doesn't have to use them so it makes less sense to force the version choice upon the user. Ideally just make it as relaxed as possible and rely on npm to hoist it. In the worst case it can get duplicated which isn't ideal but isn't harmful either.

I personally think it makes more sense to include it in dependencies. There is also a babel plugin that remove propTypes for production, which makes the inclusion of prop-types in peerDependencies less important

[robmadole]

So my objection to this is that there is a mechanism that is documented, well supported, and mature for handling this exact use case. I do understand the pragmatism of listing it in dependencies but it's a hack. Hacks cost time and have unintended consequences that degrade the quality of a project. Sometimes it's worth it, sometimes it's not.

I'd be happy to change our documentation to mention this to smooth out some of the rough corners of this. But I'm not convinced that this is an issue. I don't think it's unreasonable to expect someone who wishes to use this library to take 5 minutes and understand peer dependencies. Maybe I'm being grumpy or unreasonable (or un-empathetic).

[panjiesw]

I mean, inclusion in dependencies for this exact prop-types case is

• well documented in its readme https://github.com/facebook/prop-types#how-to-depend-on-this-package
• heavily encouraged by its main contributor Dan Abramov
• commonly practiced in almost all react components libraries. From full blown multi components libraries like ant-design, material-ui, etc, to single/small component libraries.

I've never encountered unmet peer dependency warning even though they all use prop-types package in their distribution, nor do I have experienced issues of working with different versions of prop-types be it in TypeScript or JavaScript.

prop-types is just used in development time, and should not be included in any app final bundle. My points are,

• If prop-types is included in dependencies, users who use prop-types will _most likely_ not encounter issues even if there are different/multiple prop-types versions in their node_modules, and also won't affect their production build (cause it shouldn't be included)
• If prop-types is in peerDependencies, users who don't use prop-types will encounter annoying warning of something they can't control, resulting in ugly hack like including prop-types in their deps just for the sake of removing the warning.

EDIT: also need to point this out if prop-types is moved from peerDependencies to dependencies

Currently prop-types major version is 15. With the way @fortawesome/react-fontawesome declare it as 15.x, and Users most likely using npm >= 3 or yarn, and if they declare their prop-types version using range notations, NPM or yarn will take care of deduplication by default (there won't be multiple prop-types in node_modules).

[lookfirst]

@robmadole Please re-open and fix, @panjiesw details things extremely well.

[robmadole]

Ok, I'll cry uncle :) I'll get this change made.

[robmadole]

Ok, dependency was moved in 0.0.19 and 0.1.0-10.

[lookfirst]

Thanks Rob. =) Node is a never ending battle of dependency brittleness and the more we can do to just reduce necessary dependencies, the better.

Just today, brew upgrade stuck me at Node v10. Straight up yarn install stopped working because of a commit a dude made 6 months ago to say engines <= 9 in a project that is 5 levels deep in my own projects dependencies.

[robmadole]

Yep, it's not an easy problem to solve 😃

[panjiesw]

Really appreaciate it @robmadole! @fortawesome/react-fontawesome make it really easy to include most awesome font package in React projects and this make it even a pleasant to use! :smile: