Skip to content
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

chore: Migrate firebase_core to nnbd #4656

Merged
merged 31 commits into from
Jan 15, 2021
Merged

Conversation

rrousselGit
Copy link
Contributor

Description

This migrates firebase_core, firebase_core_platform_interface and firebase_core_web to non-nullable type (but not firebase_core/example)

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.

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • My PR includes unit or integration tests for all changed/updated/fixed behaviors (See Contributor Guide).
  • All existing and new tests are passing.
  • I updated/added relevant documentation (doc comments with ///).
  • The analyzer (flutter analyze) does not report any problems on my PR.
  • I read and followed the Flutter Style Guide.
  • I signed the CLA.
  • I am willing to follow-up on review comments in a timely manner.

Breaking Change

Does your PR require plugin users to manually update their apps to accommodate your change?

  • Yes, this is a breaking change (please indicate a breaking change in CHANGELOG.md and increment major revision).
  • No, this is not a breaking change.

Comment on lines +38 to +51
Future<FirebaseAppPlatform> initializeApp({
String? name,
FirebaseOptions? options,
}) async {
return FakeFirebaseAppPlatform();
}

@override
FirebaseAppPlatform app([String name = defaultFirebaseAppName]) {
return null;
return FakeFirebaseAppPlatform();
}

@override
List<FirebaseAppPlatform> get apps => null;
List<FirebaseAppPlatform> get apps => [];
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These changes were necessary to make the tests compile. Afaik this doesn't matter but it'd be worth double checking that this doesn't change the behaviour of the related tests

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change looks ok to me

@rrousselGit rrousselGit merged commit e0fc653 into master Jan 15, 2021
@rrousselGit rrousselGit deleted the migrate_firebase_core_to_nnbd branch January 15, 2021 16:02
@firebase firebase locked and limited conversation to collaborators Feb 15, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants