Convert library to TurboModule #910

j-piasecki · 2023-02-06T15:38:51Z

PR adding New Architecture support to the library 🎉

We at Software Mansion have been working on improving support for the new architecture for quite a while now. If you need help with anything related to New Architecture, like:

or you just want to ask any questions, hit us up on [email protected]

Summary

This PR adds FabricExample app with the new arch enabled and converts the library to TurboModule.

Notable changes:

renamed Android module from RNC_AsyncSQLiteDBStorage to RNCAsyncStorage, matching iOS name
split the Package file into two: one for the Java implementation, one for the Kotlin one. This is mainly because of @ReactModuleList annotation. Previously reflection and BuildConfig field were used to check if the Kotlin module should be used, but the annotation expects static declaration. I've achieved this using gradle source sets, to conditionally include one of the modules.
the generated spec is committed to the repository and used on the old architecture, which simplifies the inheritance. In case the spec changes, this file would need to be upgraded

Test Plan

Check the example app. On Android the new arch is enabled by default, while on iOS the RCT_NEW_ARCH_ENABLED=1 flag is required when installing pods.

github-actions · 2023-04-11T02:12:30Z

This PR has been marked as stale due to inactivity. Please address any comments within 7 days or it will be closed.

# Conflicts: # android/build.gradle # package.json # react-native.config.js # src/RCTAsyncStorage.ts # yarn.lock

shwanton · 2023-06-14T05:16:19Z

Hi Jakub,

I have a branch with the iOS Turbomodule working, based on this PR: https://github.com/shwanton/async-storage/tree/%40shwanton/turbomodule-ios-working

CleanShot.2023-06-13.at.22.17.12.mp4

~~I plan to get the macOS TM working too since macOS now supports TM's & Fabric.~~
Got macOS working here: shwanton@6d2faaa

I'm not super familiar w/ Android TM's, what is the current state?

Would love to get this PR working so we can use this lib as Turbomodule!

j-piasecki · 2023-06-14T09:39:44Z

As far as I remember, both Android and iOS were ready to be merged at the time of submitting the PR. You can check if the Android version works and if it doesn't I will look into fixing it 😄.

As a side note, what is the reason behind this change? In my opinion tmSpecs is less descriptive than rnasyncstorage as it doesn't in any way point to the library (in case there are any errors during codegen or compilation).

shwanton · 2023-06-20T23:20:34Z

example/android/gradle.properties

@@ -36,7 +36,7 @@ android.enableJetifier=true

 # Enable new architecture, i.e. Fabric + TurboModule - implies USE_FABRIC=1.
 # Note that this is incompatible with web debugging.
-#newArchEnabled=true
+newArchEnabled=true


Do we want all examples to be newArchEnabled?

shwanton · 2023-06-20T23:23:47Z

example/macos/Podfile

+  :hermes_enabled => false,
+  :turbomodule_enabled => true,
+}
+


The podspec requires the RCT_NEW_ARCH_ENABLED be passed to pod install. Enabling these options w/o the flag will not enable new arch.

https://github.com/j-piasecki/react-native-async-storage/blob/8a47c53c974c666419a290ff94b6f3548e797c5e/RNCAsyncStorage.podspec#L5

Edit: I misread. Let me check again. Will update this post.
Edit 2: I still can't repro this.

Are you sure about this? I've just tested in master and it seems to be working?

diff --git a/RNCAsyncStorage.podspec b/RNCAsyncStorage.podspec index 5d74402..45be872 100644 --- a/RNCAsyncStorage.podspec +++ b/RNCAsyncStorage.podspec @@ -2,6 +2,10 @@ require 'json' package = JSON.parse(File.read(File.join(__dir__, 'package.json'))) +p '%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%' +p ENV['RCT_NEW_ARCH_ENABLED'] +p '%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%' + Pod::Spec.new do |s| s.name = "RNCAsyncStorage" s.version = package['version'] diff --git a/example/macos/Podfile b/example/macos/Podfile index 19f6452..0eda101 100644 --- a/example/macos/Podfile +++ b/example/macos/Podfile @@ -2,7 +2,13 @@ require_relative '../../node_modules/react-native-test-app/macos/test_app.rb' workspace 'AsyncStorageExample.xcworkspace' -use_test_app! do |target| +options = { + :fabric_enabled => true, + :hermes_enabled => false, + :turbomodule_enabled => true, +} + +use_test_app! options do |target| target.app do pod 'RNCAsyncStorage', :path => '../..' pod 'AsyncStorageExample', :path => '..'

Output:

% pod install ... Auto-linking React Native module for target `ReactTestApp`: ReactTestApp-DevSupport Analyzing dependencies "%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%" "1" "%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%" Downloading dependencies Generating Pods project Setting REACT_NATIVE build settings Setting CLANG_CXX_LANGUAGE_STANDARD to c++17 on /Users/tido/Source/async-storage/node_modules/.generated/macos/ReactTestApp.xcodeproj Pod install took 11 [s] to run

Hmm, when I put the log inside of the Pod::Spec block, it returns 0 for macOS

