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 androidx suppport. #691

Merged
merged 3 commits into from
Jul 24, 2019
Merged

Add androidx suppport. #691

merged 3 commits into from
Jul 24, 2019

Conversation

jgreen01su
Copy link
Contributor

- Replace `android.support.annotation.Nullable` with 
`androidx.annotation.Nullable` import.
- Thanks to @maieonbrix hard work finding this fix. 
(itinance#686 (comment))
@mikehardy
Copy link

Note that once you do this, you won't be able to provide bugfixes to people that can't convert (there will be a few I'm sure) unless you integrate the "bob" package which the community has patched to create a "reverse-jetified aar" for libraries, that legacy API consumers may use. Just some package.json lines and a bit of docs

@jgreen01su
Copy link
Contributor Author

I'd just roll a new version for androidx support. If users don't want androidx they'll have to use an older version.

We have to switch to androidx. See the link below.
react-native-community/discussions-and-proposals#129

@mikehardy
Copy link

I can certainly understand that stance as a maintainer, but the bob+jetifier "reverse-jetified aar" combo is so easy to generate, and might help so many as you continue to develop the library, I feel it's worth considering - enough to mention it again. But, not my library obviously :-). Cheers

@jgreen01su
Copy link
Contributor Author

@mikehardy You mean adding the following to gradle.properties?

android.useAndroidX=true
android.useJetifier=true

@mikehardy
Copy link

It is much more subtle than that. At the source level, the library converts (or does not convert) by doing what you mention in the pull request (or not doing it)

The problem is how people use the library. If I'm on a project stuck on react-native 0.57 (because of Expo or something) then react-native hasn't converted to AndroidX yet for me, so if you convert but react-native doesn't, what do I do? The answer is as I describe, at the library level you go ahead and convert the source to use androidx references, but you also integrate the "bob" npm package, and have it generate a reverse-jetified aar file. Then the project that can't convert can use that and everyone carries along.

Or what if I have to convert because I need an urgent bugfix to the underlying google firebase SDKs that just converted to AndroidX? Now it doesn't really matter what you do as a library, because there will always be some library that hasn't converted (or at least that will be the case for months). So I go ahead and convert my project to AndroidX then use npm i jetifier && npx jetify and everything is converted for me.

If that doesn't make sense, you can read all the background here react-native-community/discussions-and-proposals#129

@jgreen01su
Copy link
Contributor Author

I've read through that discussion and I understand what you mean a little better. But I still don't understand the steps or tools I need to use to properly add AndroidX support to this. I've seen bob a few places and jetify. As far as I can tell this package doesn't use AAR.

I've added the jetifier package, but it's not clear exactly what it does or how it does it. Do I need to wire it into the project somewhere? Does react-native know it's there? I expect that I at least need to link it.

Is it possible to get a simple / high-level explanation? @mikehardy

@mikehardy
Copy link

@jgreen01su it is a bit confusing right now as it is leading edge. Thanks for taking the time to implement it, I'm sure we can get it right :-)

The goal is for apps that are either support library, or androidx library, to be supported by one codebase.

So here is what I think needs to happen now that jetifier has a '-r' mode to reverse jetify

That should be it?

There is an alternative approach you can take at the same time, to help people that don't want to use jetifier in reverse mode themselves. You can generate an AAR at library build time via bob (which uses jetifier internally) and reverse-jetify it then publish that to npmjs along with your code. Then for people that did not want to run jetifier, they could alter their gradle dependency to use that reverse jetified AAR instead of the source

Does that make sense? If not let me know

@mikehardy
Copy link

Although looking at the change, it might be easiest to simply delete the reference to Nullable since it is just an annotation anyway? Then the library works both ways no change :-)

@maieonbrix
Copy link

maieonbrix commented Jun 21, 2019

I couldn't agree more with @mikehardy, I don't have enough experience in android nor in general to be at help here but if we can assure no breaking changes for both version that would benefit more people so I think it must be the way to go.

Although google has not dropped the previous one so there is a lot of users using it so it would be annoying for them (I didn't like updating my dependencies imports just to be able to use the latest features)

- This removes the dependency on `android.support` and `androidx`. 
Side-stepping the need to make a major change to the project to support 
both libraries.
@jgreen01su
Copy link
Contributor Author

Although looking at the change, it might be easiest to simply delete the reference to Nullable since it is just an annotation anyway? Then the library works both ways no change :-)

This actually did work! Thank you @mikehardy for pointing out this change! And for the explanation of jetify and reverse-jetify.

@crobinson42
Copy link

When will this be merged & published? I can confirm this works on a fresh install of react-native

@namvoeh
Copy link

namvoeh commented Jul 11, 2019

Can't wait to see this PR is merged

@gomdolkim
Copy link

can we make this PR merged?

@oleksandr-dziuban
Copy link

@itinance @gomdolkim Hello, can you merge please this PR to support AndroidX with react-native@0.60.0? It's really urgent please

@oleksandr-dziuban
Copy link

@itinance Hello, do we have any chance to merge this PR to have fill AndroidX compatibility with react-native@0.60 + ?
Please merge, market leaders are needed this thing :)

@oleksandr-dziuban
Copy link

@itinance can we merge this PR and publish v3.0.0?

@itinance
Copy link
Owner

Hey guys! Thanks for carrying and pushing :) I'm little confused if this should be handled as breaking change or not. Since @mikehardy wrote recently:

Although looking at the change, it might be easiest to simply delete the reference to Nullable since it is just an annotation anyway? Then the library works both ways no change :-)

Following this advice it would not be a breaking change, this we don't have to move forward to 3.0.0, isn't it?

I will merge now into master and make a new release as soon as we have clarified the versioning.

@itinance itinance merged commit 18c6d7c into itinance:master Jul 24, 2019
@oleksandr-dziuban
Copy link

Hey guys! Thanks for carrying and pushing :) I'm little confused if this should be handled as breaking change or not. Since @mikehardy wrote recently:

Although looking at the change, it might be easiest to simply delete the reference to Nullable since it is just an annotation anyway? Then the library works both ways no change :-)

Following this advice it would not be a breaking change, this we don't have to move forward to 3.0.0, isn't it?

I will merge now into master and make a new release as soon as we have clarified the versioning.

Thanks a lot!
I think it can be a minor version change, you are right, because we don't add androidx libs here. So it can be just a 2.14.0

@itinance
Copy link
Owner

v2.14.0 is out. Thanks everybody for contribution!

@mikehardy
Copy link

Yeah - minor change, the version choice was correct. 🚀

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

Successfully merging this pull request may close these issues.