Repository navigation
MOEN-47655: Added support for firebase installation id - #140
Conversation
|
🚪 Hodor is reviewing this PR... 👀 |
moe-hodor
Bot
added
cfr:neutral
![moe-hodor[bot]](https://avatars.githubusercontent.com/in/3251438?s=60&v=4){kind=small}
d2db67d to
776bf08
Compare
467e1c0 to
6d6ac69
Compare
There was a problem hiding this comment.
Reviewed the FID support end-to-end (TS public API → spec → Android bridge → expo plugin). The shape of the feature is right and the expo config-plugin tests are a good addition. Main blockers are build config that can't ship and an iOS gap.
Blockers: the mavenLocal() + -fid-local force block exists in both sdk/core/android/moengage-dependency-helper.gradle (already flagged by moe-hodor) and sdk/expo/android/build.gradle; the two new Spec methods have no iOS RCT_EXPORT_METHOD stub, breaking the repo's convention that every Android-only spec method gets a no-op on iOS; and MoEExpoFireBaseMessagingService.onNewToken now discards the token entirely, which is a behaviour removal the changelog records as [minor].
Two notes on the existing review:
- moe-hodor's "silent null return" comment on
getFirebaseInstallationId()doesn't hold — the catch block re-throws (index.ts:873). - Correction to my earlier note on the event routing: I've since reviewed the plugin-base PR (moengage/android-plugin-base#114).
installationIdEventToJsonemits{accountMeta:{appId}, data:{platform, pushService, installationId}}— it does not follow the flat shape oftokenEventToJson. Routing the event through theaccountMetabranch ofexecuteHandleris therefore correct, andgetFirebaseInstallationIdResult(payload, accountMeta)reads the right keys. moe-hodor's concern and my corroboration of it can both be disregarded.
Separately, the plugin-base PR has its own blockers that gate this one: the same -fid-local dependency pinning, and the deletion of PluginFcmTokenListener, which stops EventType.PUSH_TOKEN_GENERATED ever firing — so the pushTokenGenerated event that EventEmitterImpl and index.ts still expose goes dead for FCM apps.
001da0c to
50faacb
Compare
There was a problem hiding this comment.
why is this change part of this PR?
There was a problem hiding this comment.
rebased
There was a problem hiding this comment.
shouldn't be a part of this PR
There was a problem hiding this comment.
rebased
There was a problem hiding this comment.
shouldn't be a part of this PR
There was a problem hiding this comment.
rebased
6d6ac69 to
cfdd440
Compare
52e2e62 to
ed75ae9
Compare
50faacb to
76466a2
Compare
ed75ae9 to
49b4746
Compare
e0342ce to
46c341e
Compare
49b4746 to
04d621c
Compare
04d621c to
6e70672
Compare
Jira Ticket
https://moengagetrial.atlassian.net/browse/MOEN-47655
Description
Support for firebase installation id