Conversation
|
@cursor please review this |
|
Based on my review of the Pull Request, here's my assessment: PR Review: Add support for versioned darwin frameworkSummaryThis PR implements versioned Apple framework bundle support for Darwin/macOS targets, addressing a previously noted TODO. The changes are well-structured and follow Apple's documented framework anatomy conventions. Changes Reviewed1.
2.
3.
4.
Issues FoundBug in await updateLibraryInstallName(libraryName, libraryName, frameworkPath);The first argument should be await updateLibraryInstallName(
path.join("Versions", VERSIONED_FRAMEWORK_VERSION, libraryName),
libraryName,
frameworkPath,
);The async function updateLibraryInstallName(binaryPath: string, libraryName: string, cwd: string)So for flat frameworks, line 98 should be: await updateLibraryInstallName(libraryName, libraryName, frameworkPath);Wait, looking more carefully - since Minor Suggestions
} else if (paths.length == 1) {Overall AssessmentApprove with minor fixes. The implementation is solid and follows Apple conventions correctly. The refactoring improves code organization and the new The main item to address is the |
…al strings Co-authored-by: Kræn Hansen <mail@kraenhansen.dk>
|
@cursor are you able to understand why the MacOS test app is failing? I've failed to reproduce this locally. |
|
Based on my analysis, the CI failure is not related to the versioned framework changes in this PR. Here's what I found: Root CauseThe failure is a Metro bundler module resolution error: This happens during the "Bundle React Native code and images" build phase, which is pure JavaScript bundling - not native code compilation. Why it might fail in CI but not locallyLooking at "@react-native-node-api/node-addon-examples": path.relative(
APP_PATH,
path.join(ROOT_PATH, "packages", "node-addon-examples"),
),The issue could be:
Suggested investigation
This is a pre-existing issue with the macOS test app setup, not something introduced by the versioned framework changes in this PR. |
|
@cursor it did pass on a very recent commit and it fails consistently on this PR https://github.com/callstackincubator/react-native-node-api/actions/runs/21864104696/job/63100603421 |
|
Based on my investigation, I have an update on the CI failure: FindingsThe failure is a Metro module resolution error where What I observed
Possible causes
RecommendationTry re-running the CI job to see if it's intermittent. If it consistently fails, you might want to:
|


Fixes #353 and #280.
Merging this PR will: