feat(android): add support for AGP 8 #48
Conversation
|
@LinusU Any plan to merge that? |
There was a problem hiding this comment.
Hey there 👋 - this is a good fix, it's what I'm using locally in a patch-package as I test android gradle plugin 8 compatibility
I'm also the maintainer of react-native-netinfo, react-native-device-info, react-native-firebase, and a pile of other ones, they all need this and work well with exactly this patch
Cheers
|
worth noting react-native 0.73 - which will require this - is on rc4 now and will release very soon |
|
I feel like a broken record at this point, but you'll need to remove the |
|
No you don't actually @felipecsl - if you remove package from AndroidManifest, then you will be on-purpose breaking folks that are still on android gradle plugin 6 and below, which is to say everyone on react-native 0.66 and below. That may be something you are willing to do, but it is not necessary. If you leave the package attribute in AndroidManifest you get a warning. Not an error, things still build. To me anyway, the choice is obvious, leave it in, let it warn. |
|
@mikehardy that doesn’t match what I saw, unfortunately. The package attribute does cause a build error instead of a warning. I can pull up a log message for you if you’re still unsure, but I’m 100% certain about it. |
|
I'm 100% certain the opposite. Literally building just fine with this patch package as is, same in all packages. I maintain a large number of repositories including react-native-firebase with this solution published same style and hundreds of thousands of downloads No problems You'll need to provide evidence |
|
@mikehardy oh I think I know what might be happening. My project is on RN 72 and AGP 8, I’m on my phone now but I believe RN 73 added a backport for libraries that still have the package attribute to make it still work, while RN 72 might not have that. Does that match your understanding? |
|
No. The version skew of my react-native-firebase (and react-native-device-info, and react-native-netinfo) library consumers certainly includes every version you imagine You need to provide evidence to back your assertion. I have repos with the changes in form proposed here published and download counts on versions that contain the change style proposed here to back my assertion |
|
@mikehardy you're right, I apologize. For some reason I can't reproduce the issue right now, at least not with |
|
No reason to apologize, I like a good technical argument. My 100% statement was a bit bold as yes I do have strong evidence but software always manages to surprise us doesn't it? If there is some combination where it does not work I will be very interested to know the details since I have pushed these same PRs out in lots of repositories and support tons of users. If there is some edge case I'm bound to have a user hit it... |
|
Appreciate the work you’re doing sir 👍🏽 |
|
Sorry for the late reply on this one! I'm ready to merge, just wanted to double check one final thing: as I've read the discussion here, this would not be a breaking change. Is my understanding of this correct? Thank you 🙏 |
|
That’s my understanding as well |
|
Looked at |
|
Released as 🚢 1.10.0 / 2023-11-20 |
|
Fantastic! You released sooner than I can reply but anyway I was the one that did the netinfo release and I've released same for quite a few other heavily used packages, so far totally non-breaking. Cheers |
Change to support AGP 8 as mentioned here: react-native-community/discussions-and-proposals#671
This does not remove package attribute from AndroidManifest to not lose compatibility with AGP < 8 (React Native < 0.71 versions).
I don't think it's worth maintaining logic to remove that attribute contitionally since it will only cause a warning to users on AGP 8 and above.