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

Add gradle backward-compatibility to Version +3 #96

Closed
SaeedZhiany opened this issue Aug 11, 2019 · 57 comments
Closed

Add gradle backward-compatibility to Version +3 #96

SaeedZhiany opened this issue Aug 11, 2019 · 57 comments
Labels
bug Something isn't working

Comments

@SaeedZhiany
Copy link
Contributor

Although Jetifier Tool doing pretty much of works, but it does not change gradle files in reverse mode. so we can not use v3.x with RN pre v0.60 (like 0.59.10). as @mikehardy said, backward-compatible support is needed to work v3.x with RN pre v0.60. He also suggests a way to do that that can be inspired to add the backward-compatibility for this library

@proyecto26 is there any chance to have this soon, please?

@jdnichollsc
Copy link
Member

Hi @SaeedZhiany, sorry but that solution doesn't work in our case because we're using imports of AndroidX as you can see here https://github.com/proyecto26/react-native-inappbrowser/blob/master/android/src/main/java/com/proyecto26/inappbrowser/RNInAppBrowser.java#L12
But let me know if you find other solution for this 👍
Thanks!

@jdnichollsc
Copy link
Member

@mikehardy
Copy link

The proposed change is forwards (AndroidX) and backwards (old support libraries) compatible. I don't see a reason it wouldn't work, it's in use in multiple projects. You'd just use a different default for your support libraries (1.x.x depending on which androidx library you are referring to) vs the 28.x.x since you've already converted.

@mikehardy
Copy link

Note, as a maintainer myself I'm not saying you should do that, it's your library, just saying as the jetifier maintainer that it would work if you wanted it to

@jdnichollsc
Copy link
Member

Honestly I don't understand how to support both from the Java code because we can't have conditional imports, can you share me any example please? Otherwise, @SaeedZhiany any pull request is welcome, thanks for your help guys! 👍

@mikehardy
Copy link

There is an example linked above, which was taken from my module maintainers section on the jetifer repo. The insight is that you can have conditional imports, implemented via string substitution. The mechanism is to look at the support library version and if it's in the "androidx range" (all 1.0 right now, but safe to say below 20 or so for sure) then you use an androidx library name as your dependency name string, if it is above 20, you use a support library name. There is nothing else to it

You can see another example in react-native-device-info - another lib I maintain: https://github.com/react-native-community/react-native-device-info/blob/master/android/build.gradle

That one also hasn't officially converted, so it's defaults are the 28.0.0 support versions, not the androidx versions, but all you'd do is use the 1.0.x versions for AndroidX and it should work, if you adopted the style

Again not saying you should do it, just putting out the how so more people understand the mechanism. A very workable hack, basically. Cheers

@jdnichollsc
Copy link
Member

ohh I mean I know how to do that from the build.gradle, like this other plugin I have for {N} here
but I don't know how to handle that here

@mikehardy
Copy link

Oh interesting - I didn't think about doing a full if conditional in the build.gradle. Neat.

As for the import you shouldn't have to do anything - that's what jetifier -r will do for you (or, for your library consumers). It'll translate that back to the support version of the class, then if the build.gradle has the right conditionality it all works

@jdnichollsc
Copy link
Member

Ohh amazing 😳, so let me do that change

@mikehardy
Copy link

🤞 😄

@jdnichollsc
Copy link
Member

@mikehardy haha are you agree with this change? 9bcaf78#diff-7ae5a9093507568eabbf35c3b0665732

@jdnichollsc
Copy link
Member

@mikehardy thanks for your comments, It's really appreciated!
Guys, I have this other proposal https://github.com/proyecto26/react-native-inappbrowser/blob/develop/android/build.gradle#L48 by using a new flag called androidXVersion 😄
Let me know what you think 🙏

@SaeedZhiany
Copy link
Contributor Author

Thank you very much guys for paying attention to this issue. 👍

@jdnichollsc
I don't migrate to RN 0.60, So I don't know if the extra property supportLibVersion in RN pre 0.60 is changed to any other name like androidXVersion or not.
just saying according to what @mikehardy did in this line in his PR, it seems you should use supportLibVersion name for extracting the proper version number as a string. then split it by \\. and use the first section of it to decide for using the support library and androidX.

actually, you shouldn't suppose that there is a separated extra property for androidX version.

@mikehardy, Am I saying right?

@jdnichollsc
Copy link
Member

jdnichollsc commented Aug 13, 2019

yeah, but I prefer adding a new extra property androidXVersion instead of reusing an old property likesupportLibVersion/supportVersion (for com.android.support dependencies), also extracting the proper version number of that old property looks like a temporary workaround because in the future it would stop working when AndroidX dependencies reach a version 20... so what do you think?

You only need to have this configuration:
android/build.gradle

buildscript {
    ext {
        buildToolsVersion = "28.0.3"
        minSdkVersion = 16
        compileSdkVersion = 28
        targetSdkVersion = 28
        supportLibVersion = "28.0.0" // To use Android support
        androidXVersion = "1.x" // To use androidX by default, otherwise you can remove this
    }

@SaeedZhiany In the meantime you can use the previous version 2.0.4 :)

@SaeedZhiany
Copy link
Contributor Author

@jdnichollsc
Yeah I know this a temporary workaround and probably it's no needed in near future, but I think it's better to not propagating this logic into the main project, just for one reason again, this is a temporary solution.

It's possible that another library maintainer would have a different name (like androidxVersion with small x) then the project developers must have two different names for the same concept. (I know this is unlikely to happen but let's avoid it in this library at least.)

finally, as @mikehardy said, this is your library and what your decision is, I'll respect that.

Thank you mate

@jdnichollsc
Copy link
Member

What do you think @mikehardy? 😅

@mikehardy
Copy link

I know @matt-oakes also did not like the heuristic method (and I understand that), for me, I don't mind having things in my ext{} block if it allows me control - I do prefer explicit control vs inferred things normally, so it seems fine to me. Most people won't need it even, but if it's needed you've got it

@SaeedZhiany
Copy link
Contributor Author

@mikehardy
@jdnichollsc

I checkout new release version of RN 0.60.5 right now. android gradle is still using supportLibVersion as extra property. @mikehardy is there any chance to replace it in the future with a more suitable name like androidXVersion or just simply add it as a new one besides the other one?

If you guys agree with me lets start a discussion about it in react-native repository.

@mikehardy
Copy link

You may start a discussion there if you like, but it's important to think of those gradle ext{} block params as programming interfaces, once they are out, people use them (they are used in so many places already...) so any change like that is a breaking change. I personally have no desire to hash out and implement a breaking change there so I'm not motivated and won't participate (honesty is more important than false promise right? I hope? :-) ).

@SaeedZhiany
Copy link
Contributor Author

SaeedZhiany commented Aug 14, 2019

I know what you are saying but you should also consider that using androidx packages is at the start of its way in react-native project and it's worthy to think about a general solution, else another nightmare(!!!) is going to happen just like what happened for android gradle version used in libraries (I have used a few libraries that using completely different gradle plugin version and I had some sort of problems in building my project and finally I had to write a script that rewrites the setting for the libraries, I don't like such messy patches). I think that having an earlier break change is much better than ignoring this problem.

@SaeedZhiany
Copy link
Contributor Author

Unfortiontly, No one help me to make a convention on how using the main project's gradle plugin version in libraries.

I am afraid of such a situation happening for androidx package versions.

@mikehardy
Copy link

I like the idea you propose there, but it seems like you and Dulmandakh weren't communicating effectively - and either way that didn't seem conclusively resolved. I commented on the linked issue. I don't disagree on breaking now versus later, but you can imagine as a person with a real job after doing the jetifier I'm a bit fatigued with it all and need a break to focus on the real job now that jetifier is at least working and RN60 is out there. I just can't spearhead anything new at the moment

@SaeedZhiany
Copy link
Contributor Author

I'm still waiting for a response from Dulmandakh in that PR :)
I'm using react-native in a heavily under developing project at a company and I've tired about managing versions in libraries after every RN upgrade or adding a new library in my project. it takes me lots of time to get everything works again in such situations.

@matt-oakes
Copy link

Having anything like androidXVersion set at the project level is not the correct solution for one reason: AndroidX is a set of dependencies, not a single one, and each of them has its own version number which follows semantic versioning rules. This is different than the support library which had a single version number for all the dependencies. (More details in the 2nd bullet point here: https://developer.android.com/jetpack/androidx/)

For example, you can't guarantee that there will be a 1.0.2 version of every dependency, even if you want to use 1.0.2 of the androidx.core dependency.

For this reason we need to stop trying to use these project level variables and instead just use the Gradle dependency resolution system correctly.

If you don't care about the version you can just declare the dependency without a version listed. It will then use the correct version of the library though dependency resolution.

You can also get fancy and use the "rich version decoration" syntax to define your minimum version requirements, but also allow higher versions if that's what the developer (or another library) has asked for.

You should read more about declaring dependencies in Gradle here:

https://docs.gradle.org/current/userguide/declaring_dependencies.html#sub:declaring_dependency_rich_version

As I said, having a single androidXVersion property is not going to work. If you want to do that, you'll need to make a specific one for each AndroidX library.

@mikehardy
Copy link

From what I see (and have PRd myself) the react-native module world is going down the path of specific AndroidX version specifiers, and it is not "trying to use these" project level variables it is using them ;-). It may not be pretty, but it does work.

