-
Notifications
You must be signed in to change notification settings - Fork 4k
feat!: Migrate firebase_performance to sound null safety #5540
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
feat!: Migrate firebase_performance to sound null safety #5540
Conversation
Thanks for this. I know there's a known issue where the iOS CI e2e tests are failing to start as the application is probably crashing before it can start tests, it needs investigating really on our end and some simulator logs adding to CI so we can see what the cause may be. |
@Salakar locally the e2e tests did work as expected 👍 |
Great thanks! Are you able to provide screenshots of this for iOS - then CI shouldn't be a blocker as we can visually confirm |
Ahh, damn I only tested on Android... AndroidiOSFlutter crash report. commandflutter drive -d 00008101-0006285036C0001E --target=./test_driver/firebase_performance_e2e.dart --dart-define=CI=true exceptionHttpException: HttpException: , uri = http://127.0.0.1:52200/ws
flutter doctor
|
@@ -17,7 +15,7 @@ class FirebasePerformance { | |||
FirebasePerformance._(this._handle) { | |||
channel.invokeMethod<bool>( | |||
'FirebasePerformance#instance', | |||
<String, dynamic>{'handle': _handle}, | |||
<String, Object>{'handle': _handle}, |
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.
<String, Object?>
same for all the other maps in the PR
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.
Adjusted them :)
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.
I'm happy to ship ignoring iOS
CI as this is not something related to this PR (see comments above + screenshots of local testing working on for iOS).
Pending @rrousselGit's final approval of course :)
@rrousselGit can you review and merge this on monday? 🤔 |
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
Shipped, v0.7.0 is now available 🎉 Thank you for the PR once again @IchordeDionysos |
Description
This migrates firebase_performance to non-nullable types
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. Updating the
pubspec.yaml
and changelogs is not required.///
).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?