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

feat: improve native_module.gradle for certain custom project setups #2341

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

AlexanderEggers
Copy link

@AlexanderEggers AlexanderEggers commented Mar 21, 2024

Summary:

This change include the ability for project to run a custom setup where the react project is not directly part of the android root project but instead is located somewhere else.

This change is backwards compatible and won't break any templates or existing projects.

I have added more details as part of this issue: #2289

Test Plan:

I added a patch in my local project setup (RN 0.73.6) and was able to compile and run the project without issues. This is the following patch I've applied:

diff --git a/native_modules.gradle b/native_modules.gradle
index bbfa7f741aed02bbeed4494c56a475ec19171c70..d07af21dfed430ed8fd33eb253e7588e8d9c48b5 100644
--- a/native_modules.gradle
+++ b/native_modules.gradle
@@ -479,15 +479,6 @@ class ReactNativeModules {
   }
 }
 
-
-/*
- * Sometimes Gradle can be called outside of JavaScript hierarchy. Detect the directory
- * where build files of an active project are located.
- */
-def projectRoot = rootProject.projectDir
-
-def autoModules = new ReactNativeModules(logger, projectRoot)
-
 def reactNativeVersionRequireNewArchEnabled(autoModules) {
     def rnVersion = autoModules.reactNativeVersion
     def regexPattern = /^(\d+)\.(\d+)\.(\d+)(?:-(\w+(?:[-.]\d+)?))?$/
@@ -507,11 +498,16 @@ def reactNativeVersionRequireNewArchEnabled(autoModules) {
  *    Exported Extensions
  * ------------------------ */
 
-ext.applyNativeModulesSettingsGradle = { DefaultSettings defaultSettings ->
+ext.applyNativeModulesSettingsGradle = { DefaultSettings defaultSettings, File reactRoot = null ->
+  def projectRoot = reactRoot != null ? reactRoot : rootProject.projectDir
+  def autoModules = new ReactNativeModules(logger, projectRoot)
   autoModules.addReactNativeModuleProjects(defaultSettings)
 }
 
 ext.applyNativeModulesAppBuildGradle = { Project project ->
+  def reactExtension = rootProject.extensions.getByName("privateReact")
+  def projectRoot = reactExtension.root.getAsFile().getOrNull() ?: rootProject.projectDir
+  def autoModules = new ReactNativeModules(logger, projectRoot)
   autoModules.addReactNativeModuleDependencies(project)
 
   def generatedSrcDir = new File(buildDir, "generated/rncli/src/main/java")

Checklist

  • Documentation is up to date to reflect these changes.
  • Follows commit message convention described in CONTRIBUTING.md

This change include the ability for project to set their own custom path to the actual folder which contains the react-native project. 

Given we have a consumer app which contains an android project but not actually react-native with the exception of the react-native gradle plugin, the current integration would lead to bunch of build failures. This is because the native_modules.gradle integration assumes that the react native project will be at the same some spot as the gradle root project location.

This change is backwards compatible and won't break any templates or existing projects.
@cortinico
Copy link
Member

This is unnecessary. Instead we should access "root" from react-native config which already contain this information and is configurable

@AlexanderEggers
Copy link
Author

AlexanderEggers commented Mar 21, 2024

@cortinico I was able to figure out how to use the react plugin root property in the applyNativeModulesAppBuildGradle function. I reverted the signature changes and internally it will work as you've mentioned in your message.

But unfortunately I cannot see a working scenario for the applyNativeModulesSettingsGradle function. We've only got the DefaultProject and DefaultSettings available at that point - maybe I'm missing something. Do you have an idea?

Instead of parsing a path, I changed the signature to pass a file object instead.
@cortinico
Copy link
Member

@cortinico I was able to figure out how to use the react plugin root property in the applyNativeModulesAppBuildGradle function. I reverted the signature changes and internally it will work as you've mentioned in your message.

But unfortunately I cannot see a working scenario for the applyNativeModulesSettingsGradle function. We've only got the DefaultProject and DefaultSettings available at that point - maybe I'm missing something. Do you have an idea?

The call new ReactNativeModules(logger, providers, projectRoot) invokes react-native config under the hood.

From react-native config you should be able to access the root field in the JSON object and use it around.

@AlexanderEggers
Copy link
Author

AlexanderEggers commented Mar 22, 2024

@cortinico I see what you mean. That won't work in my project setup. This CLI call already requires a root folder defintion but in my case the react-native project sits not within the project root but within a different folder.

This is the part I'm referring to:

def cliResolveScript = "try {console.log(require('@react-native-community/cli').bin);} catch (e) {console.log(require('react-native/cli').bin);}"
String[] nodeCommand = ["node", "-e", cliResolveScript]
def cliPath = this.getCommandOutput(nodeCommand, this.root)

String[] reactNativeConfigCommand = ["node", cliPath, "config"]
def reactNativeConfigOutput = this.getCommandOutput(reactNativeConfigCommand, this.root)

If I don't do that, the android project runs into the error of not being able to find the react-native/cli. Let me know if I'm misunderstanding anything.

IMO we need to know about where the RN project is located before we actually call that CLI otherwise this will always fail.

@cortinico
Copy link
Member

If I don't do that, the android project runs into the error of not being able to find the react-native/cli. Let me know if I'm misunderstanding anything.

mmm I'm not sure I follow you.
We call require('@react-native-community/cli').bin exactly to find out where the CLI is. Once we found it, we invoke config on it.

Do you have an example of your folder setup?

@AlexanderEggers
Copy link
Author

AlexanderEggers commented Mar 22, 2024

Yep require('@react-native-community/cli').bin is exactly the part which won't know unless you know where the react root directory is before you run that.

My project has the following structure:

- Android Project
-- reactnativemodule folder

The android project is a standard Android project which includes a few lines from the react-native boilerplate (e.g. classpath defintions for the react-native gradle plugin, usage of the react-native plugin and applying changes to the settings.gradle file).

The reactnativemodule-part is a react-native project and consumed as an Android library in the base project but contains all the react-native setup including the node_modules folder, package.json etc. The benifite of that is that I can add a new react-native part to the android base project without having to impact its setup too much. I can also much easier remove the react-native part from the project again if I want that. I can provide more details if that helps.

This is how my react plugin looks in regards to its options:

react {
    root = file("../reactnativemodule/")
    reactNativeDir = file("../reactnativemodule/node_modules/react-native")
    codegenDir = file("../reactnativemodule/node_modules/@react-native/codegen")
    cliFile = file("../reactnativemodule/node_modules/react-native/cli.js")
    entryFile = file("../reactnativemodule/index.js")
}

This is the error I can see if I run my project without the patch in this PR:

Caused by: java.lang.Exception: node:internal/modules/cjs/loader:1137 throw err; ^Error: Cannot find module 'react-native/cli'Require stack:- /Users/alex.eggers/Documents/Projects/RNAndroid/[eval] at Module._resolveFilename (node:internal/modules/cjs/loader:1134:15) at Module._load (node:internal/modules/cjs/loader:975:27) at Module.require (node:internal/modules/cjs/loader:1225:19) at require (node:internal/modules/helpers:177:18) at [eval]:1:87 at Script.runInThisContext (node:vm:122:12) at Object.runInThisContext (node:vm:298:38) at node:internal/process/execution:82:21 at [eval]-wrapper:6:24 at runScript (node:internal/process/execution:81:62) { code: 'MODULE_NOT_FOUND', requireStack: [ '/Users/alex.eggers/Documents/Projects/RNAndroid/[eval]' ]}Node.js v18.19.0

@cortinico
Copy link
Member

Yep require('@react-native-community/cli').bin is exactly the part which won't know unless you know where the react root directory is before you run that.

Sorry for the dumb question, but why is this the case? You're resolving a package that you have installed in your node_modules no?

Could I ask you to have a super small reproducer with your folder setup?
I'm not convinced that we need this change in the first place + we're looking into moving some of this code inside RNGP, so it would be crucial to understand if there is a user setup we're not supporting out of the box

@AlexanderEggers
Copy link
Author

AlexanderEggers commented Mar 23, 2024

You're resolving a package that you have installed in your node_modules no?

Correct, but that node_modules is sitting with the reactnativemodule-folder and not inside the android base project (parent folder). I'm guessing that in that moment of running the react native config cli, it assumes that the node_modules folder is inside the android root project. But because it is not, I cannot run the CLI to actually retrieve the needed plugin information to know where the node_modules is located. Does that make sense to you?

I can definitely create an example project for you over the next few days.

@szymonrybczak
Copy link
Collaborator

Could I ask you to have a super small reproducer with your folder setup? I'm not

Someone linked this as a repro for the same issues, might be helpful.

@AlexanderEggers
Copy link
Author

@cortinico I have added an example project which includes my scenario including the patch.

https://github.com/AlexanderEggers/react-native-module-example

@AlexanderEggers
Copy link
Author

Hey @cortinico, I'm sure you must be super busy and I hope you don't mind tagging you in a message but I just wanted to follow up on my last message and see if you had a chance to review the example project. Because you mentioned that you will be moving some of the CLI related code into the react-native gradle plugin soon, I believe it would be great if the issue can be solved as part of that migration. Let me now if you have any thoughts.

@cortinico
Copy link
Member

Hi @AlexanderEggers
Sorry for the late reply and thanks for providing this reproducer.

I believe we don't want to support this setup.
You should instead refactor your project to be a yarn monorepo, with a top level package.json.

The current setup doesn't work because your app folder is outside a root that doesn't have access to node_modules.

We had this same issue reported in the past weeks:

The thing is that we developed all the logic under the assumption that the .gradle file where the autolinking is invoked, can have access to a node_modules folder within its hierarchy. Breaking this assumption would require a significant rework.

On top of this, @blakef is working on moving this file (native_modules.gradle) inside the React Native Gradle Plugin, so even if we were to support this setup, we'll have to wait for the file to be migrated + apply the changes to RNGP instead.

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

Successfully merging this pull request may close these issues.

None yet

3 participants