-
Notifications
You must be signed in to change notification settings - Fork 24.7k
Generate RCTThirdPartyComponents.mm with platform-specific macros #50637
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
base: main
Are you sure you want to change the base?
Conversation
Hi @cipolleschi, I noticed your name on #47518. I found a minor regression linked to that PR in the New Architecture Codegen logic and hope this PR is satisfactory for merging. Thank you for your efforts. 🙇 (The PR is small, but I added many additional tests.) |
@@ -15,6 +15,9 @@ const { | |||
isReactNativeCoreLibrary, | |||
parseiOSAnnotations, | |||
} = require('./utils'); | |||
const { | |||
generateSupportedApplePlatformsMacro, | |||
} = require('@react-native/codegen/lib/generators/components/ComponentsProviderUtils'); |
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'm a bit afraid of this deep import. I think it would be nice if we can re-export that function from @react-native/codegen
. WDYT?
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.
@cipolleschi, thank you for the feedback. I was worried about this code myself. You've all been busy, so I performed a rebase on my branch and updated the code to avoid the deep import. I also updated the code in the test app https://github.com/cgoldsby/RN-889
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.
Hi @cipolleschi, I believe I've resolved the issue with the deep import. What do you think of the changes?
Thanks 🙇
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.
It has some relations to #51077 and offers a broader approach for OOT platforms.
a826a67
to
c4a3fcf
Compare
c4a3fcf
to
9b6fd72
Compare
Summary:
iOS crashes in the New Architecture because Codegen includes all installed packages — even ones that don’t support iOS. The Legacy Architecture uses a different code generator that doesn’t have this issue. In both architectures, CocoaPods correctly excludes platform-specific native code — but Codegen doesn’t respect those exclusions in the New Architecture.
To prevent this, a macro was originally introduced to guard Objective-C code in generated components:
→ #42047
However, that macro usage was inadvertently lost during a later refactor that moved the Codegen provider to a static template:
→ #47518
This PR restores those platform guard macros in
RCTThirdPartyComponentsProvider
, ensuring that unsupported packages do not cause runtime crashes on iOS:This issue was initially reported in react-native-tvos#889, where a developer's iOS app crashed after including a tvOS-only package. CocoaPods correctly excluded the code, but Codegen still attempted to register the component at runtime.
Changelog:
[iOS] [Fixed] - Reintroduce platform guard macros in RCTThirdPartyComponentsProvider to prevent crashes when including platform-specific packages in OOT apps.
Test Plan:
MyApp contains several React Native packages that do not support iOS. When running on an iOS device, the app crashes.
Reproduce the steps for crash:
❌ MyApp launches and crashes immediately.
Apply patch to fix:
git apply patches/* yarn ios -i
✅ MyApp launches and does not crash.
Related Issues:
#42047 (Macro introduced)
#47518 (Regression)
react-native-tvos/react-native-tvos#889 (Original crash report)
https://github.com/cgoldsby/RNTV-889 (RVTV example)