-
Notifications
You must be signed in to change notification settings - Fork 7
MacOS test app and fix hermes vendoring from react-native-macos
#297
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
ea60bd0 to
4f1711e
Compare
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.
Pull Request Overview
This PR adds support for macOS test apps by introducing initialization scripts, CI workflow integration, and fixes for React Native macOS compatibility. The changes enable automated testing on macOS and improve Hermes vendoring detection for react-native-macos.
Key Changes:
- Added macOS test app initialization script with React Native macOS template setup
- Implemented caller stack inspection in Ruby script to automatically detect react-native vs react-native-macos
- Refactored XCFramework slice selection to use prioritized fallback lists instead of single hardcoded paths
Reviewed Changes
Copilot reviewed 7 out of 9 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tsconfig.scripts.json | New TypeScript configuration for scripts directory |
| scripts/init-macos-test-app.ts | Script to initialize and configure a React Native macOS test application |
| packages/host/scripts/patch-hermes.rb | Added automatic detection of React Native package type using caller stack |
| packages/ferric/src/cargo.ts | Changed XCFramework slice mapping from single values to prioritized arrays with fallback logic |
| package.json | Added init-macos-test-app script and read-pkg dependency |
| .github/workflows/check.yml | Added macOS test job with label-based triggering |
| .changeset/silly-mice-warn.md | Changeset documenting Hermes vendoring fix for macOS |
| - uses: actions/checkout@v4 | ||
| - uses: actions/setup-node@v4 | ||
| with: | ||
| node-version: lts/jod |
Copilot
AI
Oct 28, 2025
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.
Corrected spelling of 'jod' to 'iron'. The Node.js LTS codename should be 'iron'.
| // "--platform-name", | ||
| // "react-native-macos", |
Copilot
AI
Oct 28, 2025
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.
Commented-out code should be removed. If these options were used during development but are no longer needed, they should be deleted rather than left as comments.
| // "--platform-name", | |
| // "react-native-macos", |
ecb0fee to
ab26a44
Compare
|
Drafting again as I'll attempt to include running the tests in this PR as well. I believe I'm pretty far, while having difficulties initializing the host TurboModule 🤔
|
|
@copilot can you add the missing x86_64 Darwin Rust target currently failing the build. I expect this will take just a single step with a single rustup command. Keep it brief, no comments in the workflow. Add only the missing target, nothing more and get some inspiration from how the other targets are added in other jobs of the workflow. |
|
@kraenhansen I've opened a new pull request, #298, to work on those changes. Once the pull request is ready, I'll request review from you. |
063b11e to
bd42182
Compare
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.
Pull Request Overview
Copilot reviewed 22 out of 25 changed files in this pull request and generated 2 comments.
scripts/init-macos-test-app.ts
Outdated
| premacos: "killall 'MacOSTestApp' || true", | ||
| macos: "react-native run-macos --no-packager", | ||
| test: "mocha-remote --exit-on-error -- concurrently --passthrough-arguments --kill-others-on-fail npm:metro 'npm:macos -- {@}' --", | ||
| "test:allTests": "MOCHA_REMOTE_CONTEXT=allTests node --run test:ios -- ", |
Copilot
AI
Nov 1, 2025
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.
The script reference 'test:ios' appears incorrect for a macOS test app. This should likely be 'test:macos' or just 'test' to match the other test scripts defined in this file.
| "test:allTests": "MOCHA_REMOTE_CONTEXT=allTests node --run test:ios -- ", | |
| "test:allTests": "MOCHA_REMOTE_CONTEXT=allTests node --run test -- ", |
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.
Pushed a fix for this 👍
packages/host/src/node/cli/apple.ts
Outdated
| if (fs.existsSync(versionAPath) && !fs.existsSync(versionCurrentPath)) { | ||
| await fs.promises.symlink( | ||
| path.relative(path.dirname(versionCurrentPath), versionAPath), | ||
| versionCurrentPath, | ||
| ); | ||
| } | ||
|
|
||
| const { CFBundleExecutable } = await readFrameworkInfo( | ||
| path.join(frameworkPath, "Versions", "Current", "Resources", "Info.plist"), | ||
| ); | ||
|
|
Copilot
AI
Nov 1, 2025
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.
The code creates the 'Current' symlink on line 204-207, then immediately uses it on line 211 to read the Info.plist. However, if the symlink didn't exist before and was just created, the Info.plist path should use 'A' instead of 'Current', or the readFrameworkInfo call should occur before attempting to restore any symlinks.
| if (fs.existsSync(versionAPath) && !fs.existsSync(versionCurrentPath)) { | |
| await fs.promises.symlink( | |
| path.relative(path.dirname(versionCurrentPath), versionAPath), | |
| versionCurrentPath, | |
| ); | |
| } | |
| const { CFBundleExecutable } = await readFrameworkInfo( | |
| path.join(frameworkPath, "Versions", "Current", "Resources", "Info.plist"), | |
| ); | |
| let symlinkJustCreated = false; | |
| if (fs.existsSync(versionAPath) && !fs.existsSync(versionCurrentPath)) { | |
| await fs.promises.symlink( | |
| path.relative(path.dirname(versionCurrentPath), versionAPath), | |
| versionCurrentPath, | |
| ); | |
| symlinkJustCreated = true; | |
| } | |
| // If we just created the symlink, use the real path to Info.plist; otherwise, use the symlink path | |
| const infoPlistPath = symlinkJustCreated | |
| ? path.join(frameworkPath, "Versions", "A", "Resources", "Info.plist") | |
| : path.join(frameworkPath, "Versions", "Current", "Resources", "Info.plist"); | |
| const { CFBundleExecutable } = await readFrameworkInfo(infoPlistPath); |
f40d5ce to
57496a5
Compare
57496a5 to
edb09ca
Compare
Enable Hermes and Fabric Patch react_native_post_install to pass react native path Don't pod install when initializing Patch macos app with scripts and source files Re-arm the init script Move linked deps into install command to make --install-links effective Using regular linking for monorepo deps Include original dependencies Enable new arch in Podfile Fix comment from review
No need to Setup Android SDK Debugging with Copilot Pass --mode when building from CLI Install CMake 3.22 Fix bootstrap issue Trying a higher CMake version Remove debug info from workflow Build universal Darwin libraries Add missing x86_64-apple-darwin Rust target to macOS CI job (#298) * Initial plan * Add missing x86_64-apple-darwin Rust target to macOS job Co-authored-by: kraenhansen <[email protected]> * Add only missing x86_64-apple-darwin target (aarch64 is host) Co-authored-by: kraenhansen <[email protected]> --------- Co-authored-by: copilot-swe-agent[bot] <[email protected]> Co-authored-by: kraenhansen <[email protected]> Run MacOS test app Use package script to run all tests
edb09ca to
02ae255
Compare
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.
Pull Request Overview
Copilot reviewed 9 out of 12 changed files in this pull request and generated 3 comments.
| - uses: actions/checkout@v4 | ||
| - uses: actions/setup-node@v4 | ||
| with: | ||
| node-version: lts/jod |
Copilot
AI
Nov 2, 2025
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.
Corrected spelling of 'jod' to 'jod' - this appears to be a typo for 'iron' (the current Node.js LTS codename).
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.
@copilot you seem to be outdated on the name of the latest active LTS, please read https://nodejs.org/en/blog/release/v22.11.0 and update your instructions with a brief comment to ensure you won't make that mistake again.
|
|
||
| if ENV['REACT_NATIVE_OVERRIDE_HERMES_DIR'].nil? | ||
| VENDORED_HERMES_DIR ||= `npx react-native-node-api vendor-hermes --silent '#{Pod::Config.instance.installation_root}'`.strip | ||
| VENDORED_HERMES_DIR ||= `npx react-native-node-api vendor-hermes --react-native-package '#{get_react_native_package()}' --silent '#{Pod::Config.instance.installation_root}'`.strip |
Copilot
AI
Nov 2, 2025
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.
The method call uses unnecessary parentheses. In Ruby, get_react_native_package() should be written as get_react_native_package when calling methods without arguments.
| VENDORED_HERMES_DIR ||= `npx react-native-node-api vendor-hermes --react-native-package '#{get_react_native_package()}' --silent '#{Pod::Config.instance.installation_root}'`.strip | |
| VENDORED_HERMES_DIR ||= `npx react-native-node-api vendor-hermes --react-native-package '#{get_react_native_package}' --silent '#{Pod::Config.instance.installation_root}'`.strip |
| console.log("Copying source files from test-app into macos-test-app:"); | ||
| const FILE_NAMES = [ | ||
| "App.tsx", | ||
| // Adds the babel plugin needed to transform require calls | ||
| "babel.config.js", | ||
| // Adds the ability to reference symlinked packages |
Copilot
AI
Nov 2, 2025
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.
[nitpick] The comment on line 170-172 mentions 'Adds the babel plugin' and 'Adds the ability', but these describe what the files do rather than why they're being copied. The console.log could be more specific about which test-app (iOS) the files are being copied from.
| console.log("Copying source files from test-app into macos-test-app:"); | |
| const FILE_NAMES = [ | |
| "App.tsx", | |
| // Adds the babel plugin needed to transform require calls | |
| "babel.config.js", | |
| // Adds the ability to reference symlinked packages | |
| console.log(`Copying source files from ${OTHER_APP_PATH} into ${APP_PATH}:`); | |
| const FILE_NAMES = [ | |
| // Ensure macos-test-app uses the same entry point as test-app | |
| "App.tsx", | |
| // Ensure macos-test-app uses the same Babel config as test-app (for require transformation) | |
| "babel.config.js", | |
| // Ensure macos-test-app uses the same Metro config as test-app (for symlinked packages) |
|
@kraenhansen I've opened a new pull request, #306, to work on those changes. Once the pull request is ready, I'll request review from you. |
Merging this PR will:
--react-native-package- fixing Support vendoring Hermes for out-of-tree platforms #270Note how the CI job prints this while pod installing: