search

LinusU / react-native-get-random-values

A small implementation of `getRandomValues` for React Native
MIT License
354 stars 48 forks source link

feat(android): add support for AGP 8 #48

Closed AndreiCalazans closed 1 year ago

AndreiCalazans commented 1 year ago

Change to support AGP 8 as mentioned here: https://github.com/react-native-community/discussions-and-proposals/issues/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.

FelipeSSantos1 commented 1 year ago

@LinusU Any plan to merge that?

mikehardy commented 1 year ago

worth noting react-native 0.73 - which will require this - is on rc4 now and will release very soon

felipecsl commented 1 year ago

I feel like a broken record at this point, but you'll need to remove the package attribute from AndroidManifest.xml as well

mikehardy commented 1 year ago

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. If you remove the package attribute in AndroidManifest you no longer get a warning on android gradle plugin 7+ but you will have broken android-gradle-plugin 6 and below

To me anyway, the choice is obvious, leave it in, let it warn.

felipecsl commented 1 year ago

@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 realize it’s a breaking change, but if you bump the major version and make it clear in the change log it shouldn’t be a problem

mikehardy commented 1 year ago

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

felipecsl commented 1 year ago

@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?

mikehardy commented 1 year ago

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

felipecsl commented 1 year ago

@mikehardy you're right, I apologize. For some reason I can't reproduce the issue right now, at least not with react-native-get-random-values. I'll try to isolate it to figure out a consistent repro and will update here if so

mikehardy commented 1 year ago

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...

felipecsl commented 1 year ago

Appreciate the work you’re doing sir 👍🏽

LinusU commented 1 year ago

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 🙏

felipecsl commented 1 year ago

That’s my understanding as well

LinusU commented 1 year ago

Looked at react-native-netinfo and it seems like they released this as a non-breaking change, so let's go 🚀

LinusU commented 1 year ago

Released as 🚢 1.10.0 / 2023-11-20

mikehardy commented 1 year ago

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