Skip to content
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

[modules] Fix incompatible issue for react-native 0.71 #20470

Merged
merged 11 commits into from Dec 19, 2022
Merged

Conversation

Kudo
Copy link
Contributor

@Kudo Kudo commented Dec 14, 2022

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 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 for fabric. that's why we should add more header search paths and also move to EXAppDelegateWrapper.mm.

Test Plan

  • ci passed
  • bare-expo build and launch
  • bare-expo nightlies build and launch
  • fabric-tester build and launch
  • fabric-tester nighties build and launch (should apply the patch to fabric-tester)
  • rn 0.71-rc4 project build and launch (could apply the patches for testing)
  • rn 0.71-rc4 project (new architecture) build and launch (this pr for react-native is required)
  • rn 0.71-rc4 project (use_frameworks!) build and launch

Checklist

@expo-bot expo-bot added the bot: suggestions ExpoBot has some suggestions label Dec 14, 2022
Copy link
Member

@tsapeta tsapeta left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me so far 😉

Comment on lines +14 to +18
-DFOLLY_NO_CONFIG=1
-DFOLLY_HAVE_CLOCK_GETTIME=1
-DFOLLY_HAVE_MEMRCHR=1
-DFOLLY_USE_LIBCPP=1
-DFOLLY_MOBILE=1
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can use folly-flags.cmake from react-native node_modules.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tried to migrate this stuff but it bump the minimum cmake version to 3.13. i don't want to do much new changes for expo sdk 47 patch releases this time. so i'll add an internal task for me to migrate this for sdk 48.
thanks @tomekzaw for this awesome comment.

"${REACT_NATIVE_DIR}/ReactAndroid/src/main/jni/react/turbomodule"
"${REACT_NATIVE_DIR}/ReactAndroid/src/main/java/com/facebook/react/turbomodule/core/jni"
"${REACT_NATIVE_DIR}/ReactCommon"
"${REACT_NATIVE_DIR}/ReactCommon/callinvoker"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CallInvoker.h and other headers are available in ReactAndroid::runtimeexecutor prefab.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks @tomekzaw for the kind comment. i just don't want to introduce new linked libs to libexpo-av.so, as expo-av currently doesn't rely on runtimexecutor. although we can get the prefab include path through get_target_property, e.g.

get_target_property(INCLUDE_runtimexecutor
        ReactAndroid::runtimeexecutor
        INTERFACE_INCLUDE_DIRECTORIES)
target_include_directories(
        ${PACKAGE_NAME}
        PRIVATE
        ${INCLUDE_runtimexecutor}
        # ...
)

since we only use the single header, it doesn't hurt to reference the header directly from react-native folder.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @Kudo for the explanation! I really like the idea of get_target_property, gonna try it in Reanimated! 😄

Kudo and others added 3 commits December 16, 2022 14:35
Co-authored-by: Tomasz Sapeta <tsapeta@users.noreply.github.com>
@Kudo Kudo merged commit d4cd4df into main Dec 19, 2022
@Kudo Kudo deleted the @kudo/rn071-support branch December 19, 2022 15:19
Kudo added a commit that referenced this pull request Dec 20, 2022
# Why

fix further regressions from #20470

# How

- bare-expo is still running with legacy AppDelegate where it creates
bridge, rootView in AppDelegate.mm. even running with 0.71, the
EXAppDelegateWrapper should handle this case and not create the bridge
from RCTAppDelegate.
- workaround a build error when running with new arch mode

# Test Plan

ci passed
Kudo added a commit that referenced this pull request Dec 20, 2022
# Why

fix react native nightlies testing

# How

based on #20470 to further fix other breaking changes
- update base-expo android template because the legacy react.gradle is removed in nightlies.
- move reanimated transform to patch-based transform
- [asset] add `ReactNativeCompatibleAssetsRegistry` because `@react-native/assets` is renamed to `@react-native/assets-registry`

# Test Plan

nightlies ci passed: https://github.com/expo/expo/actions/runs/3737521209
Kudo added a commit that referenced this pull request Dec 21, 2022
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)
Kudo added a commit that referenced this pull request Dec 21, 2022
# Why

fix further regressions from #20470

# How

- bare-expo is still running with legacy AppDelegate where it creates
bridge, rootView in AppDelegate.mm. even running with 0.71, the
EXAppDelegateWrapper should handle this case and not create the bridge
from RCTAppDelegate.
- workaround a build error when running with new arch mode

# Test Plan

ci passed

(cherry picked from commit 4ef7fe1)
facebook-github-bot pushed a commit to facebook/react-native that referenced this pull request Dec 30, 2022
)

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
Kudo added a commit that referenced this pull request Jan 6, 2023
# Why

fix #20679 which is a regression from #20470

# How

preBuild task could be executed after cmake. we should rely on `externalNative` or `CMake` tasks. this pr copies the `nativeBuildDependsOn` from `ExpoModulesCorePlugin.gradle` and setup `prepareBoost` dependency by the `nativeBuildDependsOn`.

# Test Plan

- test the reproducible example provided by #20679 
- ci passed
kelset pushed a commit to facebook/react-native that referenced this pull request Jan 19, 2023
)

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
Kudo added a commit that referenced this pull request Jan 31, 2023
# Why

follow up #20470 (comment)
close ENG-7083

# How

reference folly build flags from `folly-flags.cmake` shipped in react-native 0.71 

# Test Plan

ci passed
OlimpiaZurek pushed a commit to OlimpiaZurek/react-native that referenced this pull request May 22, 2023
…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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bot: passed checks ExpoBot has nothing to complain about
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants