Fix build errors when inheriting RCTAppDelegate in Swift modules #35661
Conversation
facebook-github-bot
added
CLA Signed
Base commit: 8ccb861 |
Base commit: ba6a05b |
|
PR build artifact for 614dab4 is ready. |
|
@cipolleschi would be appreciated if you can review this pr when you get a chance 🙇♂️ |
|
This fix looks more a cure for a symptom rather than the cure for the cause. When Can we try to understand what's going on and see if we can fix it at the root cause? 🙏 |
|
let me share the detailed error log: in a nut shell, i could also upload the RNNext project to github for you if you want to investigate further. |
|
we have c++17 and i changed the EXAppDelegateWrapper to |
|
Can't you rename all those There is something I'm struggling to understand. How can you successfully compile if:
In other words... We have two use cases: NewArch enabled/disabled.
I hope I clarified my struggle with these use cases. I don't think that the issue is in the React native codebase, but more on how the project is set up. |
then we should rename all
yes you are right, the only problem is when RCT_NEW_ARCH_ENABLED=1 Minimal Reproducible Examplewell, to demonstrate the complicated problem, i've tried to have a minimal reproducible example with create-react-native-library. you could check with my repo here: https://github.com/Kudo/test-swift-module Steps:
i have my repro in commit history: https://github.com/Kudo/test-swift-module/commits/main
Conclusioneven though most module doesn't have such complicated objc <-> swift mixed code. i think there is a problem from react-native currently. when building main.m even in new architecture mode, we don't pass i'll leave a problem and discuss here. maybe we don't have to land this pr as we could apply the workaround internally. |
# Why sdk 48 may come next year and react-native 0.71 may be released this year. it's good to have react-native 0.71 support for our modules. if users want to upgrade 0.71, they could change their project to be a bare project for the early preview. # How - [core][av][gl] because 0.71 doesn't serve android aar along with npm package, the aar extraction from `node_modules/react-native/android/**/*.aar` will break on 0.71. 0.71 also ships modules with prefab support. it's good to rewrite the gradle/cmake files to link with prefab modules. that would reduce much complexity. however, to be backward compatible with 0.70, i moved most logic to `ExpoModulesCorePlugin.applyLegacyReactNativeLibsExtractionPlugin` and `legacy/CMakeLists.txt`. so that we could be backward compatible and easily remove the isolated code after we drop sdk 47 support. - [dev-launcher] add 0.71 sources because new parameter to `DevSupportManagerBase` - [core][dev-menu] fix ios build errors for jsc because 0.71 moved the header to `<React-jsc/JSCRuntime.h>` - [core][autolinking] integrate the `RCTAppDelegate` with our `EXAppDelegateWrapper`. that would simply the install-expo-modules migration where we only need the change `RCTAppDelegate -> EXAppDelegateWrapper` in AppDelegate.h. however, integrate RCTAppDelegate comes with some issues from expo-modules-core. [this commit](2959477) changes many code and i'll try to explain here: - RCTAppDelegate is in `React-RCTAppDelegate` pod, which does not define module. we define module for it in our patch system in autolinking. - defines `RCT_NEW_ARCH_ENABLED` as RCTAppDelegate - in new architecture mode, RCTAppDelegate comes with [more cxx dependencies](https://github.com/facebook/react-native/blob/03b17d9af7e4e3ad3f9ec078b76d0ffa33a3290e/Libraries/AppDelegate/RCTAppDelegate.h#L12-L18) for fabric. that's why we should add more header search paths and also move to EXAppDelegateWrapper.mm. # Test Plan - [x] ci passed - [x] bare-expo build and launch - [x] bare-expo nightlies build and launch - [x] fabric-tester build and launch - [x] fabric-tester nighties build and launch (should apply [the patch to fabric-tester](https://gist.github.com/Kudo/417a5159cb01a400ecee22ad4985d287)) - [x] rn 0.71-rc4 project build and launch (could apply [the patches](https://gist.github.com/Kudo/7448c733f12e700c7fbed76251fa3553) for testing) - [x] rn 0.71-rc4 project (new architecture) build and launch ([this pr](facebook/react-native#35661) for react-native is required) - [x] rn 0.71-rc4 project (use_frameworks!) build and launch
|
Something feels off, to me.
I'll try to explore better why we have the
I don't think I have time to try the repro in these days, but I'll give it a shot. I also created these guides in these days, that works with Swift Modules, and I have not encountered any of these problems.
So, I'm even more inclined to think that there could be a problem in the setup more than in React Native. |
since main.m is resident in app project, you should update the app's project.pbxproj. those cocoapods post_install handlers don't update this. that's why compiling for main.m doesn't have
these examples are not complicated enough 😑. you need both objc -> swift and swift -> objc to trigger the problem. that's why i have a swift class inherits objc class in my repro. feel free to visit this whenever you get a chance (maybe) next year. i could workaround the problem internally before that. if you have any questions, either replying here or ping me on discord. i can help to clarify the questions for you. |
|
This is a new application created with the template, and with the pods installed with As you can see, the main app has the flag properly set in the I made another test, stepping into the flags in the I tried to set the I think that the root problem is that I have to think about this, because I don't like that solution, tbh. It looks like that |
oh yes you are right. since main.m is objc, so the compile flag should be "other c flags" ( |
|
What happens if we put the |
if we add |
|
I investigated this a little bit more. I think that the right fix is to forward declare the class that are needed and use import them in the implementation file only. So, the changes are:
#import <UIKit/UIKit.h>
#if RCT_NEW_ARCH_ENABLED
// When the new architecture is enabled, the RCTAppDelegate imports some additional headers
- #import <React/RCTCxxBridgeDelegate.h>
- #import <React/RCTSurfacePresenterBridgeAdapter.h>
- #import <ReactCommon/RCTTurboModuleManager.h>
+ @class RCTSurfacePresenterBridgeAdapter;
+ @class RCTTurboModuleManager;
#endif
// ...
#if RCT_NEW_ARCH_ENABLED
/// Extension that makes the RCTAppDelegate conform to New Architecture delegates
- @interface RCTAppDelegate () <RCTTurboModuleManagerDelegate, RCTCxxBridgeDelegate>
+ @interface RCTAppDelegate ()
#if RCT_NEW_ARCH_ENABLED
+ #import <React/RCTCxxBridgeDelegate.h>
+ #import <React/RCTSurfacePresenterBridgeAdapter.h>
+ #import <ReactCommon/RCTTurboModuleManager.h>
#import <React/CoreModulesPlugins.h>
#import <React/RCTFabricSurfaceHostingProxyRootView.h>
#import <React/RCTSurfacePresenter.h>
#import <react/config/ReactNativeConfig.h>
static NSString *const kRNConcurrentRoot = @"concurrentRoot";
- @interface RCTAppDelegate () {
+ @interface RCTAppDelegate () <RCTTurboModuleManagerDelegate, RCTCxxBridgeDelegate> {
std::shared_ptr<const facebook::react::ReactNativeConfig> _reactNativeConfig;
facebook::react::ContextContainer::Shared _contextContainer;
}
@end
#endifIn this way, the header file does not have to import any C++ class, because they are limited to the implementation file, which is already, correctly, a Could you give these changes a try in your setup? If they work, we can try and bring them in 0.71.0/0.71.1. |
|
it's a clever idea and works like a charm. thanks @cipolleschi! |
sdk 48 may come next year and react-native 0.71 may be released this year. it's good to have react-native 0.71 support for our modules. if users want to upgrade 0.71, they could change their project to be a bare project for the early preview. - [core][av][gl] because 0.71 doesn't serve android aar along with npm package, the aar extraction from `node_modules/react-native/android/**/*.aar` will break on 0.71. 0.71 also ships modules with prefab support. it's good to rewrite the gradle/cmake files to link with prefab modules. that would reduce much complexity. however, to be backward compatible with 0.70, i moved most logic to `ExpoModulesCorePlugin.applyLegacyReactNativeLibsExtractionPlugin` and `legacy/CMakeLists.txt`. so that we could be backward compatible and easily remove the isolated code after we drop sdk 47 support. - [dev-launcher] add 0.71 sources because new parameter to `DevSupportManagerBase` - [core][dev-menu] fix ios build errors for jsc because 0.71 moved the header to `<React-jsc/JSCRuntime.h>` - [core][autolinking] integrate the `RCTAppDelegate` with our `EXAppDelegateWrapper`. that would simply the install-expo-modules migration where we only need the change `RCTAppDelegate -> EXAppDelegateWrapper` in AppDelegate.h. however, integrate RCTAppDelegate comes with some issues from expo-modules-core. [this commit](2959477) changes many code and i'll try to explain here: - RCTAppDelegate is in `React-RCTAppDelegate` pod, which does not define module. we define module for it in our patch system in autolinking. - defines `RCT_NEW_ARCH_ENABLED` as RCTAppDelegate - in new architecture mode, RCTAppDelegate comes with [more cxx dependencies](https://github.com/facebook/react-native/blob/03b17d9af7e4e3ad3f9ec078b76d0ffa33a3290e/Libraries/AppDelegate/RCTAppDelegate.h#L12-L18) for fabric. that's why we should add more header search paths and also move to EXAppDelegateWrapper.mm. - [x] ci passed - [x] bare-expo build and launch - [x] bare-expo nightlies build and launch - [x] fabric-tester build and launch - [x] fabric-tester nighties build and launch (should apply [the patch to fabric-tester](https://gist.github.com/Kudo/417a5159cb01a400ecee22ad4985d287)) - [x] rn 0.71-rc4 project build and launch (could apply [the patches](https://gist.github.com/Kudo/7448c733f12e700c7fbed76251fa3553) for testing) - [x] rn 0.71-rc4 project (new architecture) build and launch ([this pr](facebook/react-native#35661) for react-native is required) - [x] rn 0.71-rc4 project (use_frameworks!) build and launch (cherry picked from commit d4cd4df)
|
I'm glad to hear that! Would you update this PR with the suggested changes, so that I can import and land it? Or should I do it myself? |
This reverts commit 614dab4.
Co-Authored-By: Riccardo Cipolleschi <cipolleschi@meta.com>
Kudo
force-pushed
the
rctappdelegate-swift
branch
from
614dab4
to
097ce3b
Compare
|
@cipolleschi has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
|
PR build artifact for 097ce3b is ready. |
|
thanks @cipolleschi for the great idea and help. i've tested old architecture on my end and it works well. also the ios circle ci jobs look fine but those android ci jobs break at unstable sonatype maven server. please let me know whatever i can help. |
|
@cipolleschi merged this pull request in 5eb25d2. |
|
thanks for the great help 👍 |
) Summary: When inheriting `RCTAppDelegate` in a module with swift code, the compiler will have a build error when it goes through module headers. because swift does not support cxx headers. we found this issue when we try to inherit the class at Expo's [`EXAppDelegateWrapper`](https://github.com/expo/expo/blob/main/packages/expo-modules-core/ios/AppDelegates/EXAppDelegateWrapper.h) with RCTAppDelegate in new architecture mode. ## Changelog [IOS][FIXED] - Fix build errors when inheriting RCTAppDelegate in Swift modules Pull Request resolved: #35661 Test Plan: - ci passed - tested with expo's setup: expo/expo#20470 Reviewed By: rshest Differential Revision: D42293851 Pulled By: cipolleschi fbshipit-source-id: 8a173279db070cc0008c6f8214093951f504dcc1
…ebook#35661) Summary: When inheriting `RCTAppDelegate` in a module with swift code, the compiler will have a build error when it goes through module headers. because swift does not support cxx headers. we found this issue when we try to inherit the class at Expo's [`EXAppDelegateWrapper`](https://github.com/expo/expo/blob/main/packages/expo-modules-core/ios/AppDelegates/EXAppDelegateWrapper.h) with RCTAppDelegate in new architecture mode. ## Changelog [IOS][FIXED] - Fix build errors when inheriting RCTAppDelegate in Swift modules Pull Request resolved: facebook#35661 Test Plan: - ci passed - tested with expo's setup: expo/expo#20470 Reviewed By: rshest Differential Revision: D42293851 Pulled By: cipolleschi fbshipit-source-id: 8a173279db070cc0008c6f8214093951f504dcc1
Summary
When inheriting
RCTAppDelegatein a module with swift code, the compiler will have a build error when it goes through module headers. because swift does not support cxx headers. we found this issue when we try to inherit the class at Expo'sEXAppDelegateWrapperwith RCTAppDelegate in new architecture mode.Changelog
[IOS][FIXED] - Fix build errors when inheriting RCTAppDelegate in Swift modules
Test Plan