Finalize federated architecture #40

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

Jump to bottom

Merged

paulppn merged 10 commits into Baseflow:main from JeroenWeener:enhancement/finalize-federated-architecture

Mar 23, 2023

Merged

Finalize federated architecture #40

paulppn merged 10 commits into Baseflow:main from JeroenWeener:enhancement/finalize-federated-architecture

Mar 23, 2023

Conversation

JeroenWeener

Copy link

Contributor

@JeroenWeener JeroenWeener commented Mar 14, 2023

✨ What kind of change does this PR introduce? (Bug fix, feature, docs update...)

This PR updates the app-facing package of the google-api-availability to make use of the previously published google-api-availability-platform-interface and google-api-availability-android packages. 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.

⤵️ What is the current behavior?

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

All projects build
Follows style guide lines (code style guide)
Relevant documentation was updated
Rebased onto current develop

@JeroenWeener JeroenWeener requested a review from mvanbeusekom

March 14, 2023 10:46

@JeroenWeener JeroenWeener self-assigned this

Mar 14, 2023

paulppn

paulppn suggested changes

Mar 14, 2023

View reviewed changes

Copy link

@paulppn paulppn left a comment

Choose a reason for hiding this comment

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

Some small things from me

google_api_availability_android/CHANGELOG.md Outdated

@@ -1,3 +1,7 @@

## 1.0.0+1

* Declares `dartPluginClass` in pubspec declaration.

Copy link

@paulppn paulppn Mar 14, 2023

Choose a reason for hiding this comment

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

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?

Copy link

Contributor Author

@JeroenWeener JeroenWeener Mar 14, 2023

Choose a reason for hiding this comment

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

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.

google_api_availability/lib/google_api_availability.dart Outdated

@@ -1,4 +1,4 @@

library google_api_availability;

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';

Copy link

@paulppn paulppn Mar 14, 2023

Choose a reason for hiding this comment

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

I don't think this is correct, why would you export a different package?

Copy link

Contributor Author

@JeroenWeener JeroenWeener Mar 14, 2023

Choose a reason for hiding this comment

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

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.

google_api_availability/lib/src/google_api_availability.dart

/// Creates an instance of the [GoogleApiAvailability] class.

///

/// This constructor is exposed for testing purposes only and should not be

/// used by clients of the plugin as it may break or change at any time.

@visibleForTesting

const GoogleApiAvailability.private();

/// Acquires an instance of the [GoogleApiAvailability] class.

static const GoogleApiAvailability instance = GoogleApiAvailability._();

Copy link

@paulppn paulppn Mar 14, 2023

Choose a reason for hiding this comment

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

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?

Copy link

Contributor Author

@JeroenWeener JeroenWeener Mar 14, 2023

Choose a reason for hiding this comment

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

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.