-
Notifications
You must be signed in to change notification settings - Fork 7
Check for REACT_NATIVE_OVERRIDE_HERMES_DIR earlier
#246
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
Conversation
2865a90 to
b0f6f61
Compare
| const { status, stdout, stderr } = cp.spawnSync( | ||
| "sh", | ||
| ["gradlew", "react-native-node-api:checkHermesOverride"], | ||
| ["gradlew", "react-native-node-api:linkNodeApiModules"], |
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.
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.
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 need some task which wouldn't fail if the feature is broken, which is why I put it here.
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.
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:", |
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.
| "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:", |
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.
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.
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 pushed this as d8872b6 to main 👍
This is a follow-up to #234
Merging this PR will:
REACT_NATIVE_OVERRIDE_HERMES_DIRout of tasks to fail earlier.