If we don't specify a version at the module level, builds are not reproducible (I had a '+' dependency in react-native-device-info and thus my module brought the AndroidX breaking change to people's doorsteps that would not have otherwise been affected, and who did nothing differently except attempt a build on identical source from one day to the next. So that's a non-starter for me.

Rich dependency stuff sounds interesting and I wondered why I hadn't seen it before - because it's gradle 5.x and didn't exist until recently it seems? That might be the solution. Except...imagine you have a library that converted to AndroidX - you're going to specify an AndroidX dependency with specific AndroidX ranges, even though with jetify -r you will work fine on previous support library 28.x versions. So, that doesn't seem to work in all cases that people are actually deploying now.

So...I'm still at version specifiers myself. My ext block is not small, but it's manageable.

@SaeedZhiany
Copy link
Contributor Author

AndroidX is a set of dependencies, not a single one, and each of them has its own version number which follows semantic versioning rules. This is different than the support library which had a single version number for all the dependencies

I know that, but I'm thinking to define all of their version in a separated file and somehow import them in project-level gradle that libraries can read them. just I don't know is it possible or not and (if yes how we can achieve that)

@SaeedZhiany
Copy link
Contributor Author

SaeedZhiany commented Aug 15, 2019

I completely agree with @mikehardy. I believe we should not stop specifying the versions just because of increasing their count

@SaeedZhiany
Copy link
Contributor Author

I like to start a discussion on react-native repository, but usually, a maintainer closes it without giving a good response to me. so is there any official community to bring up this discussion?

@matt-oakes
Copy link

@SaeedZhiany This would be the best place for a discussion:

https://github.com/react-native-community/discussions-and-proposals

@jdnichollsc
Copy link
Member

jdnichollsc commented Aug 15, 2019

hey guys, I'm busy right now but any pull request is alway welcome in the meantime! 👍

@jdnichollsc
Copy link
Member

jdnichollsc commented Aug 15, 2019

having a single androidXVersion property is not going to work

@matt-oakes I don't understand that, because I can see a lot of projects by using only supportLibVersion property and also com.android.support is a set of dependencies 🤔

@mikehardy
Copy link

The AndroidX dependencies will have to be broken out - for instance, the 'v4 compat' library has a different version than 'media' and 'core' https://github.com/invertase/react-native-firebase/pull/2476/files

@matt-oakes
Copy link

@jdnichollsc From the link to the AndroidX page in my post:

Unlike the Support Library, AndroidX packages are separately maintained and updated. The androidx packages use strict Semantic Versioning starting with version 1.0.0. You can update AndroidX libraries in your project independently.

Therefore, each package has its own version number and it's completely valid to have 1.0.0 of one and 1.1.4of another. A single androidXVersion variable cannot do that so shouldn't be the convention we use.

@jdnichollsc
Copy link
Member

jdnichollsc commented Aug 15, 2019

😅 Yeah, it looks complicated, but what about starting using something like this?
android/build.gradle

