Support for bridgeless NativeModule in New Architecture #194

evan-masseau · 2025-01-30T22:18:07Z

Description

This aims to add android support for the New Architecture by narrowing our current implementation to be exclusively a NativeModule that can run in bridgeless mode in new arch apps. I believe we had already stripped turbo module codegen from iOS in an earlier issue when we took out some podspec flags that were causing issues for customers.

This will be a stepping stone toward proper TurboModule support. Since that migration may take longer, this can unblock folks who need new arch ASAP.

Check List

Are you changing anything with the public API? -- NO
Are your changes backwards compatible with previous SDK Versions? -- So far yes, pending full regresssion testing

Changelog / Code Overview

In our initial commit we selected a template intended to support Native and Turbo Modules. Since we didn't fully implement TurboModules for version 1.0, the migration to New Architecture in RN 0.76.0 uncovered an issue for us: codegen was still running and erroring out on Android builds. To remedy this, I referenced the latest NativeModule project template to ensure we are up to date with best practices for making a NativeModule available on the New Architecture and upgraded the example app to 0.77.0 to catch up to the now-default architecture for new apps. Since we're still a Native Module, old arch support should be unaffected.

Test Plan

Until I publish an alpha to npm, this version can be installed with:
npm install [email protected]:klaviyo/klaviyo-react-native-sdk.git#ecm/bridgeless

Test plans:

Github CI covers example app compiling and building on each platform, and will now be defaulting to new arch.
- It would be great if the CI could run against new and old arch, gotta look at the build scripts.
Test against the bug repo provided in issue Compilation error for Android on latest React Native 0.76 with new architecture enabled #185 for compilation
Manual tests of example app to confirm all SDK constants, setters, getters etc are bridging correctly (in progress)
Migrate internal test app to new architecture -- note this requires more package updates since we rely on more 3rd party libs for that app (in progress)

Related Issues/Tickets

#185

…s edition)

evan-masseau · 2025-01-31T17:04:32Z

.husky/pre-commit

@@ -1,9 +1,57 @@
-#!/usr/bin/env sh
-. "$(dirname -- "$0")/_/husky.sh"
+#!/bin/sh


This was from the new project template, it moves our git hooks to lefthook.yml. I don't have a strong opinion, but just figured I'd adopt it while we're updating

evan-masseau · 2025-01-31T17:05:10Z

LICENSE

@@ -1,6 +1,6 @@
 MIT License

-Copyright (c) 2023 Kenny Tsui
+Copyright (c) 2025 Klaviyo


Autogenerated by the template, no offense Kenny <3

evan-masseau · 2025-01-31T17:06:09Z

android/build.gradle

@@ -79,20 +83,6 @@ android {
    sourceCompatibility JavaVersion.VERSION_1_8
    targetCompatibility JavaVersion.VERSION_1_8
  }
-


These gradle changes were necessary yet not sufficient for removing the turbomodule code gen from our project for now. When we do add TM support, they'll have to come back

android/gradle/wrapper/gradle-wrapper.properties

evan-masseau · 2025-01-31T17:07:13Z

android/build.gradle

@@ -14,6 +14,11 @@ buildscript {
  }
 }

