Integrate FIS into FIAM Headless #1332
Conversation
Codecov Report
Continue to review full report at Codecov.
|
.../androidTest/java/com/google/firebase/inappmessaging/FirebaseInAppMessagingFlowableTest.java
Outdated
Show resolved
Hide resolved
...essaging/src/main/java/com/google/firebase/inappmessaging/internal/AbtIntegrationHelper.java
Outdated
Show resolved
Hide resolved
| }); | ||
| return firebaseInstallations | ||
| .getId() | ||
| .continueWithTask( |
There was a problem hiding this comment.
Why are you using continueWithTask? do you want to proceed even if this task is cancelled?
There was a problem hiding this comment.
so now I changed it to continueWith and I check both results against null. Am I not handling error cases correctly?
There was a problem hiding this comment.
I am actually not sure that null can be returned at all from those tasks, if an error happened getResult() will throw that error so I think it should be fine in terms of error handling.
...base-inappmessaging/src/main/java/com/google/firebase/inappmessaging/internal/ApiClient.java
Outdated
Show resolved
Hide resolved
| Subscriber firebaseEventsSubscriber) { | ||
| this.sharedPreferencesUtils = sharedPreferencesUtils; | ||
| isGlobalAutomaticDataCollectionEnabled = | ||
| new AtomicBoolean(firebaseApp.isDataCollectionDefaultEnabled()); | ||
| if (isAutomaticDataCollectionEnabled()) { | ||
| // Trigger this as early as possible, to minimize any latencies on returning the token |
There was a problem hiding this comment.
Is this no longer desirable? Or did FIS change in a way that makes it a non-issue?
There was a problem hiding this comment.
This just felt like a premature optimization and it seemed like this would be a good time to understand if it was actually necessary
...ppmessaging/src/test/java/com/google/firebase/inappmessaging/FirebaseInAppMessagingTest.java
Outdated
Show resolved
Hide resolved
...ppmessaging/src/test/java/com/google/firebase/inappmessaging/FirebaseInAppMessagingTest.java
Outdated
Show resolved
Hide resolved
...-inappmessaging/src/test/java/com/google/firebase/inappmessaging/internal/ApiClientTest.java
Outdated
Show resolved
Hide resolved
| }); | ||
| return firebaseInstallations | ||
| .getId() | ||
| .continueWithTask( |
There was a problem hiding this comment.
I am actually not sure that null can be returned at all from those tasks, if an error happened getResult() will throw that error so I think it should be fine in terms of error handling.
| (unused) -> { | ||
| String idResult = idTask.getResult(); | ||
| InstallationTokenResult tokenResult = tokenTask.getResult(); | ||
| if (tokenResult == null || idResult == null) { |
There was a problem hiding this comment.
None of the tasks will return null. Its on me for not adding the annotations on the APIs. I will update that on my end.
The Tasks would throw exceptions which you might want to handle based on completion state.
| } | ||
| } | ||
|
|
||
| private CampaignAnalytics createEventEntry(InAppMessage message, EventType eventType) { | ||
| return createCampaignAnalyticsBuilder(message).setEventType(eventType).build(); | ||
| private CampaignAnalytics createEventEntry(InAppMessage message, String id, EventType eventType) { |
There was a problem hiding this comment.
nit: call it fid or instanceId to differentiate from any other type of id used in FIAM. Idk if there is one, just think it would avoid any confusion.
| @@ -116,17 +116,17 @@ dependencies { | |||
| implementation project(':protolite-well-known-types') | |||
| implementation project(':transport:transport-api') | |||
| implementation project(':firebase-datatransport') | |||
| implementation project(':firebase-installations-interop') | |||
There was a problem hiding this comment.
Wondering if you should depend on the released version vs the HEAD? Idk whats the standard way in firebase.
This reverts commit 261ad66.
No description provided.