Finalize federated architecture #40
Conversation
| @@ -1,3 +1,7 @@ | |||
| ## 1.0.0+1 | |||
|
|
|||
| * Declares `dartPluginClass` in pubspec declaration. | |||
There was a problem hiding this comment.
I am not sure why these files are in the PR, they are already in the main branch right? Is your branch up to date with main?
There was a problem hiding this comment.
Turns out it was not. I have reread https://github.com/flutter/flutter/wiki/Tree-hygiene to learn how to do it and fixed it.
|
|
||
| export 'src/google_api_availability.dart'; | ||
| export 'src/models/google_play_services_availability.dart'; | ||
| export 'package:google_api_availability_platform_interface/google_api_availability_platform_interface.dart'; |
There was a problem hiding this comment.
I don't think this is correct, why would you export a different package?
There was a problem hiding this comment.
We export a model that can be returned from a function call. As the model resides in the platform code, we need to export it here (as the end-user might not have a dependency on the platform interface).
I changed the export to export only the model itself, as I do agree this is exposes too much.
| const GoogleApiAvailability.private(); | ||
|
|
||
| /// Acquires an instance of the [GoogleApiAvailability] class. | ||
| static const GoogleApiAvailability instance = GoogleApiAvailability._(); |
There was a problem hiding this comment.
I am genuinly not sure about this, so just asking, but does GoogleApiAvailability._() equal null? Since you use that below to actually check both of them, why not just change it here to a nullable?
There was a problem hiding this comment.
I agree that this might be a little confusing; there is both GoogleApiAvailability and GoogleApiAvailabilityPlatform, both implementing the singleton pattern (having instances). The _() and private() constructors of GoogleApiAvailability construct an instance of the class. In Dart, things starting with an _ are considered private, and cannot be called by outsiders. This is great for implementing the singleton pattern, as we can now prevent the end user from accidentally instantiating the class multiple times. To make sure tests can still instantiate as much as they want, we expose the constructor private(), marked with @visibleForTesting.
The check we do at the method calls considers the instance of the platform interface, and is unrelated to GoogleApiAvailability.
JeroenWeener
force-pushed
the
enhancement/finalize-federated-architecture
branch
from
d662303 to
0783ef5
Compare
paulppn
left a comment
paulppn
left a comment
There was a problem hiding this comment.
Two small nits, if you change those it is fine with me.
google_api_availability/CHANGELOG.md
Outdated
| @@ -1,11 +1,16 @@ | |||
| ## 5.0.0 | |||
|
|
|||
| * **BREAKING CHANGE**: No longer supports iOS, as it was never truly supported. | |||
There was a problem hiding this comment.
"Removes support for iOS, as it was never truly supported."
google_api_availability/pubspec.yaml
Outdated
| sdk: flutter | ||
| meta: ^1.8.0 | ||
| google_api_availability_android: ^1.0.0+1 | ||
| google_api_availability_platform_interface: ^1.0.0 |
There was a problem hiding this comment.
You can also upgrade this version since I merged the other one, Maurits will have to release all of them though.
JeroenWeener
force-pushed
the
enhancement/finalize-federated-architecture
branch
from
83c5341 to
8848f15
Compare
2 participants
✨ What kind of change does this PR introduce? (Bug fix, feature, docs update...)
This PR updates the app-facing package of the
google-api-availabilityto make use of the previously publishedgoogle-api-availability-platform-interfaceandgoogle-api-availability-androidpackages. This concludes #33, finishing our work on making this plugin use the federated architecture.In the process, the example has been updated and an issue in the PR template has been resolved.
The package contains an android implementation and uses this to achieve its functionality.
🆕 What is the new behavior (if this is a feature change)?
The package will use the aforementioned packages to achieve the same functionality in a federated way.
💥 Does this PR introduce a breaking change?
Yes it does, as we no longer support iOS (we never actually did, but now we mention it).
🐛 Recommendations for testing
App-facing tests have been updated.
As models have moved around, we should make sure that all the right classes are exposed to the end-user.
📝 Links to relevant issues/docs
Closes #33.
🤔 Checklist before submitting