Derive tag on fork from .hermesversion in "react-native" #158
Conversation
kraenhansen
added
enhancement
🦋 Changeset detectedLatest commit: 2b82046 The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 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 |
| ); | ||
| console.log(hermesPath); | ||
| } catch (error) { | ||
| process.exitCode = 1; |
There was a problem hiding this comment.
Turns out this didn't result in a failure.
There was a problem hiding this comment.
Pull Request Overview
This PR replaces the hardcoded Hermes git tag with a version derived from the .hermesversion file in the React Native package and adjusts the clone logic and error handling accordingly.
- Remove static
HERMES_GIT_TAGand derivepatchedTagfrom.hermesversion - Read and log the Hermes version from React Native’s SDK folder
- Update the git clone command to use the dynamic tag and add better error handling
- Add a changeset entry for the new behavior
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| packages/host/src/node/cli/hermes.ts | Read .hermesversion, compute patchedTag, update clone command, and refine error handling |
| .changeset/upset-paws-follow.md | Add a changeset to document deriving the fork tag from .hermesversion |
Comments suppressed due to low confidence (1)
packages/host/src/node/cli/hermes.ts:63
- [nitpick] Add unit tests covering the new logic that reads
.hermesversionand constructspatchedTag, ensuring the correct tag string is produced for different version inputs.
const patchedTag = `node-api-${hermesVersion}`;
| "sdks", | ||
| ".hermesversion" | ||
| ); | ||
| const hermesVersion = fs.readFileSync(hermesVersionPath, "utf8").trim(); |
There was a problem hiding this comment.
Wrap the .hermesversion read in a try/catch to surface a clear error if the file is missing or unreadable, e.g., ‘Failed to read Hermes version from .hermesversion: ’.
| const hermesVersion = fs.readFileSync(hermesVersionPath, "utf8").trim(); | |
| let hermesVersion: string; | |
| try { | |
| hermesVersion = fs.readFileSync(hermesVersionPath, "utf8").trim(); | |
| } catch (error) { | |
| throw new Error(`Failed to read Hermes version from ${hermesVersionPath}: ${error.message}`); | |
| } |
There was a problem hiding this comment.
This is very unlikely to happen. If I were to do this, I would call a function instead of using a mutable var.
| console.error( | ||
| `\n🛑 React Native uses the ${hermesVersion} tag and cloning our fork failed.`, | ||
| `Please see the Node-API package's peer dependency on "react-native" for supported versions.` | ||
| ); |
There was a problem hiding this comment.
Ideally, we would match on the output of the command, but that isn't yet supported by bufout (see kraenhansen/bufout#2)
kraenhansen
added
Apple 🍎
Merging this PR will:
.hermesversionin "react-native" brought by the app: https://github.com/facebook/react-native/blob/v0.79.2/packages/react-native/sdks/.hermesversionWith this change, we can start pushing multiple tags to the React Native fork, to enable multiple React Native versions, ultimately fixing #157.