-
Notifications
You must be signed in to change notification settings - Fork 4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[firebase_crashlytics] Migrate to new Crashlytics SDK #2288
Conversation
@axel-op I believe the title of the PR needs amending to start with |
@MaikuB Thanks, it's done. What do you think of the PR? |
@axel-op I'm not familiar with the APIs to comment. Came across your PR as another member in the community mentioned it :) |
14966df
to
b22a659
Compare
packages/firebase_crashlytics/darwin/Classes/FirebaseCrashlyticsPlugin.m
Outdated
Show resolved
Hide resolved
packages/firebase_crashlytics/darwin/Classes/FirebaseCrashlyticsPlugin.m
Show resolved
Hide resolved
5de1034
to
748633d
Compare
I'll definitely need help for the iOs part. |
What do we need to do to get this moving ... it's already very urgent. |
2f337ad
to
d49998a
Compare
I have to update the README file |
8ec938a
to
5f88c57
Compare
Wow ... @axel-op great progress 🎉🎉🎉 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good, just some beta version updates.
5f88c57
to
08e56dd
Compare
Thanks @workerbee22 for pointing that out :) |
@workerbee22 and @creativecreatorormaybenot is everything OK for you? |
It seems as though my feedback has been addressed 👍🏼 |
Well, I was just asking for your opinion, since I knew you would answer much faster ;) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Noted 👍 |
If that's the case then it worth debugging the plugin further as it could mean that this isn't being called. One alternative is to have the plugin register to receive AppDelegate calls and call |
@MaikuB Thanks! I'm not sure I should make these changes right now, as I believe the current rework by the people from Invertase will probably bring some important changes to the native part of all Firebase plugins, as this comment suggests. |
With the upcoming changes, We should have a PR up soon which overhauls Core, along with other changes (CI, tests etc) so it'd make more sense to sync this work up with that. |
@Ehesp has the team looked at minimising the amount of code users need to put into their AppDelegate file as well? Flutter already provides mechanisms for plugins to receive AppDelegate calls. It looks like there's potential opportunity to do that in this PR as well |
Yep that was a terrible example, we're doing that already - will link to the PR when it's up. |
This is slowly getting urgent as the Firebase team is sending out automated emails in regards to the Fabric Crashlytics SDK now being officially deprecated and will stop working on November 15, 2020. |
implementation 'com.google.firebase:firebase-analytics:17.4.0' | ||
|
||
// Add the Firebase SDK for Crashlytics. | ||
implementation 'com.google.firebase:firebase-crashlytics:17.0.0' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This dependency is already in the firebase_crashlytics
package ./android/build.gradle
file, right? We don't need to add it to our Flutter app build.gradle file IMO.
Update: Works fine without it in my case. I don't think the whole fourth step is needed. It's even a bit confusing and misleading to add the firebase-analytics
dependency too here since it's not necessary for Firebase Crashlytics to work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah if it's already a dependency used by the plugin then developers shouldn't need to add it to their app's build.gradle file
cc @axel-op
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ditto for the firebase_analytics
! I don't like the way the official documentation like this: https://firebase.google.com/docs/flutter/setup?platform=ios
Encourages other packages. It just leads to possible reader confusion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will be updated when the package will depend on the new versions of firebase_core
.
Do we have any updates on the status of this PR @axel-op ? I can help if needed but I'm pretty sure this PR is becoming quite urgent and it would be great to be able to push it asap ;) |
Any updates? The discrepancy of documentations is very confusing now. |
I'm gonna work on it this weekend. |
f9100e2
to
66d2d14
Compare
I think that we should now wait for the new releases of |
@axel-op @Ehesp What is a good way to support NDK crash reports using the new SDK? See this page for a example on the native integration: https://firebase.google.com/docs/crashlytics/ndk-reports It seems that integration of NDK crash reports requires a dependency change and some additional gradle configuration. Perhaps it is enough to mention it in the README? |
firebase_core-v0.5.0-dev.1 is released! When will this pr be merged? |
Hey everyone, good news; Crashlytics has been bumped to the top of the priority list on the roadmap #2582 - so we'll be looking at this next. What would be helpful is if anyone that has been using this PR in production could respond here if there's still any outstanding issues they've discovered while using the code in this PR, thanks! @axel-op if you're willing, do you have any form of messaging so we can chat quicker and help get this PR merged quicker? I'm |
Hey all 👋 I've started the process of breaking out the work done by @axel-op into a federated plugin setup, along with some QOL updates to get it ready for review and release. The PR is in-progress here https://github.com/invertase/flutterfire/pull/41 Thanks again for this contribution @axel-op 🥳 |
@Ehesp Thanks |
Any updates on this PR ? ☺ |
Nope, currently going through updates and review still. |
Is there progress on this PR ? And is there any way for me to help ? Would gladly help is you need some ;) |
@blaxou , the PR is under active development, but at a new location https://github.com/invertase/flutterfire/pull/41. Latest activity was 4 days ago. |
Closing in favour of #3420 |
Description
This PR updates the Crashlytics implementation to use the new SDK, which doesn't use the Fabric SDK anymore: https://firebase.google.com/docs/crashlytics/get-started-new-sdk.
Related Issues
Checklist
Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes (
[x]
). This will ensure a smooth and quick review process.///
).flutter analyze
) does not report any problems on my PR.Breaking Change
Does your PR require plugin users to manually update their apps to accommodate your change?