-
Notifications
You must be signed in to change notification settings - Fork 2
Add React Navigation with React Native Gradle Plugin #27
Changes from 11 commits
94b46a4
e76d204
812774d
5e05f1d
0366369
145233e
eb94777
6085b79
3012948
d200fce
73c373a
7c5c493
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,5 +1,5 @@ | ||
pluginManagement { | ||
gradle.ext.agpVersion = '7.2.1' | ||
gradle.ext.agpVersion = '7.4.2' | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If we use There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I am working on updating JDK and AGP for all projects, so hopefully it won't become a big issue. Unfortunately, if we don't update AGP, the project did not build for me. So, I felt it was a must at the time. However, we can certainly try again to see if we can get There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Gotcha. At the time of writing my first comment, I don't know why, but I assumed that React Native Gradle Plugin has not fixed version, but it's always somehow the newest one (like "+" dependency). That's of course incorrect - the React Native Gradle Plugin is bundled with the React Native version we define in That's where my concerns with AGP synchronization raised. For now, at this stage of experiment, I think it's completely fine to use AGP 7.4.2 here. I've updated AGP to 7.4.2 as well on woocommerce/woocommerce-android#9225 PR so we can still include |
||
gradle.ext.kotlinVersion = '1.8.10' | ||
|
||
repositories { | ||
|
@@ -18,3 +18,5 @@ apply from: file("../../node_modules/@react-native-community/cli-platform-androi | |
|
||
include(":library") | ||
include(":demo") | ||
|
||
includeBuild('../../node_modules/react-native-gradle-plugin') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I'm not mistaken, this means that we'll also need to apply
com.facebook.react
toWCAndroid
and introducereact {
extension to configure it.I'm worried if applying
com.facebook.react
is safe forWCAndroid
- it feels fragile, but I don't have arguments to support it.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since we'll package the
library
module into an artifact, this shouldn't be the case. However, I have doubts that it'll work out of the box and we'll likely need to do some of what the React Native Gradle Plugin is doing ourselves. For example, I am hoping that the plugin substitutes thereact-native-*
library dependencies with the artifacts frommavenCentral
, as it suggests in its document. However, if it doesn't do that, since we won't have access to thenode_modules
folder from WCAndroid, we'll have to make sure we point to an artifact for these dependencies.In short, I don't know if this setup will work for WCAndroid when we use binary dependencies. However, I don't think we'll ever want to apply
com.facebook.react
plugin to WCAndroid. Whatever issues come up, we'll have to deal with them some other way.We should consider this PR just a step for you to be able to work on the project. Hope that makes sense.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Gotcha, I think it's a good approach 👍 .
My biggest concern about this PR was that it'll break the included builds configuration with WCAndroid and, therefore, block our further progress. But, I managed to make it work by:
dependencySubstitiution
block d976f25So, as after those changes we can still include
WooCommerce-Shared
toWCAndroid
, I have no concerns with merging this PR 👍 .