+def reactNativeArchitectures() {


This just came from the template

evan-masseau · 2025-01-31T17:20:35Z

lefthook.yml

-#  commands:
-#    commitlint:
-#      run: npx commitlint --edit
+pre-commit:


Our pre-commit logic lives here now, still installed via husky tho

evan-masseau · 2025-01-31T17:21:44Z

package.json

  "jest": {
    "preset": "react-native",
    "modulePathIgnorePatterns": [
      "<rootDir>/example/node_modules",
      "<rootDir>/lib/"
    ]
  },
-  "commitlint": {


we weren't using this, and it enforces a rather annoying commit message format that I don't expect our devs to follow, for the most part because we typically squash merge and format commit messages at that time.

evan-masseau · 2025-01-31T17:23:06Z

src/KlaviyoReactNativeSdk.ts

-// @ts-expect-error
-const isTurboModuleEnabled = global.__turboModuleProxy != null;
-
-const KlaviyoReactNativeSdkModule = isTurboModuleEnabled


This was supposed to be the basis of supporting NativeModules or TurboModules in the same lib. We'll probably have to revive this in the next phase so that we can maintain NM support when we add TM.

evan-masseau · 2025-01-31T17:23:47Z

turbo.json

@@ -2,6 +2,7 @@
  "$schema": "https://turbo.build/schema.json",
  "pipeline": {
    "build:android": {
+      "env": ["ORG_GRADLE_PROJECT_newArchEnabled"],


turbo build != turbo modules, but I did copy over this default env variable for new arch

what is turbo build? 😮‍💨

Turborepo is a high-performance build system for monorepos, allowing you to manage multiple packages within a single repository efficiently. It optimizes build processes by caching and parallelizing tasks, resulting in faster development builds and improved performance.

I guess there's only so many adjectives in the english language lol. This was something that really boosted our CI performance especially iOS (but also caused those caching woes when we first added the GH actions)

evan-masseau · 2025-01-31T17:27:12Z

src/Klaviyo.ts

@@ -1,5 +1,3 @@
-import { TurboModuleRegistry } from 'react-native';


Renaming this file was probably the least obvious yet most important change. Evidently the Native* prefix is detected by codegen. I would get a different error relating to CMake trying to build for libs that the package does not actually provide.
I had expected that renaming this, in combination with the build.gradle changes above, should have fixed our new arch code gen issues... but didn't. That's why I suspect there might have been an additional underlying bug in the RN/gradle libs but didn't have the time/bandwitdth to track it down myself. Besides which, this was a good time to update to latest RN version anyways so it wasn't really worth the effort.

android/gradle.properties

kennyklaviyo · 2025-01-31T18:45:38Z

android/gradle.properties

-android.enableJetifier=true
-android.useAndroidX=true


are these defaulted to true now? or incompatible with new arch

I was just noticing this as well. I'm not sure about jetifier, but we use androidX so that probably needs to come back

Looking back in git, it appears I added them pretty early on, but I'm not clear why. They weren't part of the original template, nor are they in the latest one. And I'm not sure why we'd need them, since the SDK's android files do not make use of androidx at all.
The example app does have useAndroidx=true which makes sense because it has androidx dependency for the activities and notifications and such.
So I'm thinking this is safe to remove.

ajaysubra

LGTM some minor comments/questions.

ajaysubra · 2025-01-31T18:45:08Z

.husky/pre-commit

-if [ -f "package-lock.json" ];
-  then echo "package-lock.json is not allowed";
-  exit 1;
+if [ "$LEFTHOOK" = "0" ]; then


Fast. It is written in Go. Can run commands in parallel.
Okay!

example/android/app/src/main/java/com/klaviyoreactnativesdkexample/MainApplication.kt

ajaysubra · 2025-01-31T18:49:43Z

example/android/build.gradle

-        ndkVersion = "25.1.8937393"
-        kotlinVersion = "1.8.0"
+        buildToolsVersion = "35.0.0"
+        minSdkVersion = 24


so on native we support 23 but for RN it's only 24 onwards? NBD but checking.

ajaysubra · 2025-01-31T18:50:23Z

example/android/gradle.properties

@@ -21,8 +21,6 @@ org.gradle.jvmargs=-Xmx2048m -XX:MaxMetaspaceSize=512m
 # Android operating system, and which are packaged with your app's APK
 # https://developer.android.com/topic/libraries/support-library/androidx-rn
 android.useAndroidX=true
-# Automatically convert third-party libraries to use AndroidX


any implication of removing this?

ajaysubra · 2025-01-31T18:52:02Z

example/ios/KlaviyoReactNativeSdkExample.xcodeproj/project.pbxproj

@@ -546,7 +546,7 @@
 					"$(inherited)",
 				);
 				INFOPLIST_FILE = KlaviyoReactNativeSdkExampleTests/Info.plist;
-				IPHONEOS_DEPLOYMENT_TARGET = 13.4;


I think same question as android, if someone where to update to these RN version they are letting go of lower iOS versions so it's not really us who is responsible for the higher min deployment target?

example/ios/KlaviyoReactNativeSdkExample.xcodeproj/project.pbxproj

example/package.json

package.json

ajaysubra · 2025-01-31T18:58:20Z

src/Klaviyo.ts

@@ -1,5 +1,3 @@
-import { TurboModuleRegistry } from 'react-native';


ajaysubra · 2025-01-31T18:59:39Z

turbo.json

@@ -2,6 +2,7 @@
  "$schema": "https://turbo.build/schema.json",
  "pipeline": {
    "build:android": {
+      "env": ["ORG_GRADLE_PROJECT_newArchEnabled"],


what is turbo build? 😮‍💨

android/gradle.properties

jordan-griffin · 2025-01-31T19:13:52Z

babel.config.js

@@ -1,3 +1,3 @@
 module.exports = {
-  presets: ['module:@react-native/babel-preset'],
+  presets: ['module:react-native-builder-bob/babel-preset'],


Is this necessary? Just confused on its relevance to this change

There were quite a number of changes irrelevant to the turbomodules / new arch fix itself, that just came along with updating to the latest RN Native Module library template. I'm not certain about this one, but agree it'd be good to read up just to stay up to date with RN practices.

https://callstack.github.io/react-native-builder-bob/faq

react-native-builder-bob wraps tools such as babel and typescript to simplify these common tasks across multiple projects. While it can be used for any library, it's primarily tailored to React Native projects to minimize the configuration required.

yup so just some build tools moving around.

example/android/app/build.gradle

example/metro.config.js

package.json

…is just a boilerplate RN readme.

Evan Masseau added 6 commits January 30, 2025 12:08

Updated from latest Old Arch NativeModule library template (full bean…

1a2adfb

…s edition)

Lets put a version on this

cfeb850

it must be called husky because it always has something to say

1ff8f07

Minor iOS adjustments related to the package updates

bf2fff0

Lost a package which caused pre-commit to skip linting

7222e79

Ah ok, it lives in lefthook now, sure.

ac39379

evan-masseau commented Jan 31, 2025

View reviewed changes

evan-masseau mentioned this pull request Jan 31, 2025

Compilation error for Android on latest React Native 0.76 with new architecture enabled #185

Open

4 tasks

evan-masseau marked this pull request as ready for review January 31, 2025 18:12

evan-masseau requested a review from a team as a code owner January 31, 2025 18:12

evan-masseau self-assigned this Jan 31, 2025

evan-masseau requested a review from ajaysubra January 31, 2025 18:23

kennyklaviyo reviewed Jan 31, 2025

View reviewed changes

android/gradle.properties Outdated Show resolved Hide resolved

kennyklaviyo reviewed Jan 31, 2025

View reviewed changes

ajaysubra approved these changes Jan 31, 2025

View reviewed changes

jordan-griffin reviewed Jan 31, 2025

View reviewed changes

android/gradle.properties Show resolved Hide resolved

jordan-griffin reviewed Jan 31, 2025

View reviewed changes

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

jordan-griffin reviewed Jan 31, 2025

View reviewed changes

example/metro.config.js Show resolved Hide resolved

jordan-griffin reviewed Jan 31, 2025

View reviewed changes

package.json Show resolved Hide resolved

PR feedback

da6de00

jordan-griffin approved these changes Jan 31, 2025

View reviewed changes

evan-masseau linked an issue Jan 31, 2025 that may be closed by this pull request

Compilation error for Android on latest React Native 0.76 with new architecture enabled #185

Open

4 tasks

We have instructions about the example app in the root dir, and this …

e7485df

…is just a boilerplate RN readme.

dan-peluso approved these changes Feb 4, 2025

View reviewed changes

ashiu95 merged commit e81662f into master Feb 7, 2025
9 checks passed

ashiu95 deleted the ecm/bridgeless branch February 7, 2025 14:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support for bridgeless NativeModule in New Architecture #194

Support for bridgeless NativeModule in New Architecture #194

evan-masseau commented Jan 30, 2025 •

edited

Loading

evan-masseau Jan 31, 2025

evan-masseau Jan 31, 2025

kennyklaviyo Jan 31, 2025

evan-masseau Jan 31, 2025

evan-masseau Jan 31, 2025

evan-masseau Jan 31, 2025

evan-masseau Jan 31, 2025

evan-masseau Jan 31, 2025

evan-masseau Jan 31, 2025

ajaysubra Jan 31, 2025

evan-masseau Jan 31, 2025

evan-masseau Jan 31, 2025 •

edited

Loading

ajaysubra Jan 31, 2025

kennyklaviyo Jan 31, 2025

evan-masseau Jan 31, 2025

evan-masseau Jan 31, 2025 •

edited

Loading

ajaysubra left a comment

ajaysubra Jan 31, 2025

ajaysubra Jan 31, 2025

ajaysubra Jan 31, 2025

ajaysubra Jan 31, 2025

ajaysubra Jan 31, 2025

ajaysubra Jan 31, 2025

jordan-griffin Jan 31, 2025

evan-masseau Jan 31, 2025

evan-masseau Jan 31, 2025

		@@ -1,5 +1,3 @@
		import { TurboModuleRegistry } from 'react-native';

		android.enableJetifier=true
		android.useAndroidX=true

Support for bridgeless NativeModule in New Architecture #194

Support for bridgeless NativeModule in New Architecture #194

Conversation

evan-masseau commented Jan 30, 2025 • edited Loading

Description

Check List

Changelog / Code Overview

Test Plan

Related Issues/Tickets

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

evan-masseau Jan 31, 2025 • 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

evan-masseau Jan 31, 2025 • edited Loading

Choose a reason for hiding this comment

ajaysubra left a comment

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

evan-masseau commented Jan 30, 2025 •

edited

Loading

evan-masseau Jan 31, 2025 •

edited

Loading

evan-masseau Jan 31, 2025 •

edited

Loading