fix(ios): restore legacy-architecture build and collapse duplicate RCTConvert categories - #396
Merged
Merged
Conversation
…TConvert categories
Building with RCT_NEW_ARCH_ENABLED=0 (React Native <= 0.81) failed to compile:
RNMParticle.mm:769:33 No visible @interface for 'MParticle'
declares the selector 'logCommerceEvent:'
-[MParticle logCommerceEvent:] was deprecated in mParticle-Apple-SDK 7.12.6
with an explicit "call logEvent: instead" message, and its declaration was
removed from the public headers in 9.0.0 while the implementation stayed behind
as an internal entry point. When the podspec moved to
mParticle-Apple-SDK-ObjC ~> 9.0, the New Architecture leg was migrated to
logEvent: but the legacy leg was not. Nothing caught it: the only iOS build in
CI targets sample/, which is on React Native 0.84, and React Native removed the
legacy architecture in 0.82 -- so every #else leg under ios/ has zero compiler
coverage. This aligns iOS with Android, where logEvent(BaseEvent) is the sole
public entry point and logCommerceEvent is private.
Investigating that blind spot surfaced a second defect on the same path. Two
RCTConvert categories implemented five identical selectors with different
bodies, and which one the Objective-C runtime picked was undefined behaviour.
They are now collapsed into a single RCTConvert (RNMParticle) category, keeping
per selector the implementation that is correct rather than the one that
happened to win:
- +MPEvent: kept the copy that converts startTime/endTime from JS milliseconds
into NSDate. The copy that was running assigned the raw NSNumber straight
into the NSDate * properties.
- +MPAliasRequest: kept the copy that treats startTime/endTime as
milliseconds. The copy that was running read them as seconds, producing
dates roughly 1000x in the future. Milliseconds matches the Android SDK
("the time, in milliseconds"), the Android bridge, the codegen spec, and the
New Architecture path, which already divided by 1000.
- +MPCommerceEvent: kept the copy that maps promotions, impressions, currency,
checkout fields, productList* and screenName. The other mapped none of them.
- +MPGDPRConsent:/+MPCCPAConsent: kept the millisecond timestamp handling added
deliberately for device consent, and added the other copy's null handling for
document/location/hardwareId.
Explicit JS null is now treated as absent rather than stored as NSNull for
those consent fields and for the commerce string fields, matching the Android
bridge. Also removes two converters dead in both architectures, the duplicate
category interface, the declared-vs-implemented parameter type mismatches, and
a podspec variable with no reader since the SDK 9 bump.
Tests cover the timestamp conversions and null handling for MPEvent,
MPAliasRequest and both consent types -- none of which had any coverage before,
which is why the conflicts went unnoticed -- plus a guard that commerce events
remain loggable through logEvent:.
Verified by reproducing the reported failure in a React Native 0.81.6 app with
RCT_NEW_ARCH_ENABLED=0 and confirming it builds cleanly afterwards, the first
time the whole iOS legacy leg has been compiled. The New Architecture suite
passes 18/18 on React Native 0.84.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
PR SummaryMedium Risk Overview Consolidates duplicate Adds unit tests in Reviewed by Cursor Bugbot for commit 3a6b757. Bugbot is set up for automated code reviews on this repo. Configure here. |
Contributor
There was a problem hiding this comment.
🟢 Approval recommended
The focused native changes are covered by regression tests, with no unresolved correctness issues found.
Pull request overview
Fixes legacy iOS builds against mParticle SDK 9.x and consolidates duplicate RCTConvert implementations.
Changes:
- Routes legacy commerce logging through
logEvent:. - Merges converters and corrects timestamp/null handling.
- Adds regression tests and removes an unused podspec variable.
File summaries
| File | Description |
|---|---|
ios/RNMParticle/RNMParticle.mm |
Consolidates converters and fixes legacy logging. |
sample/ios/MParticleSampleTests/RCTConvertCommerceMappingTests.m |
Adds converter and API regression coverage. |
react-native-mparticle.podspec |
Removes stale architecture configuration. |
Review details
- Files reviewed: 3/3 changed files
- Comments generated: 0
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
BrandonStalnaker
previously approved these changes
Sep 18, 2026
BrandonStalnaker
left a comment
BrandonStalnaker
left a comment
Contributor
There was a problem hiding this comment.
All the functionality is there. I just noticed one test wasn't really testing what you wanted it to
…assertion The previous guard asserted respondsToSelector:@selector(logEvent:), which cannot detect the failure mode it was written for. That failure mode is declaration removed, implementation retained: point the same assertion at logCommerceEvent: on main today and it passes while the build is broken, because the implementation is still there (mParticle.m:1169). @selector() is no help either -- logCommerceEvent: is still declared in MPKitProtocol.h, so the expression compiles without even a -Wundeclared-selector warning. Only compiling a real message send requires a visible declaration, so the test now builds and executes the call. shouldUploadEvent = NO keeps it from uploading. Verified by negative control: swapping the call to logCommerceEvent: fails the test target build with "no visible @interface for 'MParticle' declares the selector 'logCommerceEvent:'" -- the same error class as the original break. Restored, suite is 18/18. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
nickolas-dimitrakas
approved these changes
Sep 18, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes the iOS build for consumers on the legacy React Native architecture (
RCT_NEW_ARCH_ENABLED=0, React Native ≤ 0.81), and resolves a second defect found on the same code path: twoRCTConvertcategories implementing the same five selectors, where which one ran was undefined behaviour.Reported failure:
Root cause
RNMParticle.mm:769sits in the#else(legacy architecture) leg and calls-[MParticle logCommerceEvent:]. That selector is no longer declared in any public header ofmParticle-Apple-SDK-ObjC9.x. The implementation is still present (mParticle.m:1213) — it was demoted to an internal entry point that-logEvent:dispatches commerce events through. Objective-C permits an implementation with no declaration, so external callers get a hard compile error whilerespondsToSelector:still returnsYES.The demotion mirrors Android, where
public void logEvent(BaseEvent)(MParticle.java:517) is the sole public entry point andprivate void logCommerceEvent(CommerceEvent)(MParticle.java:632) is internal. iOS 9.0 converged on that shape, andmParticle.h:844-851documents passing anMPCommerceEventtologEvent:.Why it worked before and breaks now
logCommerceEvent:deprecated withDEPRECATED_MSG_ATTRIBUTE("Replace calls to \logCommerceEvent:` with `logEvent:`")`logCommerceEvent:Include/mParticle.h, implementation retained. Undocumented — absent from the 9.0.0 changelog and fromMIGRATING.md, which lists ~10 other 9.0 removalsmParticle-Apple-SDK-ObjC ~> 9.0. New Arch leg migrated tologEvent:(:671); legacy leg was notVerified across SDK tags: declared through
v8.31.0, gone fromv9.0.0onward.Why CI didn't catch it
The only iOS compile in the repo is
ios-sample-app, which buildssample/on React Native 0.84. React Native removed the legacy architecture in 0.82, soRCT_NEW_ARCH_ENABLED=0is not merely untested there — it is unrepresentable. Every#elseleg underios/has zero compiler coverage, and no static analysis touches Objective-C.Android is the inverse and is fine:
android/build.gradle:75-77selectssrc/oldarch/javaby default, soandroid-unit-tests/android-lint/android-kotlin-lintcompile the legacy Android leg on every PR. Only iOS has the blind spot, and this PR does not close it — see Follow-ups.Second defect: duplicate
RCTConvertcategoriesRNMParticle.mmhad two categories —(MParticle)and(MPCommerceEvent)— implementing+MPEvent:,+MPCommerceEvent:,+MPGDPRConsent:,+MPCCPAConsent:and+MPAliasRequest:with materially different bodies. Categories produce no duplicate-symbol error and no warning, so this was silent.Which one ran, proven:
testMPCommerceEventFromJSON_mapsCurrencyCheckoutStepAndCheckoutOptionsassertscurrency/checkoutStep/productListName/screenName, which only the(MPCommerceEvent)copy maps, and it is green onmain. So the later-defined category won and the(MParticle)copies were dead, shadowed code.Origin:
RNMParticle.mhad exactly oneRCTConvertcategory from 2017 to 2025. The New Architecture rewrite to.mm(landed squashed via #230) copied it over verbatim and added a second parallel category. Nothing in the 13 months since acknowledges the collision — #336's body says "same bug class if that code path is used", and #378's calls+[RCTConvert MPCommerceEvent:]"the legacy bridge" converter, singular.Collapsed into one
RCTConvert (RNMParticle), keeping per selector the implementation that is correct, not the one that happened to win. Android is the reference for the JS contract since both platforms consume the identical payload, and Android passes every timestamp through as epoch milliseconds, scaling nothing; the iOS properties areNSDate *, so iOS must divide by 1000.+MPEvent:(MParticle)customAttributesrename (#311) and null handling (#362), and is the only copy convertingstartTime/endTimefrom milliseconds intoNSDate. The copy that was running assigned the rawNSNumberinto theNSDate *properties+MPAliasRequest:(MParticle)AliasRequest.java:129), Android bridge pass-through, Android's own test,js/codegenSpecs/NativeMParticle.ts:108-113,sample/index.js:76-77, and the New Arch path at:471-473, which already divided by 1000+MPCommerceEvent:(MPCommerceEvent)productList*,screenName,nonInteractive. The other maps none. Already live and pinned by required CI+MPGDPRConsent:/+MPCCPAConsent:timestamphandling verbatim — the only intentional converter change in the whole history, made against a High-Severity review finding — and added the other copy's null handling fordocument/location/hardwareIdNothing in the history deliberately created a divergence this merge undoes. #351's fix moved the legacy copy toward the
(MParticle)copy; every other asymmetry was an author editing whichever copy they knew about.Also removed: two converters dead in both architectures (
+MPIdentityApiResult:,+MParticleUser:), the duplicate category interface, the declared-vs-implemented parameter type mismatches (+MPCommerceEventAction:(id)declared vs(NSNumber *)implemented), and a podspec variable with no reader since #311.This is not only a build fix. On the legacy bridge, except where noted:
logCommerceEventroutes through-[MParticle logEvent:]. Equivalent, except the Rokt API diagnostic string isLOG_EVENTrather thanLOG_COMMERCE_EVENT— which the New Arch leg has already been emitting since feat: Support Shoppable ads and v9 of mParticle iOS SDK #311, so no new divergence between architectures.startTime/endTimemove from seconds to milliseconds. Requests passing explicit times were previously producing dates ~1000× in the future.MPEventstartTime/endTimestart working at all.nullis treated as absent rather than stored asNSNull— consentdocument/location/hardwareId(both architectures, via the shared+MPConsentState:) and commercecurrency/checkoutOptions/productActionListName/productActionListSource/screenName.Blast radius on the new architecture is nil: the two consent bodies were already behaviourally identical on
timestamp, and only guards were added.Testing
RCTConvertCommerceMappingTestscovered+MPCommerceEvent:only — there was no coverage of+MPEvent:times,+MPAliasRequest:,+MPGDPRConsent:or+MPCCPAConsent:, which is precisely why the conflicts survived. Added tests for the millisecond→NSDateconversions and null handling on all four, plus a guard that compiles and executes a real[[MParticle sharedInstance] logEvent:event]call. That has to be a compiled message send, not an assertion: declaration-removed / implementation-retained is invisible at runtime, sorespondsToSelector:and@selector()both still succeed against the removed selector. Negative control: pointing that call atlogCommerceEvent:fails the test target build with the same error as the original break.RCT_NEW_ARCH_ENABLED=0, published 3.3.3RNMParticle.mm:769:33— and it was the only error, confirming the symbol audit was completeyarn dev:packsample/New Architecture build +RCTConvertCommerceMappingTests(RN 0.84)yarn test(jest + eslint)