okonet/react-scroll-sync

Move `prop-types` to regular `dependencies`

JalilArfaoui opened this issue · 10 comments

prop-types  have been moved out of react for the very reason that it’s not mandatory in a React project …

If this module depends on it, it should be a regular dependency and not a peerDependency.

Feel free to submit a PR!

I don't want any module forces me to use its version of prop-types or react instead of mine.
Please let prop-types stay in peerDependencies instead of dependencies.

Again, please submit a PR with a fix and I’ll merge it. Adding more comments here of what you don’t want won’t solve the problem. 🤷🏻‍♂️

@okonet sorry to be not clear enough, I just wanted to point out I like the current behavior and I don't want any changes to it. So I've asked to not accept such a PR :)

Ah okay I see your point. I believe react could be a peer dependency but prop-types I’m not sure tbh. Can someone do some pros / cons comparison for every approach?

After some thought I think you both have valid points and the right solution is to move it to dependencies but allow any version to be used. I believe this way if your project using a specific one, it will satisfy the range here and yours project version will be used. Otherwise the latest version will be used. Thoughts?

I'll be happy both with your last idea and with status quo.

Hi folks, sorry for the delay …

I think we should have a minimal version requirement to guarantee it works correctly : https://www.npmjs.com/package/prop-types#how-to-depend-on-this-package

I’d suggest ^15.5.7 as recommended by the docs

Is that OK @okonet @vadzim ?

Yeah sticking to the recommended policy from the package itself is good for me.

Here you go !

#39