feat(crashlytics)!: upgrade to new Firebase Crashlytics SDK #3580

Aure77 · 2020-04-29T22:51:29Z

Description

Fabric is deprecated and will be available until May 4, 2020.
Fabric is now part of Firebase services.
Firebase has introduced a new SDK for handling Crashlytics reports. So this PR is an upgrade from upcoming deprecated Fabric SDK to new Firebase Crashlytics SDK.

Release Summary

Upgrade to new Firebase Crashlytics SDK

Checklist

I read the Contributor Guide and followed the process outlined there for submitting PRs.
- Yes
My change supports the following platforms;
- Android
- iOS
My change includes tests;
- e2e tests added or updated in packages/\*\*/e2e
- jest tests added or updated in packages/\*\*/__tests__
I have updated TypeScript types that are affected by my change.
This is a breaking change;
- Yes
- No

Test Plan

CLAassistant · 2020-04-29T22:51:34Z

All committers have signed the CLA.

mikehardy · 2020-04-30T03:19:03Z

First I just want to say this is fantastic. Thank you for taking this on

But second I do want to say that using the SDK, but connected to Firebase vs legacy Fabric is (I believe) not what's going away on May 4, 2020. I refer to these as Fabric/Fabric Crashlytics, Fabric/Firebase Crashlytics and Firebase/Firebase Crashlytics. I could be wrong but the Firebase/Firebase Crashlytics beta literally just came out of beta and I can't imagine them then having a whole vital service going away about 6 days later. I think Fabric/Fabric Crashlytics is going away, but Fabric/Firebase Crashlytics is fine if I understand it correctly.

All of that said, forward-porting to the future - which is Firebase/Firebase Crashlytics of course is important so this effort is huge. I just don't think it's on a such a high pressure deadline

.../main/java/io/invertase/firebase/crashlytics/ReactNativeFirebaseCrashlyticsInitProvider.java

mikehardy · 2020-05-04T15:18:20Z

...id/src/main/java/io/invertase/firebase/crashlytics/ReactNativeFirebaseCrashlyticsModule.java

-  public void setUserEmail(String userEmail, Promise promise) {
-    if (ReactNativeFirebaseCrashlyticsInitProvider.isCrashlyticsCollectionEnabled()) {
-      Crashlytics.getInstance().core.setUserEmail(userEmail);
+      FirebaseCrashlytics.getInstance().setUserId(userId);


Similar to the init changes, are setUserName and setUserEmail going away?
If functionality is being reduced or changed there should be some migration documentation

https://firebase.google.com/docs/crashlytics/upgrade-sdk?platform=ios#setuseridentifier_is_now_setuserid_setusername_and_setuseremail_are_removed

Reason for change
We adopted the method name setUserID to be consistent with other Firebase APIs and removed setUserName and setUserEmail to discourage logging PII through Crashlytics.

packages/crashlytics/ios/RNFBCrashlytics/RNFBCrashlyticsModule.m

mikehardy

I think there should be some migration information as commented in my specific review notes, to help developers understand how to enable this in debug builds now, and the migration to setUserId vs setUserName/setUserEmail, but I want to be clear that I consider those important but small things vs most of this looking right on to me?

@Salakar / @Ehesp - do either of you two have time for a review of this? It's an important forward-port step for react-native-firebsea

mikehardy · 2020-05-04T15:26:41Z

packages/crashlytics/ios/RNFBCrashlytics/RNFBCrashlyticsInitProvider.m

-    // TODO(salakar): 6.x/7.x Option to disable auto init
-    // TODO(salakar): If disabled; when the default app is initialized from JS land then init crashlytics (register a block handler somehow in RNFBApp?)
-    if ([[RNFBJSON shared] contains:KEY_CRASHLYTICS_DEBUG_ENABLED]) {
-      [Crashlytics sharedInstance].debugMode = [[RNFBJSON shared] getBooleanValue:KEY_CRASHLYTICS_DEBUG_ENABLED defaultValue:NO];


It looks like the way to turn it on in debug now is -FIRDebugEnabled in iOS, in Android unsure? https://firebase.google.com/docs/crashlytics/test-implementation-new-sdk?platform=ios#enable_debug_logging

Aure77 · 2020-05-04T16:20:15Z

I having issue with e2e testing. More specifically with FirebaseInstanceId delete functions. Could you give me some support ? I saw that there is SDK change on instanceId in 6.15.0.
And there is maybe API-restrictions:
https://firebase.google.com/support/release-notes/ios#instanceid_5

Added a new dependency on the Firebase Installations SDK. The Firebase Installations SDK introduces the Firebase Installations API. Developers that use API-restrictions for their API-Keys may experience blocked requests (https://stackoverflow.com/questions/58495985/). A solution is available here. (#4533)

docs/crashlytics/ios-setup.md

Salakar · 2020-05-12T17:51:27Z

Quick glance this provisionally looks ok to me, though I need to do a proper full review still which I'll do on Friday.

I think there's a couple external blockers here still that I need time to sort;

InstanceID / Installations change
- Currently blocking upgrading Android & iOS Firebase SDK versions. I need to look into what this change actually means and setting up a new installations package to support it. This is whats causing the E2E failure on this PR.
Firebase SDK version change support on master needs a release still
- It's ready to release automatically on master, but there's a bit of meta work around it that needs doing first - e.g. a changelog/post explaining the major version change across the board and new independent versioning setup, which I've not had time to do yet

Aure77 · 2020-05-13T10:16:45Z

I have try InstanceID on e2e. I don't know what the problem. But my first observation is that some methods (delete) never ends, so promise never get resolved (and test fail).
I think this maybe a quota limit issue (chain get/delete id) ?

Salakar · 2020-05-23T10:59:27Z

Master now has the updated Firebase SDK, so this PR is now unblocked, would you be able to sync with master and resolve any conflicts? Thanks

Remove setUserName/setUserEmail methods (removed from SDK)

Co-authored-by: David Buchan-Swanson <[email protected]>

Aure77 · 2020-06-30T08:52:16Z

@mikehardy PR is up to date & fix FirebaseCrashlyticsCollectionEnabled is done.
There is Spelling & Grammar that fail on a file that is not part of this PR. Is it normal ?

@TPXP

Thx to @TPXP

TPXP · 2020-06-30T13:45:00Z

Thanks for taking my feedback into account, for what it's worth we have a production build with those changes and it works perfectly, so I believe this is good to go :-)

mikehardy · 2020-06-30T19:47:35Z

@Aure77 champion! 🏆 thank you! Good question about the spell check. @Salakar / @andersonaddo the spelling failure is from faqs-and-tips so should have been flagged there #3524 - this PR should be green or at least not blocked by that CI failure.

@TPXP fantastic that you took the initiative to integrate the PR locally and report success, that helps a lot

I don't think there should be anything in the way of this? Hopefully it can go in quickly before some new conflict creeps into it 🙏

mikehardy

Thank you very much!

andersonaddo · 2020-06-30T19:53:48Z

IIRC, it wasn't because of a spelling mistake; I think it was because I used an acronym ("RNFB"). I figured it'd be fine.

Should I push a fix?

Salakar · 2020-06-30T19:54:30Z

Should I push a fix?

Nah, fix it in post. About to merge and release, can send a grammar PR after if you like :)

mikehardy · 2020-06-30T19:54:50Z

@andersonaddo it is actually the string 'qa' 🤷 - seems valid in a software context

Salakar · 2020-06-30T19:58:03Z

LGTM! Shipped! A new major release will be up shortly after CI finishes. Thanks everyone that contributed.

mikehardy · 2020-07-01T01:11:37Z

I just integrated this in my work project with no issue, and my test crash button worked as expected in android and ios, perfect

…e#3580) Goodbye Fabric, hello Firebase Crashlytics. BREAKING CHANGE: This is a breaking change to remove the use of the Fabric SDKs. Co-authored-by: David Buchan-Swanson <[email protected]> Co-authored-by: Mike Diarmid <[email protected]> [publish]

Aure77 force-pushed the feature/new-firebase-crashlytics-sdk branch 2 times, most recently from bda9a8c to e96daba Compare April 29, 2020 23:21

Aure77 force-pushed the feature/new-firebase-crashlytics-sdk branch 3 times, most recently from 4821504 to 78f42bf Compare May 4, 2020 15:10

mikehardy requested review from Ehesp and Salakar and removed request for Ehesp May 4, 2020 15:10

mikehardy reviewed May 4, 2020

View reviewed changes

.../main/java/io/invertase/firebase/crashlytics/ReactNativeFirebaseCrashlyticsInitProvider.java Show resolved Hide resolved

mikehardy reviewed May 4, 2020

View reviewed changes

packages/crashlytics/ios/RNFBCrashlytics/RNFBCrashlyticsModule.m Show resolved Hide resolved

mikehardy reviewed May 4, 2020

View reviewed changes

packages/crashlytics/ios/RNFBCrashlytics/RNFBCrashlyticsModule.m Show resolved Hide resolved

mikehardy requested changes May 4, 2020

View reviewed changes

mikehardy mentioned this pull request May 4, 2020

🔥Crashlytics: recordError method not working on iOS #3594

Closed

7 tasks

Salakar requested review from russellwheatley and dackers86 May 4, 2020 19:33

Salakar added the plugin: crashlytics Firebase Crashlytics label May 6, 2020

This was referenced May 10, 2020

Crashlytics setup documentation results in an app that won't compile #3637

Closed

Why do we use fabric for crashlytics? #3510

Closed

deecewan reviewed May 12, 2020

View reviewed changes

docs/crashlytics/ios-setup.md Outdated Show resolved Hide resolved

Salakar self-assigned this May 12, 2020

Ehesp mentioned this pull request May 14, 2020

Crashlytics does not work [iOS] #3650

Closed

This was referenced May 21, 2020

The library dos not supported for new Crashlytics . we can not use Fabric anymore. #3482

Closed

Android - Crashlytics logs not showing in crashes, on the Firebase Console #3648

Closed

Aure77 and others added 6 commits June 30, 2020 10:05

feat(crashlytics,android): move to new Firebase crashlytics SDK

0a5fd4e

Remove setUserName/setUserEmail methods (removed from SDK)

feat(crashlytics,ios): move to new Firebase crashlytics SDK

fa214c2

fix(crashlytics): typescript type test

58c8583

fix(crashlytics): e2e tests

6edd200

fix(crashlytics): remove explicit FirebaseCrashlytics import

60f68f1

Update docs/crashlytics/ios-setup.md

be48899

Co-authored-by: David Buchan-Swanson <[email protected]>

Aure77 dismissed mikehardy’s stale review via e94b376 June 30, 2020 08:21

Aure77 force-pushed the feature/new-firebase-crashlytics-sdk branch from 5a72ffc to e94b376 Compare June 30, 2020 08:21

Aure77 force-pushed the feature/new-firebase-crashlytics-sdk branch from 79e6e68 to cc4f018 Compare June 30, 2020 08:53

fix(crashlytics) use FirebaseCrashlyticsCollectionEnabled in plist

65c9edd

Thx to @TPXP

Aure77 force-pushed the feature/new-firebase-crashlytics-sdk branch from cc4f018 to 65c9edd Compare June 30, 2020 09:57

TPXP approved these changes Jun 30, 2020

View reviewed changes

mikehardy approved these changes Jun 30, 2020

View reviewed changes

Salakar approved these changes Jun 30, 2020

View reviewed changes

Salakar changed the title ~~feat(crashlytics): Upgrade to new Firebase Crashlytics SDK~~ feat(crashlytics)!: upgrade to new Firebase Crashlytics SDK Jun 30, 2020

Salakar merged commit cad58e1 into invertase:master Jun 30, 2020

Aure77 deleted the feature/new-firebase-crashlytics-sdk branch June 30, 2020 20:37

andersonaddo mentioned this pull request Jul 4, 2020

Docs inconsistency about the location of apply plugin: 'com.google.gms.google-services' #3875

Closed

This was referenced Jul 7, 2020

Release Status invertase/react-native-notifee#1

Closed

Crashlytics documentation for setUserName and setUserEmail is possibly outdated #3908

Closed

mikehardy mentioned this pull request Jul 17, 2020

iOS Crashlytics setup broken link #3970

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(crashlytics)!: upgrade to new Firebase Crashlytics SDK #3580

feat(crashlytics)!: upgrade to new Firebase Crashlytics SDK #3580

Aure77 commented Apr 29, 2020 •

edited

Loading

CLAassistant commented Apr 29, 2020 •

edited

Loading

mikehardy commented Apr 30, 2020

mikehardy May 4, 2020

Aure77 May 4, 2020

mikehardy left a comment

mikehardy May 4, 2020

Aure77 commented May 4, 2020

Salakar commented May 12, 2020 •

edited

Loading

Aure77 commented May 13, 2020

Salakar commented May 23, 2020

Aure77 commented Jun 30, 2020

TPXP commented Jun 30, 2020

mikehardy commented Jun 30, 2020

mikehardy left a comment

andersonaddo commented Jun 30, 2020

Salakar commented Jun 30, 2020

mikehardy commented Jun 30, 2020

Salakar commented Jun 30, 2020

mikehardy commented Jul 1, 2020

feat(crashlytics)!: upgrade to new Firebase Crashlytics SDK #3580

feat(crashlytics)!: upgrade to new Firebase Crashlytics SDK #3580

Conversation

Aure77 commented Apr 29, 2020 • edited Loading

Description

Release Summary

Checklist

Test Plan

CLAassistant commented Apr 29, 2020 • edited Loading

mikehardy commented Apr 30, 2020

mikehardy May 4, 2020

Choose a reason for hiding this comment

Aure77 May 4, 2020

Choose a reason for hiding this comment

mikehardy left a comment

Choose a reason for hiding this comment

mikehardy May 4, 2020

Choose a reason for hiding this comment

Aure77 commented May 4, 2020

Salakar commented May 12, 2020 • edited Loading

Aure77 commented May 13, 2020

Salakar commented May 23, 2020

Aure77 commented Jun 30, 2020

TPXP commented Jun 30, 2020

mikehardy commented Jun 30, 2020

mikehardy left a comment

Choose a reason for hiding this comment

andersonaddo commented Jun 30, 2020

Salakar commented Jun 30, 2020

mikehardy commented Jun 30, 2020

Salakar commented Jun 30, 2020

mikehardy commented Jul 1, 2020

Aure77 commented Apr 29, 2020 •

edited

Loading

CLAassistant commented Apr 29, 2020 •

edited

Loading

Salakar commented May 12, 2020 •

edited

Loading