Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Android lib: update to build.gradle #56

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

rozPierog
Copy link

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
Copy link
Collaborator

artald commented Jun 26, 2018

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
Copy link
Author

rozPierog commented Jun 26, 2018

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
Copy link
Author

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

@artald
Copy link
Collaborator

artald commented Jun 28, 2018

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
Copy link
Contributor

radex commented Jul 25, 2018

@rozPierog ⬆️

@rozPierog
Copy link
Author

@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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants