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

feat: migrate to new rn architecture #213

Open
wants to merge 20 commits into
base: develop
Choose a base branch
from

Conversation

edeckers
Copy link
Owner

@edeckers edeckers commented Apr 29, 2022

Progress

  • Support TurboModules on Android
  • Replace absolute file path from Android.mk with relative path
  • Support TurboModules on iOS
  • Keep backwards compatibility for Android with NativeModules on pre 0.68 apps
  • Keep backwards compatibility for iOS with NativeModules on pre 0.68 apps
  • Run instrumented Android tests for both NativeModules and TurboModules on PR
  • Add documentation

Notes

Some notes on my migration adventures for future reference and to hopefully benefit people struggling with the same issues.

TurboModuleRegistry.getEnforcing

Solved TurboModuleRegistry.getEnforcing Spec problem by loading the package like so:

# file: MainApplicationReactNativeHost.java

protected List<ReactPackage> getPackages() {
(...)
+  packages.add(new BlobCourierPackage());

Context:
Invariant Violation: TurboModuleRegistry.getEnforcing(...): 'BlobCourier' could not be found. Verify that a module by this name is registered in the native binary.

Notice that having packages.add in MainApplication::mReactNativeHost is not sufficient, because when working with TurboModules the app uses MainApplication::mNewArchitectureNativeHost.

And to my surprise, I did not need to add this line in order to get this minimal TurboModule app to work:
https://github.com/edeckers/RNNewArchitectureLibraries/tree/elydeckers/feature/add-android-app

I'm guessing this has something to do with autolinking not being configured correctly, might be worth investigating later on.

Enable TurboModule on iOS

https://medium.com/achieving-100ms/part-1-automatic-batching-w-react-native-fabric-react-18-438ad1282896

To enable Fabric on iOS, run RCT_NEW_ARCH_ENABLED=1 pod install in the ios folder.

Memory file not found iOS

Currently running into Lexical or Preprocessor Issue 'memory' file not found on RCTCxxBridgeDelegate.h even though I did set CLANG_CXX_LANGUAGE_STANDARD = "c++17" on the BloubCourierExample project and I renamed AppDelegate.m to AppDelegate.mm which enables mixing Objective-C and C++.

Same error, maybe related somehow:
facebook/react-native#31303

This seems to do the trick:

When I did the react-native upgrade command, the .pbxproj had to be updated manually, then, I forgot to change the source code sourcecode.c.objc on AppDelegate.mm reference to sourcecode.cpp.objcpp.
facebook/react-native#33606 (comment)

File must be compiled as Obj-C++

Currently I'm stuck on

# file RNBlobCourierSpec.h

This file must be compiled as Obj-C++. If you are importing it, you must change your file extension to .mm.

Things I tried to fix it to no avail:

  • Exclude from source_files in podspec and include in {private,public}_source_headers
  • Add to BlobCourier-Bridging-Header.h
  • Change file Type to C++ Header in Pod-BlobCourierExample

Exclude generated header from podspec

The must be compiled as Obj-C++ problem was caused by the inclusion of the generated headers in the podspec, which both causes conflicts because its a Swift library and because it the generated header should not be part of the library but of the app that depends on the library.

When I removed the generated header from the podspec, removed the pod inclusion of react-native-blob-courier from the app's Podfile and added a link dependency to the libary to the app's package.json instead everything worked. The header file is still generated but is part of the app's build instead of the library's build, as it should.

Information

@edeckers edeckers added the enhancement New feature or request label Apr 29, 2022
@edeckers edeckers self-assigned this Apr 29, 2022
@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@edeckers edeckers force-pushed the elydeckers/feature/migrate-to-new-architecture branch from 9528c47 to aeff337 Compare April 30, 2022 08:25
@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@edeckers edeckers force-pushed the elydeckers/feature/migrate-to-new-architecture branch from aeff337 to 6a56a0c Compare April 30, 2022 13:12
@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@edeckers
Copy link
Owner Author

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@edeckers edeckers force-pushed the elydeckers/feature/migrate-to-new-architecture branch from 088dcf3 to 92a801b Compare July 28, 2022 14:49
@github-actions
Copy link
Contributor

TypeScript Test Report

  1 files    1 suites   2s ⏱️
35 tests 35 ✔️ 0 💤 0 ❌

Results for commit 92a801b.

@github-actions
Copy link
Contributor

Android Unit Test Report

  1 files    1 suites   9s ⏱️
19 tests 19 ✔️ 0 💤 0 ❌

Results for commit 92a801b.

@github-actions
Copy link
Contributor

Android Instrumented Test Report

1 files  1 suites   1h 17m 56s ⏱️
8 tests 8 ✔️ 0 💤 0 ❌

Results for commit 92a801b.

@github-actions
Copy link
Contributor

iOS Test Report

  1 files    1 suites   11s ⏱️
15 tests 15 ✔️ 0 💤 0 ❌

Results for commit 92a801b.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
Planned
  
Awaiting triage
Development

Successfully merging this pull request may close these issues.

None yet

1 participant