Skip to content

Conversation

@kraenhansen
Copy link
Collaborator

This is a follow-up to #234

Merging this PR will:

  • Move REACT_NATIVE_OVERRIDE_HERMES_DIR out of tasks to fail earlier.

@kraenhansen kraenhansen self-assigned this Sep 20, 2025
@kraenhansen kraenhansen added Android 🤖 Anything related to the Android platform (Gradle, NDK, Android SDK) Host 🏡 Our `react-native-node-api-modules` package labels Sep 20, 2025
@kraenhansen kraenhansen force-pushed the kh/android/check-hermes-override-globally branch from 2865a90 to b0f6f61 Compare September 21, 2025 09:56
const { status, stdout, stderr } = cp.spawnSync(
"sh",
["gradlew", "react-native-node-api:checkHermesOverride"],
["gradlew", "react-native-node-api:linkNodeApiModules"],
Copy link

Choose a reason for hiding this comment

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

Bug: Test Misplaced: Hermes Directory Check

The test "should fail if REACT_NATIVE_OVERRIDE_HERMES_DIR is not set" is nested under the linkNodeApiModules task describe block, but the REACT_NATIVE_OVERRIDE_HERMES_DIR check now occurs during Gradle build script evaluation. This means the failure happens before the task executes, making the test's context misleading as it implies the check is part of the task itself.

Fix in Cursor Fix in Web

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I need some task which wouldn't fail if the feature is broken, which is why I put it here.

@kraenhansen kraenhansen merged commit c72970f into main Sep 21, 2025
17 of 19 checks passed
@kraenhansen kraenhansen deleted the kh/android/check-hermes-override-globally branch September 21, 2025 18:42
Copy link
Contributor

@matthargett matthargett left a comment

Choose a reason for hiding this comment

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

Minor wording improvement suggestion

if (!System.getenv("REACT_NATIVE_OVERRIDE_HERMES_DIR")) {
throw new GradleException([
"React Native Node-API needs a custom version of Hermes with Node-API enabled.",
"Run the following in your terminal, to clone Hermes and instruct React Native to use it:",
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
"Run the following in your terminal, to clone Hermes and instruct React Native to use it:",
"Run the following in your Bash- or zsh-compatible terminal, to clone Hermes and instruct React Native to use it:",

Copy link
Contributor

Choose a reason for hiding this comment

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

putting in separate examples for powershell and cmd prompt probably isn't reasonable, but hopefully this will make it clear (to a human and LLM) not to try to paste this command.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I pushed this as d8872b6 to main 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Android 🤖 Anything related to the Android platform (Gradle, NDK, Android SDK) Host 🏡 Our `react-native-node-api-modules` package

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants