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

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

mikehardy
Copy link
Contributor

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 --> invertase/react-native-firebase@b52d0ce

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
Copy link
Owner

oblador commented Dec 14, 2023

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
Copy link
Contributor Author

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!

@gmantuanrosa
Copy link

My project was running react-native 0.72.6 and I am upgrading to 0.73.9 AGP 8 and Gradle 8.3.

This package and some other packages started to have this issue related to buildFeatures.buildConfig, most of them applied the fix adding the buildConfig parameter, vector-icons is one of that is not yet implementing this.

For now I will patch node_modules (and it worked without any issues) but it would be extremely important to have this enabled so I or others could bump the react-native version.

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

Successfully merging this pull request may close these issues.

4 participants