feat!: Migrate firebase_analytics to sound null safety #5341
Conversation
|
@Salakar any idea why the iOS build fails? 🤔 When I run the command locally on my MacBook it works: By the way, PR was closed by accident ;) |
packages/firebase_analytics/firebase_analytics/test/observer_test.dart
Outdated
Show resolved
Hide resolved
|
I want to make some changes, but I can't edit this PR, so I created a PR to your repo (simpleclub-extended#2). @IchordeDionysos Can you review and merge it? It includes analytics example migration and uses plugin_platform_interface. |
This reverts commit 6191c38.
|
@untp thanks for your PR incorporated what I could, unfortunately migrating the example to null-safety is not possible yet integration tests null-safety is not yet in stable. Also, I did not want to migrate to the platform interfaces to minimize the scope of the PR. :) @rrousselGit @Salakar @Ehesp can you review the PR? 😌 |
|
That's a must!
…On Fri, Mar 19, 2021 at 1:22 PM Dennis Kugelmann ***@***.***> wrote:
@rrousselGit <https://github.com/rrousselGit> I've implemented your
suggestions. For it to work properly on web I had to adjust the requirement
for firebase that you changed 10 days ago in this PR #5276
<#5276>.
What do you think of requiring version ^9.0.1 of firebase?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#5341 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAAEFCVGL42UD3QNI3ZSQKDTEOW7LANCNFSM4ZE5XD5A>
.
|
|
The |
|
@sysint64 thanks for the hint, somehow forgot to update the types in the main package... |
|
@Salakar @rrousselGit how should we go forward with this PR to get null-safety for Firebase Analytics? :) |
packages/firebase_analytics/firebase_analytics/lib/firebase_analytics.dart
Outdated
Show resolved
Hide resolved
packages/firebase_analytics/firebase_analytics/lib/firebase_analytics.dart
Outdated
Show resolved
Hide resolved
|
It appears that I do not have the permissions to commit on this PR Could you sync the branch with master and revert the CI update? |
IchordeDionysos
changed the title
Co-authored-by: Remi Rousselet <[email protected]>
|
@rrousselGit I've made the adjustments, but for some reason, the two integration checks for Android/iOS aren't starting successfully and are stuck queueing ... |
|
They can take a while to start, because there's a limit on how many checks can be performed at the same time It appears that they properly started and failed though. I'll retry them |
|
Looks like everything passed after a retry. It's good to go then 😄 |
|
Hi guys, thanks for merging this 👍. After updating to nullsafety getting error in this part, is it okay to cast it?
|
|
I'm currently using the git-sources package with |
|
@abegehr pre-release version was just published :) @rrousselGit what do you suggest with the Object vs dynamic thing that @NarHakobyan brought up. In general, I try to use Object rather than dynamic. (You know the reasons :D), but in this case it might make the API less clean, as the Flutter API returns dynamic 🤔 I would revert it to the previous behavior. Do you agree? |
Description
This migrates firebase_analytics 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.yamland 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?