buildscript {
    ext {
        ...
        androidXVersion = "1.+"
    }

But yeah, you're absolutely right, we need to be able to specify each version separately

@mikehardy
Copy link

mikehardy commented Aug 15, 2019

maybe that's the start of using the gradle rich version description stuff, that could be in the project-level gradle, I just don't see how it could be in module-level gradle without making it impossible to forward or reverse jetify (since that process takes libraries into completely different ranges) -

edit: but, 1.x could quickly be wrong when 2.x comes out. I think they really must be separate. they are separate dependencies in androidx and should be treated as such

@jdnichollsc
Copy link
Member

@jdnichollsc
Copy link
Member

jdnichollsc commented Aug 15, 2019

So we can have a configuration like this from our projects:
android/build.gradle

buildscript {
    ext {
        buildToolsVersion = "28.0.3"
        minSdkVersion = 16
        compileSdkVersion = 28
        targetSdkVersion = 28
        supportLibVersion = "28.0.0"
        androidXVersion = "1.x" // Default version for AndroidX dependencies
        androidXAnnotation = "1.1.0"
        androidXBrowser = "1.0.0"
       // Put here other androidX dependencies
    }

@mikehardy
Copy link

the proposal looks interesting - the only thing I don't like is there is still a 28.0.0 supportLibVersion in your ext block while we have these 1.x.x androidx versions, once you go androidx it looks weird to have any any 28.x anything

@jdnichollsc
Copy link
Member

ohh I pasted the current configuration we have when we create a new RN project using the command line: react-native init AwesomeProject

@jdnichollsc jdnichollsc added the bug Something isn't working label Aug 15, 2019
@jdnichollsc
Copy link
Member

@SaeedZhiany are you agree with that PR?

@SaeedZhiany
Copy link
Contributor Author

@jdnichollsc
I like that PR and it's similar to the one we've discussed so far. but for more consistency, it's worthy to mention in the Change.md that users must either have supportLibVersion or (androidX, androidXAnnotation, and etc). they should not have supportLibVersion and androidX... extra property together in their project gradle.
you should add few statements in README.md file like this

make sure your ext block is similar to one of below sections according to the version of react-native you are using:

**For RN pre 0.60.0**
Put 'supportLibVersion' extra property into ext block and make sure you have not any AndroidX related versioning 

ext {
    supportLibVersion = '28.0.0'
}

For RN + 0.60.0
remove 'supportLibVersion' and Put androidXAnnotation and androidXBrowser into ext block

ext {
    -supportLibVersion = '28.0.0'
    +androidXAnnotation = "1.1.0"
    +androidXBrowser = "1.0.0"
}

About this line I'm not sure if gradle system understands 1.x, do you mean something like 1.+? however, it also causes the android builds to be nondeterministic, Remember we want to have precise control over the version of packages. you can keep the logic of using a default version in your library gradle file. if users want to specify the versions, they should accept to having more extra properties in their project-level gradle. AndroidX packages might have break changes in future just like other libraries and it's not safe to having unknown versioning usage in build time. So I think forcing to use an exact version would the best and deterministic build process.

@jdnichollsc
Copy link
Member

Ohh you're right and I fixed that, thanks mate for your help too! Let me know, but having a default version looks good to me, I'm trying to be flexible :)

@SaeedZhiany
Copy link
Contributor Author

SaeedZhiany commented Aug 16, 2019

Thank you too,

I'm ok with this line and it's very helpful
I'm just saying remove this line and these lines

@SaeedZhiany
Copy link
Contributor Author

But you may want to wait, maybe in the discussion, someone has a better idea than mine.
Just I hope the discussion wraps up with a good result.

@jdnichollsc
Copy link
Member

But yeah, I don't have any problem by removing the androidXVersion property, and we need to update the README with that info 🙏

@matt-oakes
Copy link

@SaeedZhiany I still think having dynamic versions in the library and letting Gradle resolve them as needed is the best solution. I get that non-determinism is an issue, but why not use the same tactic as Yarn and NPM and enable locking? Documentation for Gradle dependency locking is here:

https://docs.gradle.org/current/userguide/dependency_locking.html

@SaeedZhiany
Copy link
Contributor Author

SaeedZhiany commented Aug 16, 2019

I don't know anything about that, I must read the article.
Are you mean that for every library, the maintainer should mention which androidx packages his library needs, and the user should add them in project gradle file? something like npm peer dependencies?

@SaeedZhiany
Copy link
Contributor Author

And please let's continue this conversion at the discussion I made to involve more people.

@jdnichollsc
Copy link
Member

Perfect, thanks guys for all your help, check the last version 3.0.1 with the new installation steps https://github.com/proyecto26/react-native-inappbrowser#mostly-automatic-installation and let's continue the discussion from the RN community repo 👍

@SaeedZhiany
Copy link
Contributor Author

Thank you very much @jdnichollsc
I just want to remind you to update the Change.md file as well if it's necessary

@jdnichollsc
Copy link
Member

Ohh yes, I updated the README.md 👍 Check my little tweet if you want to share https://twitter.com/jdnichollsc/status/1162451046544879616

@SaeedZhiany
Copy link
Contributor Author

Yeah, I saw that changes in the README.md file 👍 . I meant to update CHANGELOG.md file (I wrote it badly), so people can keep track of changes history

@jdnichollsc
Copy link
Member

yeah too, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

4 participants