pod install --verbose ... -> Fetching podspec for `RNCAsyncStorage` from `../..` "%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%" "0" "%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%"

diff --git a/RNCAsyncStorage.podspec b/RNCAsyncStorage.podspec index 25c9420..502ae48 100644 --- a/RNCAsyncStorage.podspec +++ b/RNCAsyncStorage.podspec @@ -16,6 +16,10 @@ Pod::Spec.new do |s| s.source = { :git => "https://github.com/react-native-async-storage/async-storage.git", :tag => "v#{s.version}" } s.source_files = "ios/**/*.{h,m,mm}" + p '%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%' + p ENV['RCT_NEW_ARCH_ENABLED'] + p '%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%' + if fabric_enabled folly_compiler_flags = '-DFOLLY_NO_CONFIG -DFOLLY_MOBILE=1 -DFOLLY_USE_LIBCPP=1 -Wno-comma -Wno-shorten-64-to-32' diff --git a/example/macos/Podfile b/example/macos/Podfile index d25aa3d..a79620d 100644 --- a/example/macos/Podfile +++ b/example/macos/Podfile @@ -3,9 +3,9 @@ require_relative '../../node_modules/react-native-test-app/macos/test_app.rb' workspace 'AsyncStorageExample.xcworkspace' options = { - :fabric_enabled => false, + :fabric_enabled => true, :hermes_enabled => false, - :turbomodule_enabled => false, + :turbomodule_enabled => true, } use_test_app! do |target|

Same Podspec when run from iOS project:

pod install --verbose ... -> Fetching podspec for `RNCAsyncStorage` from `../..` "%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%" "1" "%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%"

I've just cleaned out my clone in case I had something stale. Still getting the same results.

Not a blocker, I can just pass the flag when I install.

tido64 · 2023-06-21T09:10:15Z

FYI: pod install --project-directory=… current fails: facebook/react-native#37992

RNCAsyncStorage.podspec

android/build.gradle

example/android/build.gradle

package.json

src/RCTAsyncStorage.ts

Co-authored-by: Tommy Nguyen <[email protected]>

