Skip to content

[draft]Optimize turbo module e2e#581

Open
namArora3112 wants to merge 46 commits into
mainfrom
optimize-turbo-module-e2e
Open

[draft]Optimize turbo module e2e#581
namArora3112 wants to merge 46 commits into
mainfrom
optimize-turbo-module-e2e

Conversation

@namArora3112

Copy link
Copy Markdown
Collaborator

Description

Related Issue

Motivation and Context

How Has This Been Tested?

Screenshots (if appropriate):

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • I have signed the Adobe Open Source CLA.
  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

Naman Arora and others added 30 commits March 23, 2026 14:14
Conflict resolutions:
- metro.config.js: kept our version (useWatchman:false for sandbox env)
- OptimizeView.tsx: took main's customScopeInput dynamic scope
- package.json: took main's yarn@4.1.1 format, re-added AwesomeProject+e2e workspaces
- App.tsx, yarn.lock: took main's versions

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@namArora3112 namArora3112 self-assigned this Jun 9, 2026
// decisionScopeText,
// decisionScopeImage,
// decisionScopeHtml,
// decisionScopeJson,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Are these changes intended? I see we have updated the default target store and commented out decision scopes.

// For class components, call initializeWithAppId inside componentDidMount.
MobileCore.setLogLevel(LogLevel.DEBUG);
MobileCore.initializeWithAppId("YOUR-APP-ID")
MobileCore.initializeWithAppId("3149c49c3910/0f12baf27522/launch-0d096c129660-development")

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Let's not commit App ID

// decisionScopeText,
// decisionScopeImage,
// decisionScopeHtml,
// decisionScopeJson,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Same as above, please validate

call(propositions: Map<String, Proposition>) {
console.log('onPropositionUpdate called');
if (propositions) {
console.log('propositions bnana', JSON.stringify(Object.fromEntries(propositions), null, 2));

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Please sanitize logs

@namArora3112 namArora3112 requested a review from ishwetansh June 22, 2026 05:34

@ishwetansh ishwetansh left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Review — TurboModule / Architecture

Skipping test-session cleanup items (App ID, debug logs, commented scopes) — focusing only on architecture.


1. USE_INTEROP_ROOT in the iOS podspec contradicts the wiki

packages/optimize/RCTAEPOptimize.podspecs.pod_target_xcconfig:

"GCC_PREPROCESSOR_DEFINITIONS" => "$(inherited) USE_INTEROP_ROOT=\#{use_interop_root}"

The wiki you wrote (TurboModule vs Interop Layer) explicitly says:

Remove USE_INTEROP_ROOT from the iOS podspec — it misleads reviewers into thinking iOS has Android parity.

RCTAEPOptimize.mm has zero #if USE_INTEROP_ROOT guards, so this preprocessor definition is a compile-time no-op. The withInteropRoot.js Expo plugin also injects ENV['USE_INTEROP_ROOT'] into the iOS Podfile, but since the native code never reads it, it has no effect on iOS. Recommend removing use_interop_root from the podspec entirely and limiting withInteropRoot.js to Android, or at minimum adding a clear comment that it is a no-op on iOS.


2. Two near-identical spec files — maintenance burden

  • packages/optimize/specs/NativeAEPOptimize.ts — uses CodegenTypes.EventEmitter<PropositionsPayload> (codegen input, consumed by the RN build system)
  • packages/optimize/src/NativeAEPOptimize.ts — uses an explicit function type (handler: ...) => EventSubscription (runtime import used by Optimize.ts, Offer.ts, Proposition.ts)

The comment in src/NativeAEPOptimize.ts explains the @types/react-native era incompatibility with CodegenTypes — that rationale is valid. But both files define the full Spec interface independently, so a method added to one must be manually mirrored in the other. Please add a note at the top of each file pointing to its counterpart and calling out the intentional divergence, so the next contributor knows to keep them in sync.


3. RCTAEPOptimizeUtil.emitOnPropositionsUpdate — appears to be dead code

packages/optimize/android/src/main/java/.../RCTAEPOptimizeUtil.java:

static void emitOnPropositionsUpdate(
        final ReactApplicationContext context,
        final Map<DecisionScope, OptimizeProposition> decisionScopePropositionMap,
        final boolean checkActiveInstance) {
    ...
    context.getJSModule(DeviceEventManagerModule.RCTDeviceEventEmitter.class)
            .emit("onPropositionsUpdate", writableMap);
}

This uses the old bridge RCTDeviceEventEmitter. NativeAEPOptimizeModule (turbo path) calls emitOnPropositionsUpdated(payload) directly via JSI and doesn't use this method. If it's meant for RCTAEPOptimizeModule (interop path), that call site isn't visible in this diff. Please confirm it's called somewhere, or remove it to avoid confusion about which event channel is active.


4. Proposition updates silently dropped on iOS before JS subscribes

packages/optimize/ios/src/RCTAEPOptimize.mm — inside onPropositionsUpdate:

if (_eventEmitterCallback) {
    [self emitOnPropositionsUpdated:@{@"propositions": propositionDictionary}];
}

The guard is correct — JSI EventEmitterCallback is nil until JS subscribes and crashing here would be worse. But any native AEP SDK callback that fires between native registration and the JS onPropositionUpdate() call is silently dropped with no log, no queue. This is the same behavior as the old hasListeners guard, so not a regression. However, a warning log in the else branch would make this diagnosable in the field:

} else {
    [AEPLog warningWithLabel:TAG message:@"onPropositionsUpdate: eventEmitterCallback nil, update dropped"];
}

5. Module rename is a silent breaking change for Old-Arch consumers

packages/optimize/android/src/main/java/.../RCTAEPOptimizeModule.java:

// Before
public String getName() { return "AEPOptimize"; }
// After
public String getName() { return "NativeAEPOptimize"; }

Correct for the TurboModule path (matches the codegen spec name). But any host app still on Old Architecture that used NativeModules.AEPOptimize will silently get undefined — no exception, just a broken SDK. Should be called out explicitly in the changelog / migration guide before release.


6. Confirm DataReader is on the classpath

packages/optimize/android/src/main/java/.../RCTAEPOptimizeUtil.javacachePropositionOffers:

activityId = DataReader.getString(activity, "id");

com.adobe.marketing.mobile.util.DataReader isn't declared explicitly in build.gradle — it's presumably pulled in transitively via the AEP SDK BOM. Worth a quick verify this doesn't become a NoClassDefFoundError in isolated build environments.


Minor

  • Optimize.ts, Offer.ts, Proposition.ts — multiple as any casts introduced (new Proposition(value as any), .then((propositions: any)). Worth typing properly against the Spec interface.
  • No Android unit tests for NativeAEPOptimizeModule — the turbo path's core flows (onPropositionsUpdate, getPropositions, updatePropositions) have no JUnit coverage.

Overall the architecture is solid and consistent with the wiki. iOS unified path (SpecBase + getTurboModule: + emitOnPropositionsUpdated:) and Android dual-class USE_INTEROP_ROOT toggle are correctly implemented. The items above are what to address before graduating from draft.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants