search

wix-incubator / react-native-keyboard-input

Use your own custom input component instead of the system keyboard
MIT License
819 stars 150 forks source link

Android lib: update to build.gradle #56

Open rozPierog opened 6 years ago

rozPierog commented 6 years ago

Previous gradle configuration had hard coded version numbers which sometimes clashed with project versions. Well not anymore. Now library uses rootProject values when its possible with fallback on default values. I also removed ProGuard because the library is open source and obscuring code sounds like a bad idea in this case because it doesn't give use any benefits I can think of, and may cause problems on some configurations.

artald commented 6 years ago

Hi @rozPierog, thanks for the contribution.

I'm not sure that this solves the real issue. While it will work for this specific library, a typical RN project uses multiple dependencies, and naturally you will not be able to apply this strategy to all of them...

If you need specific versions, why not use resolutionStrategy in your app's build.gradle and force your project versions there? you can do it once for all current and future dependencies.

rozPierog commented 6 years ago

Hi @artald great to hear from you, here is my take on this.

Well first of all it solves issue with 'compile' is obsolete and has been replaced with 'implementation'. It will be removed at the end of 2018 which is, or at least soon will be, a big deal.

resolutionStrategy will still work with approach presented in this PR, or at least I'm pretty sure it will, but this also gives an option to do it differently: via ext {} in build.gradle which is in my opinion more clean way of doing this.

Also keeping buildTools and targetSDK updated benefits everyone using this library because of shorter compile times and better compatibility.

rozPierog commented 6 years ago

Well @artald, my approach is now part of react-native. Does this change your mind on this PR?

artald commented 6 years ago

If it's a widely adopted pattern I don't mind merging it since it does no harm (none that I can't think of at least).

I'm just wondering - what are you planning to do with other libraries that haven't adopted this approach yet? PRing everyone can't scale, and on RN projects lots of dependencies are typically being used and added all the time. That's why I thought that resolutionStrategy is better and lets you handle everything in once place. Let me know what you think..

radex commented 6 years ago

@rozPierog ⬆️

rozPierog commented 6 years ago

@artald Sorry for late reply it got buried in my inbox, and thanks @radex for reminding mi of this.

PRing everyone can't scale

I know, but making changes in few libraries can be a cause of snowball effect so that in the future its going to be preferred way of writing versions. There is no need to force all libraries to this "new" approach, change can happen gradually, not all at once.

resolutionStrategy is good, but it isn't elegant, and imho its poorly documented. Approach seen in this PR as well as in react-native repo is understandable on a glance. Moreover you can still use resolutionStrategy with this if that is your preferred way