Summary: ![image](https://user-images.githubusercontent.com/4123478/217618490-4f99e408-1a07-4acf-a05c-e8837562a931.png) `pod install --project-directory=ios` currently fails when new arch is enabled: react-native-async-storage/async-storage#910 - Patch for 0.71: #37993 - Patch for 0.72: #37994 ## Changelog: [IOS] [FIXED] - Fix `pod install --project-directory=...` when New Arch is enabled Pull Request resolved: #37992 Test Plan: ``` git clone https://github.com/react-native-async-storage/async-storage.git cd async-storage gh pr checkout 910 yarn RCT_NEW_ARCH_ENABLED=1 pod install --project-directory=example/ios ``` Reviewed By: cortinico Differential Revision: D46966225 Pulled By: cipolleschi fbshipit-source-id: 00b4650189175c09c9ec928f85d075890ba4c7c1

# Conflicts: # yarn.lock

shwanton · 2023-06-26T16:50:29Z

I merged master on my branch and TM worked great on iOS/macOS. Any other blockers to merging?

tido64

The iOS bits look fine to me. It'd be great if you could get @krizzu to review the Android bits as well.

example/android/build.gradle

example/android/gradle.properties

krizzu · 2023-06-29T19:18:52Z

android/src/javaPackage/java/com/reactnativecommunity/asyncstorage/AsyncStoragePackage.java

+                AsyncStorageModule.class,
+        }
+)
+public class AsyncStoragePackage extends TurboReactPackage implements ViewManagerOnDemandReactPackage {


Do we need to implement ViewManagerOnDemandReactPackage ? Seems unnecessary, as we only provide functional component

You're right, I don't actually remember why I added that. Anyway, removed it in b84f11e.

RNCAsyncStorage.podspec

android/src/paper/java/com/reactnativecommunity/asyncstorage/NativeAsyncStorageModuleSpec.java

android/src/kotlinPackage/java/com/reactnativecommunity/asyncstorage/AsyncStoragePackage.kt

krizzu

If you don't mind changing paper to oldarch that'd be great, but not a blocker.
Great work on this 👏

tido64

Thank you both for adding TM support ❤️

AsyncStorageBot · 2023-07-03T06:22:57Z

🎉 This PR is included in version 1.19.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

WoLewicki · 2023-07-03T08:22:42Z

react-native.config.js

+        sourceDir: path.join('example', 'ios'),
+      },
+      macos: {
+        sourceDir: path.join('example', 'maacos'),


Looks like a typo. Does it work correctly on macos?

My bad, I'll forward fix & test.

j-piasecki added 10 commits February 7, 2023 11:36

Init FabricExample

be521e4

Use next implementation in the example app

1ede6e7

Add spec and configure codegen

125cee4

Setup new arch support

d7a1927

iOS implementation

466b93e

Android implementation (for java version)

874d715

Use java package when next is disabled

f284faf

Add kotlin package

851d817

Update kotlin module

1a5587a

Compatibility with the old arch

1a99729

j-piasecki force-pushed the @jpiasecki/turbomodule branch from a090679 to 1a99729 Compare February 7, 2023 10:44

j-piasecki mentioned this pull request Feb 7, 2023

Add FabricExample application with the new arch enabled #911

Closed

j-piasecki added 7 commits February 7, 2023 13:02

Don't look for module with old android name

c4757c0

Update dependencies, remove react-native-macos

db4da7b

Update react-native.config.js

0e6a420

Add workaround for codegened files not being included

d82c80f

Enable new arch in the example app

abed02f

Remove FabricExample

d2c56ef

Merge branch 'master' into @jpiasecki/turbomodule

6007b94

j-piasecki marked this pull request as ready for review February 9, 2023 10:02

j-piasecki added 2 commits February 9, 2023 11:39

Bump gradle version

a39a698

Rename variable

109011e

github-actions bot added the Stale label Apr 11, 2023

github-actions bot closed this Apr 19, 2023

Shawn Dempsey added 2 commits June 13, 2023 13:54

Merge remote-tracking branch 'origin/master' into @jpiasecki/turbomodule

c6be49f

# Conflicts: # android/build.gradle # package.json # react-native.config.js # src/RCTAsyncStorage.ts # yarn.lock

TM working on iOS

47eaff3

Turbomodule working on macOS

6d2faaa

shwanton reviewed Jun 20, 2023

View reviewed changes

tido64 mentioned this pull request Jun 21, 2023

fix(ios): fix pod install --project-directory=ios failing facebook/react-native#37992

Closed

This was referenced Jun 21, 2023

fix(ios): fix pod install --project-directory=ios failing facebook/react-native#37993

Merged

fix(ios): fix pod install --project-directory=ios failing facebook/react-native#37994

Merged

tido64 reviewed Jun 21, 2023

View reviewed changes

j-piasecki and others added 8 commits June 21, 2023 12:33

Update RNCAsyncStorage.podspec

509b4bc

Co-authored-by: Tommy Nguyen <[email protected]>

Remove devDependencies

a1cbaeb

Revert space

6f50030

Revert build.gradle changes

2b07b10

Don't add codegen directory manually

1169935

Use install_modules_dependencies

e6ed072

Remove support for RN <0.71 on Android

16b2165

Revert module resolution

7bcb08b

Merge branch 'master' into @shwanton/turbomodule-ios-working

31d0da2

# Conflicts: # yarn.lock

tido64 approved these changes Jun 27, 2023

View reviewed changes

example/android/build.gradle Outdated Show resolved Hide resolved

example/android/gradle.properties Show resolved Hide resolved

krizzu reviewed Jun 29, 2023

View reviewed changes

j-piasecki added 3 commits June 30, 2023 10:07

Update build.gradle and workflow

2e93938

Remove ViewManagerOnDemandReactPackage

b84f11e

Mention updating in comment

0d41f20

krizzu approved these changes Jun 30, 2023

View reviewed changes

Rename paper -> oldarch

f4edc00

tido64 approved these changes Jul 3, 2023

View reviewed changes

tido64 merged commit 86a7e90 into react-native-async-storage:master Jul 3, 2023

AsyncStorageBot added the released label Jul 3, 2023

WoLewicki reviewed Jul 3, 2023

View reviewed changes

shwanton mentioned this pull request Jul 3, 2023

Fix macOS example block in react-native-config #983

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Convert library to TurboModule #910

Convert library to TurboModule #910

j-piasecki commented Feb 6, 2023 •

edited

Loading

github-actions bot commented Apr 11, 2023

shwanton commented Jun 14, 2023 •

edited

Loading

j-piasecki commented Jun 14, 2023

shwanton Jun 20, 2023

shwanton Jun 20, 2023

tido64 Jun 21, 2023 •

edited

Loading

shwanton Jun 21, 2023

tido64 Jun 21, 2023

shwanton Jun 22, 2023

tido64 commented Jun 21, 2023 •

edited

Loading

shwanton commented Jun 26, 2023

tido64 left a comment

krizzu Jun 29, 2023

j-piasecki Jun 30, 2023

krizzu left a comment

tido64 left a comment

AsyncStorageBot commented Jul 3, 2023

WoLewicki Jul 3, 2023

shwanton Jul 3, 2023

tido64 Jul 3, 2023

Convert library to TurboModule #910

Convert library to TurboModule #910

Conversation

j-piasecki commented Feb 6, 2023 • edited Loading

PR adding New Architecture support to the library 🎉

Summary

Test Plan

github-actions bot commented Apr 11, 2023

shwanton commented Jun 14, 2023 • edited Loading

j-piasecki commented Jun 14, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tido64 Jun 21, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tido64 commented Jun 21, 2023 • edited Loading

shwanton commented Jun 26, 2023

tido64 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

krizzu left a comment

Choose a reason for hiding this comment

tido64 left a comment

Choose a reason for hiding this comment

AsyncStorageBot commented Jul 3, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

j-piasecki commented Feb 6, 2023 •

edited

Loading

shwanton commented Jun 14, 2023 •

edited

Loading

tido64 Jun 21, 2023 •

edited

Loading

tido64 commented Jun 21, 2023 •

edited

Loading