Skip to content
This repository has been archived by the owner on Jan 31, 2024. It is now read-only.

Add React Navigation with React Native Gradle Plugin #27

Merged
merged 12 commits into from
Jun 19, 2023

Conversation

wzieba
Copy link
Contributor

@wzieba wzieba commented Jun 16, 2023

Description

This PR adds React Navigation dependencies. To make the build pass, it furthermore:

  • adds React Native Gradle Plugin
  • declares application and library plugins in library/build.gradle
  • replaces Jetpack Compose with android.view.* views

@wzieba wzieba requested a review from oguzkocer June 16, 2023 08:58
@wzieba
Copy link
Contributor Author

wzieba commented Jun 16, 2023

I'll put this in draft, as I'm uncertain if we'll end up integrating React Native Navigation - I'm having some difficulties in making it work (after successful build).

@wzieba wzieba marked this pull request as draft June 16, 2023 10:02
Copy link
Contributor Author

@wzieba wzieba left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @oguzkocer . I left some comments, WDYT?

Comment on lines +4 to +17
id 'com.facebook.react'
}

repositories {
mavenCentral()
google()
}

react {
root = file("../../../")
entryFile = file("../../../index.tsx")
reactNativeDir = file("../../../node_modules/react-native")
codegenDir = file("../../../node_modules/react-native-codegen")
}
Copy link
Contributor Author

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 to WCAndroid and introduce react { extension to configure it.

I'm worried if applying com.facebook.react is safe for WCAndroid - it feels fragile, but I don't have arguments to support it.

Copy link
Collaborator

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 the react-native-* library dependencies with the artifacts from mavenCentral, as it suggests in its document. However, if it doesn't do that, since we won't have access to the node_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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Gotcha, I think it's a good approach 👍 .

We should consider this PR just a step for you to be able to work on the project. Hope that makes sense.

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:

  • Updating Gradle Wrapper to 7.5.1 722e761
  • Updating AGP to 7.4.2 1d1f720
  • Updating Aztec to v1.6.4 (previous version caused some DEX merging failing) 8fb35c0
  • Substituting React Native invalid dependencies with correct ones in dependencySubstitiution block d976f25

So, as after those changes we can still include WooCommerce-Shared to WCAndroid, I have no concerns with merging this PR 👍 .

@@ -1,5 +1,5 @@
pluginManagement {
gradle.ext.agpVersion = '7.2.1'
gradle.ext.agpVersion = '7.4.2'
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we use includeBuild with WCAndroid, it means that we'll also have to bump AGP in WCAndroid to 7.4.2. Then, we'll have to bump FluxC to 7.4.2 (if we want to keep included builds), then WPAndroid 😞 . It creates chain of work dependent from the authors of React Native Gradle Plugin - I feel it might be dangerous in a long run. WDYT?

Copy link
Collaborator

Choose a reason for hiding this comment

The 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 7.2.1 to work.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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 WooCommerce-Shared to WCAndroid.

@wzieba wzieba marked this pull request as ready for review June 19, 2023 13:36
@wzieba wzieba enabled auto-merge June 19, 2023 13:44
@wzieba wzieba merged commit 6ede16b into trunk Jun 19, 2023
@wzieba wzieba deleted the build/add_navigation-with-react-native-gradle-plugin branch June 19, 2023 13:46
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants