Add support for RN 0.73 #537
Conversation
packages/core/android/build.gradle
Outdated
| buildFeatures { | ||
| buildConfig = true | ||
| } | ||
| namespace = "com.datadog.reactnative" |
There was a problem hiding this comment.
by having it here I guess you can now completely delete AndroidManifest file then.
There was a problem hiding this comment.
I will try, but I think doing so might break compatibility with RN < 0.71
There was a problem hiding this comment.
why? If you are using AGP version which is using this property, then AndroidManifest is not needed if there are only basic things are used there. If AGP version used doesn't have this property, then compilation of build script will fail anyway.
There was a problem hiding this comment.
Ok I noticed the following behaviour:
- if I only remove the
packagefield from theAndroidManifest, the build fails for RN < 0.70 (as already mentioned here) - if I remove the whole
AndroidManifestfile, then the build passes.
So does this mean we did not need this AndroidManifest file from the start?
Are there any functionality we might miss when removing it?
There was a problem hiding this comment.
you shouldn't need the manifest file, it is removed in SDK v2 of Android.
There was a problem hiding this comment.
Do you have documentation about the support of libraries without a manifest file?
I could not find any docs mentioning that the manifest can be removed: https://developer.android.com/studio/projects/android-library
It seems to work fine with latest versions, but I want to be sure it won't break earlier versions of RN.
There was a problem hiding this comment.
I want to be sure it won't break earlier versions of RN
from what i have seen, rn cli reads the namespace from AndroidManifest.xml in older versions so it will likely fail the build.
| if (agpVersion.tokenize('.')[0].toInteger() >= 7) { | ||
| namespace = "com.datadog.reactnative" | ||
| } | ||
| if (agpVersion.tokenize('.')[0].toInteger() >= 8) { | ||
| buildFeatures { | ||
| buildConfig = true | ||
| } | ||
| } |
There was a problem hiding this comment.
I'm not quite sure it will work (but who knows, it is Groovy). If types are statically checked, then this build script won't compile with AGP 6.x even if new properties are wrapped in if. But if things are dynamic (maybe because of Groovy), it will be fine. Does you change work fine if you are running it in the context of AGP 6.x and 7.x?
There was a problem hiding this comment.
I tested with AGP 4.2.2 (no RN version uses AGP 5.x or 6.x, but can test these as well), and it worked fine.
Also I'm a bit surprised that even if I remove these if conditions everything builds as well 🤷♂️
Still, adding these if conditions is what's done by most RN libraries and I might be missing something here. I don't want to spend too much time on this so I think it's ok to go with the safest approach even if it adds a bit of useless code.
|
Builds for all RN versions are passing in Bitrise, I'm confident to merge this :) |
…pport Add support for RN 0.73
What does this PR do?
This PR brings support to RN 0.73 by adding support for AGP 8.0 (only change impacting us).
Tested:
Motivation
Fixes #493
Additional Notes
I think we might also release a
1.8.6version with this change only once the 0.73 is ready to be released to avoid coupling v2 release (which is still not 100% ready) with RN 0.73 release.Review checklist (to be filled by reviewers)