Skip to content

feat(rokt)!: accept only placeholder names in selectPlacements - #428

Open
thomson-t wants to merge 1 commit into
thomson-t/spm-06-package-swiftfrom
thomson-t/spm-07-remove-placeholder-map
Open

thomson-t wants to merge 1 commit into
thomson-t/spm-06-package-swiftfrom
thomson-t/spm-07-remove-placeholder-map

Conversation

@thomson-t

Copy link
Copy Markdown
Contributor

Why

Embedded placements can still be requested with a map of placeholder name to findNodeHandle view tag. That form keeps the SDK on view-tag lookups React Native is removing (viewRegistry_DEPRECATED on iOS, NativeViewHierarchyManager and UIManager.resolveView on Android), and integrations that use it can still read a tag before the view exists and fail to embed without an error. Name-based placeholders already cover every case the map did, including views that mount after the call. Once this lands, selectPlacements has one way to embed a placement: an array of RoktLayoutView placeholder names. This is a breaking change for apps that still pass the map; the release that ships the Swift Package Manager work is a major version for this reason.

Programme

Part of the plan to support Swift Package Manager in this package before the CocoaPods central repository becomes read-only on 2026-12-02. This is the seventh pull request in the series merging into the workstation/spm-migration branch, which merges into main once the series is complete and ships in one release; the plan record is internal and cannot be linked here.

What changes

Before: placeholders is a name array or a map of name to view tag. The wrapper turns both into a map, using 0 to mean "look up by name", and each native module resolves positive values as view tags.

After: placeholders is an array of placeholder names, from the TypeScript type through the native module interface to both native modules.

  • JavaScript: RoktPlaceholders is string[]. A map passed from plain JavaScript logs an error, and the placement is requested without embedded views. Without this check the platforms would disagree: Android would throw, and iOS would drop or misread the argument.
  • iOS: resolvePlaceholders: and the wait for unmounted views read names only and skip non-string entries. The view-tag lookup and @synthesize viewRegistry_DEPRECATED are removed.
  • Android: both architectures call one name resolver in MPRoktModuleImpl, which skips non-string entries. The view-tag lookup (UIManager.resolveView, NativeViewHierarchyManager) is removed. The legacy architecture still uses addUIBlock to run after pending UI work.
  • Tests: the jest, Android unit and XCTest cases for view tags are replaced by array cases, including non-string entries.
  • Docs: README and MIGRATING say the map form is removed, and that earlier 3.x releases accept both forms, so apps can switch before they upgrade.

Start reading at js/rokt/rokt.ts, then MPRoktModuleImpl.kt and RNMPRokt.mm. Unchanged on purpose: the name registry, the 2-second wait for unmounted views, and PlacementFailure for dropped waits.

Linked work

Depends on: the experimental Package.swift (branch thomson-t/spm-06-package-swift) and the pull requests below it in this series; merge those first.
Related: the pull request that added name-based placeholders, #410.

Rollout

Path: this merges into workstation/spm-migration, not main, so nothing reaches main or a release until the whole series has merged there and that branch is merged into main. It then ships in the next release, which must be a major version.
Feature flags: none.
Turning it off: revert this pull request; the map form comes back in the next release.
What we watch: this repository's issues, for embedded placements that stop rendering after an upgrade, and the error selectPlacements: placeholders must be an array in partner reports.

Risks

  • Apps that still pass the map lose their embedded placements after upgrading. Not prevented, because this is the intended break. It is mitigated by the TypeScript error, the logged error, MIGRATING, and 3.x releases that accept both forms. We would see reports of embedded placements not rendering, with the error above in the logs.
  • A native caller that bypasses the wrapper passes a map or a non-string entry. Prevented, because both native modules read only string entries and skip anything else. We would see Cannot resolve placeholder logs.
  • The Old Architecture iOS and Android modules change signature. Covered by the Android unit tests and lint, which compile the legacy module. An Expo 54 / React Native 0.81.5 app with the New Architecture off builds on iOS. We would see build errors on React Native 0.81 with the New Architecture off.

Risk class: medium, because this is a breaking API change.

Who

Written by: an automated coding agent (Claude Code), at an engineer's request.
Code reviewed before opening: an independent review agent reviewed the change before it was committed.
Design reviewed before opening: the requesting engineer approved removing the map form in the release that ships Swift Package Manager support, and chose the log-and-drop behaviour for a leftover map.
Decision this implements: the engineering decision on 2026-09-29 to ship the breaking placeholder change together with Swift Package Manager support; the record is internal and cannot be linked.
Checked: on 2026-09-30, with Xcode 27.0, an iOS 26.5 simulator and an Android emulator (API 36):

  • yarn test (jest and eslint), yarn build and yarn build:plugin; ./gradlew test ktlintCheck lint in android/.
  • Sample app (React Native 0.84, New Architecture, default CocoaPods mode):
    • The full Debug XCTest suite, with Metro running: 54 passed and 1 skipped, the legacy-architecture-only test.
    • The Podfile.lock is unchanged, and the Release build has one copy of each SDK class.
    • Embedded placement by name, called from the same effect that renders the view: renders on iOS and Android.
    • The old map form, passed from plain JavaScript: the error is logged, the app keeps running, and the placement request goes out without embedded views, ending in PlacementFailure on iOS and Android.
  • Swift Package Manager mode (MP_USE_SPM=1): the sample builds in Release with one copy of each SDK class, all in the app binary.
  • Old Architecture: an Expo 54 / React Native 0.81.5 app with the New Architecture off builds in Release on iOS.
    Not checked: overlay placements (they pass no placeholders, so that path is unchanged); React Native's Swift Package Manager mode; physical devices.

Size

Hand-written: 149 lines added and 263 removed in 14 files (55 added and 70 removed of them in tests).
Generated: none.

🤖 Generated with Claude Code

@thomson-t
thomson-t marked this pull request as ready for review September 30, 2026 15:28
@thomson-t
thomson-t requested a review from a team as a code owner September 30, 2026 15:28
@cursor

cursor Bot commented Sep 30, 2026 •

Copy link
Copy Markdown

PR Summary

Medium Risk
Breaking API change that removes legacy view-tag lookup support for Rokt embedded placements. Applications passing a map of placeholder names to React tags will need to migrate to placeholder name arrays.

Overview
Removes support for the legacy placeholder-to-React-tag map in MParticle.Rokt.selectPlacements, requiring embedded placeholders to be specified solely as an array of RoktLayoutView placeholder names.

Passing an unsupported map from JavaScript now logs an error and issues the placement request without embedded views instead of failing native conversion. Native modules across iOS and Android (both new and legacy architectures) and TurboModule specs have been updated to accept array types directly, removing native view-tag resolution logic (viewRegistry_DEPRECATED, UIManager.resolveView, NativeViewHierarchyManager). Tests and migration documentation have been updated accordingly.

Reviewed by Cursor Bugbot for commit 77c0bf7. Bugbot is set up for automated code reviews on this repo. Configure here.

@thomson-t
thomson-t added this pull request to stack #427 September 30, 2026 15:29
Copilot AI balanced review requested due to automatic review settings October 1, 2026 18:29
@thomson-t
thomson-t force-pushed the thomson-t/spm-07-remove-placeholder-map branch from 5ba7575 to 046c618 Compare October 1, 2026 18:29

Copilot AI left a comment

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.

Copilot review overview

🔵 Needs a closer look

The coordinated breaking API change spans JavaScript and both native platforms across two React Native architectures.

Review effort: Balanced
Findings: None

What changed in this PR

Removes legacy React-tag placeholder maps, standardizing selectPlacements on placeholder-name arrays across JavaScript, iOS, and Android.

Changes:

  • Updates the public API and native interfaces to accept string[].
  • Removes native React-tag resolution while retaining delayed name resolution.
  • Updates tests and migration documentation for the breaking change.
File Description
README.md Documents removal of map placeholders.
MIGRATING.md Adds migration and behavior guidance.
js/​rokt/​rokt.ts Enforces arrays and rejects legacy maps.
js/​codegenSpecs/​rokt/​NativeMPRokt.ts Changes the native specification to arrays.
js/​__tests__/​rokt-placeholders.test.ts Tests array forwarding and map rejection.
ios/​RNMParticle/​RoktPlaceholderRegistry.h Updates registry documentation.
ios/​RNMParticle/​RNMPRokt.mm Removes tag lookup and resolves names only.
sample/​ios/​MParticleSampleTests/​RNMPRoktPlaceholderTests.m Updates iOS placeholder tests.
android/​src/​main/​java/​com/​mparticle/​react/​rokt/​MPRoktModuleImpl.kt Centralizes Android name resolution.
android/​src/​main/​java/​com/​mparticle/​react/​rokt/​RoktPlaceholderRegistry.kt Updates registry documentation.
android/​src/​oldarch/​java/​com/​mparticle/​react/​rokt/​MPRoktModule.kt Migrates legacy architecture to arrays.
android/​src/​oldarch/​java/​com/​mparticle/​react/​NativeMPRoktSpec.kt Updates the legacy native interface.
android/​src/​newarch/​java/​com/​mparticle/​react/​rokt/​MPRoktModule.kt Migrates new architecture to shared resolution.
android/​src/​test/​java/​com/​mparticle/​react/​rokt/​MPRoktModuleImplTest.kt Updates Android array filtering coverage.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

selectPlacements now takes an array of RoktLayoutView placeholderNames only.
The map of placeholder name to findNodeHandle react tag is removed, together
with the native view-tag lookups behind it (viewRegistry_DEPRECATED on iOS,
UIManager.resolveView and NativeViewHierarchyManager on Android).

The native module interface now declares placeholders as Array<string>. A
map passed from plain JavaScript logs an error, and the placement is
requested without embedded views, so every platform behaves the same instead
of Android throwing and iOS dropping or misreading the argument. Both native
modules skip non-string entries. The Android name lookup is shared by both
architectures in MPRoktModuleImpl.

BREAKING CHANGE: selectPlacements no longer accepts
{ [placeholderName]: findNodeHandle(ref) }. Pass ['placeholderName'] instead;
earlier 3.x releases accept both forms, so apps can switch before upgrading.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
@thomson-t
thomson-t force-pushed the thomson-t/spm-07-remove-placeholder-map branch from 046c618 to 77c0bf7 Compare October 2, 2026 14:59
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.

2 participants