Chore: migrate repack native modules to turbo modules #344

teneeto · 2023-04-17T07:12:40Z

Summary

This PR aims to enable newArch and make re.pack native modules to turbo modules while ensuring newArch is implemented with backward compatibility.

re.pack current implementation directly uses RN native modules, but with this change, there is a SPEC implementation that allows for more efficient communication between native and JavaScript. This change allows for both old and new architecture.

Test plan

Make sure you are currently using the newArch and the setup conforms to React Native Doc directives which also means you're on RN>=0.68.0

NOTE: TesterApp currently meets this requirement if you're using repack TesterApp.

For Android

You will only need to update your android/gradle.properties file as follows:
newArchEnabled=true
and then yarn TesterApp:start

For iOS

Run pod install with the flags:
RCT_NEW_ARCH_ENABLED=1 pod install
and then yarn TesterApp:start

Screen Records

newArch - iOS

Screen.Recording.2023-04-17.at.13.00.01.mov

oldArch - iOS

Screen.Recording.2023-04-17.at.13.13.11.mov

newArch - Android

Screen.Recording.2023-04-17.at.13.39.30.mov

oldArch - android

Screen.Recording.2023-04-17.at.13.26.39.mov

…w and old arch

changeset-bot · 2023-04-17T07:12:43Z

⚠️ No Changeset found

Latest commit: 5f77968

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

README.md

packages/repack/android/src/newarch/java/com/callstack/repack/ChunkManagerModule.kt

packages/repack/ios/ScriptManager.mm

packages/repack/js/index.ts

packages/repack/js/NativeScriptManager.ts

packages/repack/tsconfig.json

packages/repack/js/NativeScriptManager.ts

troZee · 2023-04-21T11:06:53Z

Overall it looks good 🎉 I have added a few comments

cc @thymikee @jbinda

troZee · 2023-04-21T11:29:02Z

packages/repack/android/src/main/java/com/callstack/repack/ChunkManagerModuleImpl.kt

+
+    }
+
+    fun loadScript(scriptId: String, configMap: ReadableMap, promise: Promise, remoteLoader: RemoteScriptLoader, fileSystemLoader: FileSystemScriptLoader) {


[nit]: as an improvement, we can consider rewriting it to follow the bob pattern:
https://github.com/callstack/react-native-builder-bob/tree/main/packages/create-react-native-library/templates/java-library-mixed/android/src

cc @thymikee

README.md

packages/repack/android/src/newarch/java/com/callstack/repack/ChunkManagerModule.kt

packages/repack/android/src/main/java/com/callstack/repack/ChunkManagerPackage.kt

packages/repack/js/NativeScriptManager.ts

…-repack-to-turbo-modules

…llstack/repack into chore/migrate-repack-to-turbo-modules

RafikiTiki

I don't have much experience when it comes to New Architecture so if @troZee says it's looking good from the configuration perspective I trust his judgment :)

However, one thing that I don't like is that the native implementation of the prefetchScript, loadScript, and invalidateScripts methods is duplicated, thus making the maintenance cost higher in the future. Is this expected when writing backward-compatible modules? That would be a weird design TBH.

RafikiTiki · 2023-08-03T17:20:18Z

packages/repack/android/src/main/java/com/callstack/repack/ChunkManagerPackage.kt

+                        ScriptManagerModuleImpl.NAME, // name
+                        ScriptManagerModuleImpl.NAME, // className
+                        false, // canOverrideExistingModule
+                        false, // needsEagerInit


I don't know what exactly needsEagerInit means, but I'd say Repack should be initialized & available as soon as possible, especially in the Module Federation architecture we're now treating as a recommended one.

Take a look at the Super App Showcase Host App – https://github.com/callstack/super-app-showcase/blob/main/packages/host/index.js ScriptManager is used before the App component is registered.

Of course, what I'm saying here might be completely wrong since I'm not that familiar with the New Arch – if that's the case, sorry for the confusion :)

On repack side, i'm not sure how eager it needs to be initialised (even though i feel it's still initialised and available), however this is the default spec from docs and i'm sure it can be set to Eagerly initialise.

README.md

teneeto · 2023-08-16T14:01:57Z

I don't have much experience when it comes to New Architecture so if @troZee says it's looking good from the configuration perspective I trust his judgment :)

However, one thing that I don't like is that the native implementation of the prefetchScript, loadScript, and invalidateScripts methods is duplicated, thus making the maintenance cost higher in the future. Is this expected when writing backward-compatible modules? That would be a weird design TBH.

Yes, that's the implementation for backward compatibility. There's a common module with the actual implementation of prefetchScript, loadScript, and invalidateScripts within the ChunkManagerModuleImpl.kt file. The only difference between the old and new arch is how these methods are exposed, where the new arch uses override for methods, the old arch uses the @ReactMethod. And that's the only time we use the old or new arch files.

github-actions · 2023-12-22T00:11:50Z

This pull request has been marked as stale because it has been inactive for 60 days. Please update this pull request or it will be automatically closed in 14 days.

karimb11 · 2024-01-06T10:49:51Z

Hi, any ETA on this improvement? Thanks!

jbroma · 2024-02-01T18:08:49Z

Closing in favour of #495

jbroma · 2024-02-01T23:07:11Z

Thank you @teneeto for your all work on this PR 🙏

teneeto added 6 commits April 17, 2023 07:16

chore: add and use spec for ScriptManager

4b9a9df

chore: setup codegen to generate stubs using NativeScriptManagerSpec

e68eb4d

chore: configure podspec to selectively use old and new arch

46e4f1c

chore: enable newarch support for iOS and implement stubs

80a85c8

chore: configure android build.gradle to support new and old arch

f74ed2a

chore: enable newarch support for android and implement module for ne…

bae8035

…w and old arch

teneeto added 2 commits April 17, 2023 08:25

chore: fix lint

f27ae60

chore: fix lint - delete whitespace

742f9fb

teneeto force-pushed the chore/migrate-repack-to-turbo-modules branch from 4ed60f4 to 742f9fb Compare April 17, 2023 10:19

teneeto added 2 commits April 20, 2023 02:03

chore: mock test using NativeScriptManagerSpec

1e18f7d

chore: new arch guide

391900f

troZee self-requested a review April 21, 2023 10:38

troZee requested changes Apr 21, 2023

View reviewed changes

troZee reviewed Apr 21, 2023

View reviewed changes

packages/repack/js/NativeScriptManager.ts Show resolved Hide resolved

troZee reviewed Apr 21, 2023

View reviewed changes

chore: review fixes

5f345dc

okwasniewski reviewed Apr 24, 2023

View reviewed changes

README.md Show resolved Hide resolved

okwasniewski reviewed Apr 24, 2023

View reviewed changes

packages/repack/android/src/newarch/java/com/callstack/repack/ChunkManagerModule.kt Outdated Show resolved Hide resolved

okwasniewski reviewed Apr 24, 2023

View reviewed changes

packages/repack/android/src/main/java/com/callstack/repack/ChunkManagerPackage.kt Outdated Show resolved Hide resolved

teneeto added 7 commits April 24, 2023 13:10

chore: fix code style - update shared implementation to use object

6c382d4

Merge branch 'main' into chore/migrate-repack-to-turbo-modules

626c32c

chore: remove js included path on repack tsconfig

4d26acc

chore: add proper types to NativeScriptManager.ts

4a09696

Merge branch 'main' into chore/migrate-repack-to-turbo-modules

839de63

chore: update yarn.lock

ba282bf

Merge branch 'main' into chore/migrate-repack-to-turbo-modules

9700c52

RafikiTiki reviewed Apr 25, 2023

View reviewed changes

packages/repack/js/NativeScriptManager.ts Outdated Show resolved Hide resolved

RafikiTiki reviewed Apr 25, 2023

View reviewed changes

packages/repack/js/NativeScriptManager.ts Outdated Show resolved Hide resolved

RafikiTiki reviewed Apr 25, 2023

View reviewed changes

packages/repack/js/NativeScriptManager.ts Outdated Show resolved Hide resolved

teneeto added 5 commits April 26, 2023 09:22

Merge branch 'main' into chore/migrate-repack-to-turbo-modules

3740e24

update ScriptLocator types for Spec

f36769a

revert to using Object as config types

ae164e4

Merge branch 'main' into chore/migrate-repack-to-turbo-modules

8be4291

Merge branch 'main' into chore/migrate-repack-to-turbo-modules

ef926f4

thymikee mentioned this pull request May 30, 2023

Support new React Native new architecture (TurboModules/Fabric) #259

Closed

thymikee linked an issue May 30, 2023 that may be closed by this pull request

Support new React Native new architecture (TurboModules/Fabric) #259

Closed

teneeto added 4 commits June 14, 2023 13:02

Merge branch 'main' into chore/migrate-repack-to-turbo-modules

de5f952

Merge branch 'main' of github.com:callstack/repack into chore/migrate…

154be1f

…-repack-to-turbo-modules

Merge branch 'chore/migrate-repack-to-turbo-modules' of github.com:ca…

98287ae

…llstack/repack into chore/migrate-repack-to-turbo-modules

Merge branch 'main' into chore/migrate-repack-to-turbo-modules

5c68783

RafikiTiki reviewed Aug 3, 2023

View reviewed changes

teneeto added 3 commits August 16, 2023 15:02

Merge branch 'main' into chore/migrate-repack-to-turbo-modules

71199f8

remove irrelevant readme section

a585956

Merge branch 'main' into chore/migrate-repack-to-turbo-modules

31e119f

github-actions bot added the Stale label Dec 22, 2023

github-actions bot removed the Stale label Jan 7, 2024

Merge branch 'main' into chore/migrate-repack-to-turbo-modules

5f77968

This was referenced Jan 31, 2024

[RFC] sync API #493

Closed

Improve types for prefetchScript and loadScript #494

Closed

jbroma closed this Feb 1, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Chore: migrate repack native modules to turbo modules #344

Chore: migrate repack native modules to turbo modules #344

teneeto commented Apr 17, 2023 •

edited

Loading

changeset-bot bot commented Apr 17, 2023 •

edited

Loading

troZee commented Apr 21, 2023

troZee Apr 21, 2023

RafikiTiki left a comment

RafikiTiki Aug 3, 2023

teneeto Aug 16, 2023

teneeto commented Aug 16, 2023

github-actions bot commented Dec 22, 2023

karimb11 commented Jan 6, 2024

jbroma commented Feb 1, 2024

jbroma commented Feb 1, 2024


		}

		fun loadScript(scriptId: String, configMap: ReadableMap, promise: Promise, remoteLoader: RemoteScriptLoader, fileSystemLoader: FileSystemScriptLoader) {

Chore: migrate repack native modules to turbo modules #344

Chore: migrate repack native modules to turbo modules #344

Conversation

teneeto commented Apr 17, 2023 • edited Loading

Summary

Test plan

For Android

For iOS

Screen Records

changeset-bot bot commented Apr 17, 2023 • edited Loading

⚠️ No Changeset found

troZee commented Apr 21, 2023

troZee Apr 21, 2023

Choose a reason for hiding this comment

RafikiTiki left a comment

Choose a reason for hiding this comment

RafikiTiki Aug 3, 2023

Choose a reason for hiding this comment

teneeto Aug 16, 2023

Choose a reason for hiding this comment

teneeto commented Aug 16, 2023

github-actions bot commented Dec 22, 2023

karimb11 commented Jan 6, 2024

jbroma commented Feb 1, 2024

jbroma commented Feb 1, 2024

teneeto commented Apr 17, 2023 •

edited

Loading

changeset-bot bot commented Apr 17, 2023 •

edited

Loading