-
Notifications
You must be signed in to change notification settings - Fork 2
chore: better gradle setup #188
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
base: main
Are you sure you want to change the base?
Conversation
🦋 Changeset detectedLatest commit: 9077adf The changes in this PR will be included in the next version bump. This PR includes changesets to release 4 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
8b3614a
to
6b62848
Compare
@@ -19,3 +19,7 @@ apply(from: { | |||
throw new GradleException("Could not find `react-native-test-app`"); | |||
}()) | |||
applyTestAppSettings(settings) | |||
|
|||
// customize the path to match your node_modules location | |||
apply(from: "../../../node_modules/react-native-node-api/android/consumerSettings.gradle") |
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.
I wonder now ... how this ever worked 🤔
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.
Oh - I get it now.
The React Native Test App doesn't need this, because of https://github.com/microsoft/react-native-test-app/blob/d06a4ea4abd4dc31231b1c28c253873ce7f80215/test-app.gradle#L55-L65 and
react.buildFromSource=true |
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.
Right, that's already set up thanks to RNTA's scripts - I wasn't aware that you had it configured from my talk to @paradowstack before I jumped into the scripts. Do you think we should leave it there / maybe commented, as a reference sample, additionally to the MD docs? I think some may be concerned that the sample app does not contain this configuration if they browse its source for reference.
docs/ANDROID.md
Outdated
|
||
To integrate automatic setup of Hermes engine, a special env variable (`REACT_NATIVE_OVERRIDE_HERMES_DIR`) must be set to a proper path. Since Gradle does not really support loading `.env` files, this must be automated by the consumer. We provide the script `react-native-node-api vendor-hermes --silent` which will output a single line, the path to Hermes directory. | ||
|
||
Each time you run Android Studio, make sure this is in place. |
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.
Each time you run Android Studio, make sure this is in place. | |
Each time you run Android Studio or start the Android app from a terminal, make sure this is in place. |
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.
I'd only change 'start' to 'build' - applied 👍
First off - thanks a ton for your interest in the project! I recognize that your PR is in draft and there's a few things that I'd like for you to address before merging this. |
Sure thing, thank you for the feedback & insights @kraenhansen . All we discussed is applied in the latest commit. I left one comment to discuss the test-app's |
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.
Code seems fine 👍 I'm curious if this is actually an improvement in DX and would like a bit more feedback before merging, if that's okay?
@@ -27,6 +27,10 @@ | |||
|
|||
See the document on ["how it works"](./docs/HOW-IT-WORKS.md) for a detailed description of what it's like to write native modules using this package. | |||
|
|||
### Quick start | |||
|
|||
To get started, first install the dependencies with `npm i` and then build all packages using `npm run build`. |
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.
Actually, I intended the "bootstrap" script to be the one you run to build both TypeScript and prebuilds, but it does take a while 🤔
To get started, first install the dependencies with `npm i` and then build all packages using `npm run build`. | |
To get started, first install the dependencies with `npm i` and then build all packages using `npm run bootstrap`. |
|
||
```groovy | ||
// customize the path to match your node_modules location | ||
apply(from: "../../../node_modules/react-native-node-api/android/app-settings.gradle") |
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.
When we introduce our own settings script it becomes a bit more magical IMO and I'm not 100% convinced this is an increase in DX 🤔 There's something "simple" in just instructing users to build React Native from source and defer to whatever upstream documentation is available for doing that, with a code-snippet to make it easier.
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.
WDYT @shirakaba?
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.
Right now we have asymmetry between the iOS and Android sides – iOS is set up automatically, while Gradle isn't. So I think we need to at least make things consistent.
If this were an Expo project, I'd probably arrange it as a config plugin called withBuildReactNativeFromSource
, so that it involves no magic specific react-native-node-api
and is just clearly a convenience function.
I think it's okay to defer to the upstream docs for building React Native from source. Maybe that part needn't be handled by our plugin, then?
Something I would like to improve the DX on, though, is this part:
export REACT_NATIVE_OVERRIDE_HERMES_DIR=`npx react-native-node-api vendor-hermes --silent`
It's a pain because it relies on shell stuff, so there's great potential for people to get it wrong (e.g. run it in the wrong directory or forget to export it again).
If this were an Expo project, I'd probably say that npx react-native-node-api vendor-hermes
should just append the necessary env var into .env.local
for you. But because React Native Community CLI projects don't have any standard env file that they source variables from, and because React Native strictly grabs that variable via System.getenv()
(in both react-native-gradle-plugin and hermes-engine), I'm not sure we have any great options.
Any ideas?
> [!IMPORTANT] | ||
> Each time you run Android Studio or build the Android app from a terminal, make sure the below is in place. | ||
|
||
Because we're using a version of Hermes patched with Node-API support, we need to build React Native from source. A special environment variable (`REACT_NATIVE_OVERRIDE_HERMES_DIR`) must be set to the path of a Hermes engine with Node-API support. Since Gradle does not support loading `.env` files directly, this must be automated by the consumer. We provide the `react-native-node-api vendor-hermes --silent` command, which will download Hermes and output the path to Hermes directory path as its only output. |
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.
Because we're using a version of Hermes patched with Node-API support, we need to build React Native from source. A special environment variable (`REACT_NATIVE_OVERRIDE_HERMES_DIR`) must be set to the path of a Hermes engine with Node-API support. Since Gradle does not support loading `.env` files directly, this must be automated by the consumer. We provide the `react-native-node-api vendor-hermes --silent` command, which will download Hermes and output the path to Hermes directory path as its only output. | |
Because we're using a version of Hermes patched with Node-API support, we need to build React Native and Hermes from source. A special environment variable (`REACT_NATIVE_OVERRIDE_HERMES_DIR`) must be set to the path of a Hermes engine with Node-API support. Since Gradle does not support loading `.env` files directly, this must be automated by the app developer. We provide the `react-native-node-api vendor-hermes --silent` command, which will download Hermes and output the path to Hermes directory path as its only output. |
export REACT_NATIVE_OVERRIDE_HERMES_DIR="$(npx react-native-node-api vendor-hermes --silent)" | ||
``` | ||
|
||
This either needs to be done each time before Gradle / Android Studio invocation, or permanently in a shell init script such as `~/.zshrc` on Zsh (MacOS) or `~/.bashrc` on Bash (Linux). |
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.
I'm not sure this is a good suggestion. It has the potential to mess up people's setup, quite significantly - in a way which is really hard to debug.
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.
Agreed, I think it'd be best to avoid mentioning the idea of populating the user profiles at all.
For Expo apps, we can recommend writing it into .env.local
in the root of the project – it's a standard location read by all the Expo build scripts (Gradle included).
For non-Expo apps, I don't think we have any options. The React Native Community CLI template has no hits for .env
or .env.local
.
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.
Have added some thoughts!
And thanks @artus9033, I was worrying about the DX on this part so this is a great PR to see. Let's keep talking about the ideal shape it should take.
export REACT_NATIVE_OVERRIDE_HERMES_DIR="$(npx react-native-node-api vendor-hermes --silent)" | ||
``` | ||
|
||
This either needs to be done each time before Gradle / Android Studio invocation, or permanently in a shell init script such as `~/.zshrc` on Zsh (MacOS) or `~/.bashrc` on Bash (Linux). |
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.
Agreed, I think it'd be best to avoid mentioning the idea of populating the user profiles at all.
For Expo apps, we can recommend writing it into .env.local
in the root of the project – it's a standard location read by all the Expo build scripts (Gradle included).
For non-Expo apps, I don't think we have any options. The React Native Community CLI template has no hits for .env
or .env.local
.
ext.applyNodeAPISettings = { DefaultSettings settings -> | ||
def searchDirectory = rootDir.toPath() | ||
do { | ||
def p = searchDirectory.resolve("node_modules/react-native") |
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.
Instead of walking the file system, could we use actual Node module resolution, like the Expo Android template demonstrates?
Caution: I'm not clear whether the reactNativeDir variable is populated in projects made with the React Native Community CLI, so it'd be safest to just re-evaluate the path inside this Gradle script.
|
||
```groovy | ||
// customize the path to match your node_modules location | ||
apply(from: "../../../node_modules/react-native-node-api/android/app-settings.gradle") |
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.
This path (../../../node_modules
) assumes a monorepo structure, so won't work on a regular repo.
Can we use proper Node module resolution instead? As demonstrated here.
|
||
```groovy | ||
// customize the path to match your node_modules location | ||
apply(from: "../../../node_modules/react-native-node-api/android/app-settings.gradle") |
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.
Right now we have asymmetry between the iOS and Android sides – iOS is set up automatically, while Gradle isn't. So I think we need to at least make things consistent.
If this were an Expo project, I'd probably arrange it as a config plugin called withBuildReactNativeFromSource
, so that it involves no magic specific react-native-node-api
and is just clearly a convenience function.
I think it's okay to defer to the upstream docs for building React Native from source. Maybe that part needn't be handled by our plugin, then?
Something I would like to improve the DX on, though, is this part:
export REACT_NATIVE_OVERRIDE_HERMES_DIR=`npx react-native-node-api vendor-hermes --silent`
It's a pain because it relies on shell stuff, so there's great potential for people to get it wrong (e.g. run it in the wrong directory or forget to export it again).
If this were an Expo project, I'd probably say that npx react-native-node-api vendor-hermes
should just append the necessary env var into .env.local
for you. But because React Native Community CLI projects don't have any standard env file that they source variables from, and because React Native strictly grabs that variable via System.getenv()
(in both react-native-gradle-plugin and hermes-engine), I'm not sure we have any great options.
Any ideas?
This does seem like a real problem. I've also run into Gradle's limitation here in the past. One approach would be to modify the React Native Community CLI so that it sources But I agree that allowing these to be configured as Gradle properties sounds like the ideal approach, as it's accessible by plugins. Would also be good to cover all Hermes options that are currently only configurable by env vars (e.g. |
This PR introduces the following:
react-native-node-api
):consumerSettings.gradle
that shall be included in the demo app'ssettings.gradle
; the integration has been described on the bottom of this post & in theANDROID.md
docshermes.ts
script which setisEnabled
toora
spinners based on the silent mode flag value, which wasn't really a silent mode but just turned off animations of the spinners, by replacing these withisSilent
to ensure a true silent modepostinstall
script to the root workspacepackage.json
to automaticallynpm run build
all packages upon install for ease of developmentThere unfortunately is no way to set env vars from Gradle scripts, therefore automation of export of
REACT_NATIVE_OVERRIDE_HERMES_DIR
is impossible to be achieved. If this is meant to be a temporary setup, then I would leave this step as manual for the consumer; if this is meant to be permanent, I would send a PR to RN, adding optional secondary resolution of custom Hermes path in those files using Gradle Properties instead, if this is worth the effort. CC @paradowstack @kraenhansenGradle script integration code to be added to any app's
settings.gradle
: