search

oblador / react-native-vector-icons

Customizable Icons for React Native with support for image source and full styling.
https://oblador.github.io/react-native-vector-icons/
MIT License
17.31k stars 2.12k forks source link

fix(android): add buildFeatures.buildConfig for AGP8+ compatibility #1569

Open mikehardy opened 7 months ago

mikehardy commented 7 months ago

Summary

Upcoming react-native 0.73+ includes android gradle plugin 8+ which needs namespace in build.gradle but also needs buildFeatures.buildConfig enabled as well for modules that use it

It is not sufficient to enable this at the top-level app build.gradle, specific modules that use it (such as those that implement new architecture, it seems) must also enable it at the module level

This change was necessary and is in-use in my work app via patch-package as I work through android-gradle-plugin 8+ issues in prep for react-native 0.73 launching for everyone

This change is not backwards compatible but I notice the namespace addition here also is not. So this will not work on android gradle plugin < 7, but neither does the namespace addition. It will work with android gradle plugin 7 though

It is similar to changes I needed to do as react-native-firebase maintainer --> https://github.com/invertase/react-native-firebase/commit/b52d0ce6723c077190618641ce0f33ced9fd4090

Test Plan

What's required for testing (prerequisites)?

With apologies, you have to alter an app that integrates this module to use android gradle plugin 8, it's difficult to do that in repos I'm proposing these changes in because bumping to android gradle plugin 8 requries a large amount of transitive dependency changes in CI

I have integrated this in an app and tested it, and done similar work as maintainer of react-native-firebase, react-native-netinfo and react-native-device-info, and I'm now pushing these out to the repos

What are the steps to reproduce (after prerequisites)?

run the build for android

oblador commented 6 months ago

Thanks for your PR! However I'm not sure I fully understand/agree with the premise. For me it works fine in a fresh react-native init 0.73 project which I assumed is using AGP8? Also the project has AGP7 support by keeping the namespace in the xml, so if we don't have to I would very much like to keep BW support for now.

mikehardy commented 6 months ago

It is possible that in the react-native-gradle-plugin they added some auto-magic config for gradle to enable the buildConfig option on all auto-linked modules, there was some chatter about that, thus a 0.73 clean project as released may work now

I was testing with 0.72 and AGP8 in order to fuzzbust problems prior to 0.73 release during it's rc phase and noticed this.

At this point, the patch may only be necessary for whatever niche population both stays on rn 0.72 and updates to AGP8

I think there is something to be said for a module that uses a gradle feature (buildConfig) to correctly configure itself for that feature vs relying on react-native magic, but I recognize that's a stylistic opinion and no one needs to agree with it

In particular, you are the maintainer, so obviously feel free to disagree with me and discard the PR :-). I can obviously patch-package if it's something I need and if rn73 does something that makes it work with AGP8 without the buildConfig toggle on in the local build.gradle, awesome

Either way, I continue to appreciate react-native-vector-icons and your work here. Cheers!