search

schiehll / react-alert

alerts for React
MIT License
608 stars 98 forks source link

Feature Request: React-transition-group be part of dependencies and not peer dependencies #118

Closed benjie91 closed 5 years ago

benjie91 commented 5 years ago

Is there any specific reason as to why react-transition-group is listed as peer dependencies? I am using a different version of the react-transition-group lib for my project and this will cause conflict with the lib.

schiehll commented 5 years ago

Hey @benjie91

You should update your react-transition-group, why would you want two different versions of the same dependency in your bundle?

That's why it's a peer dependency, so you can have only one version of it, same reason why react is a peer dependency.

benjie91 commented 5 years ago

In that case, your documentation state that it is v2.5.3 and up. So are you limiting users of your lib to this particular version range.

For newer projects who are using v4.1.1 of the react-transition-group, do you suggest them to include both version of dependencies which I am not in favor of as you mentioned.

React is a different story because you are building your lib/app with it. It is expected that everyone who use react for the view component to include it as part of their dependencies.

Likewise for prop-types, I am also strongly in favor it being in the dependencies. You can see facebook also recommending it that way. https://github.com/facebook/prop-types#how-to-depend-on-this-package

benjie91 commented 5 years ago

Another link for you to consider. https://github.com/cssinjs/react-jss/issues/164

You can also see Gearon's explanation on why it should be dependencies instead.

schiehll commented 5 years ago

No, if there's a newer version of react-transition-group that doesn't work with this lib, we should update it to make it work with the newer version.

But no, I will not make it a dependency because if every lib does that you would end up with a lot of duplicated code in your bundle, and I don't think that this is the right thing to do.

benjie91 commented 5 years ago

I think you can let yarn and npm to be the dependency manager as you had already use the carrot sign for the packages and thus there is minimal impact on duplicated code.

In the client side land, an old project especially enterprise application will not likely be able to use your lib because the risks of upgrading these dependencies such as react-transition-group might cause more harm than good. Likewise for new projects, if they are using a later version of react-transition-group, then they have to wait for the maintainer of this lib to upgrade before they can use it. Of course, there are other ways to resolve it (e.g. forking your project, maintaining two separate version in the package.json) but it is not elegant at all.

For example, react-bootstrap lib is already packaging react-transition-group as v4 and up (dependency) and your lib is forcing users to use ^2.5.3 and up (peerDependencies). Here there are already duplicated code but your lib is imposing more restriction on users by dictating what version they need to use. If I also need to use the react-transition-group but I specify it to be v4.1.1 (Yarn and npm would resolve it to the same dependency as the one used by reasct-bootstrap), I would be able to use react-bootstrap but not your lib due to breaking changes between v2 and v4.

schiehll commented 5 years ago

My concern is not about what you install in your node_modules, but what ends up in your final bundle, the one your users will have to download before using your app.

Again, I think this type of dependencies should be peer, because I don't want the final user to have to download multiple versions of a 4.4kb (gziped) dep, just because the developer didn't wanted to manage its dependencies. If the people building react-bootstrap and other libs think different, its ok, but as I've said before, if everyone do it like that your final user will have to download a ton of dup code.

By the way, I've just tested with the latest version of react-transition-group (4.1.1) and it worked, so feel free to use the latest one.

schiehll commented 5 years ago

But maybe I'm getting it wrong 🤔

If I tell rollup that it's external, even as a dependency it wouldn't be included in the bundle of the lib, so maybe there's a way to make it work even for UMD bundles.

schiehll commented 4 years ago

It was included in the v6 release, thanks for the suggestion!