-
Notifications
You must be signed in to change notification settings - Fork 9.6k
[in_app_purchase] platform interface improvement #3821
Changes from 2 commits
674bddd
875d3f1
06daea2
7f1653f
0f3d0b9
899bafb
8a7341d
a697f09
b8774b6
2d9c2ae
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -24,15 +24,16 @@ abstract class InAppPurchasePlatform extends PlatformInterface { | |
| /// The instance of [InAppPurchasePlatform] to use. | ||
| /// | ||
| /// Defaults to `null`. | ||
| static InAppPurchasePlatform? get instance => _instance; | ||
| static InAppPurchasePlatform get instance => _instance; | ||
|
|
||
| static InAppPurchasePlatform? _instance; | ||
| // Should only be accessed after setter is called. | ||
| static late InAppPurchasePlatform _instance; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This should be either above or below the getter and setter, rather than between them.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. done |
||
|
|
||
| /// Platform-specific plugins should set this with their own platform-specific | ||
| /// class that extends [InAppPurchasePlatform] when they register themselves. | ||
| // TODO(amirh): Extract common platform interface logic. | ||
| // https://github.com/flutter/flutter/issues/43368 | ||
| static void setInstance(InAppPurchasePlatform instance) { | ||
| static set instance(InAppPurchasePlatform instance) { | ||
| PlatformInterface.verifyToken(instance, _token); | ||
| _instance = instance; | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2,18 +2,34 @@ | |
| // Use of this source code is governed by a BSD-style license that can be | ||
| // found in the LICENSE file. | ||
|
|
||
| // ignore: avoid_classes_with_only_static_members | ||
| import 'package:in_app_purchase_platform_interface/in_app_purchase_platform_interface.dart'; | ||
| import 'package:plugin_platform_interface/plugin_platform_interface.dart'; | ||
|
|
||
| /// The interface that platform implementations must implement when they want to | ||
| /// provide platform specific in_app_purchase features. | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit: platform-specific
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. done. Also fixed some other appearance in the plugin. |
||
| abstract class InAppPurchasePlatformAddition { | ||
| /// | ||
| /// Platform implementations should extend this class rather than implement it as `in_app_purchase` | ||
| /// does not consider newly added methods to be breaking changes. Extending this class | ||
| /// (using `extends`) ensures that the subclass will get the default implementation, while | ||
| /// platform implementations that `implements` this interface will be broken by newly added | ||
| /// [InAppPurchasePlatformAddition] methods. | ||
| abstract class InAppPurchasePlatformAddition extends PlatformInterface { | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm making the
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What's the advantage of doing this? It is not at all clear to me that if we added new methods to
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is mainly to take advantage of using the token verification of the Also, Now thinking about the usage of other "PlatformInterface"s, I'm not confident with WDYT?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Isn't the Is this a concern for the I don't think we will need to be afraid of the
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We do need some sort of enforcement to make sure the addition is not a type of Make sure an addition class to extend
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Why do we need to enforce that? The concern is purely around pubspec dependency cycles with endorsement IIRC, in which case it's the pubspec cycle, not the classes, that are the issue. This seems like a very indirect way of trying to enforce a lack of cycles (and is both not sufficient to prevent a cycle, and not necessary in the case of an in-package implementation if this is something we adopt as a general solution).
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If the addition is a type of
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sorry, I'm not following. Can you give a snippet of what that mistake would look like in code?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Let's say we allow an class InAppPurchasePlatformGooglePlay extends InAppPurchasePlatform implements InAppPurchasePlatformAddition {
@override
Future<void> buyConsumable(...){...}
Future<void> someAdditionMethod(...){...}
}Then the user could potentially use this class to do something like: InAppPurchasePlatformGooglePlay googlePlayAddition = InAppPurchaseConnection.instance.getPlatformAddition<InAppPurchasePlatformGooglePlay>();
googlePlayAddition.buyConsumable(...);In this case, the user is accessing the platform interface API directly, which we want to avoid.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think we need to go out of our way to forbid that construction. It's a bad idea, but only in the same way that, say, making a bunch of implementation details of a What I wanted to avoid was the design where we forced extensions to use that, so that every extension would be required to have that poor design. If we make it so that it's easy to do the right thing, and model the right thing in our plugins, that should be enough. |
||
| /// Constructs a InAppPurchasePlatform. | ||
| InAppPurchasePlatformAddition() : super(token: _token); | ||
|
|
||
| static final Object _token = Object(); | ||
|
|
||
| // Should only be accessed after setter is called. | ||
| static late InAppPurchasePlatformAddition _instance; | ||
|
|
||
| /// The instance containing the platform-specific in_app_purchase | ||
| /// functionality. | ||
| /// | ||
| /// To implement additional functionality extend | ||
| /// [`InAppPurchasePlatformAddition`][3] with the platform-specific | ||
| /// functionality, and when the plugin is registered, set the | ||
| /// `InAppPurchasePlatformAddition.instance` with the new addition | ||
| /// implementationinstance. | ||
| /// implementation instance. | ||
| /// | ||
| /// Example implementation might look like this: | ||
| /// ```dart | ||
|
|
@@ -22,7 +38,7 @@ abstract class InAppPurchasePlatformAddition { | |
| /// } | ||
| /// ``` | ||
| /// | ||
| /// The following snippit shows how to register the `InAppPurchaseMyPlatformAddition`: | ||
| /// The following snippet shows how to register the `InAppPurchaseMyPlatformAddition`: | ||
| /// ```dart | ||
| /// class InAppPurchaseMyPlatformPlugin { | ||
| /// static void registerWith(Registrar registrar) { | ||
|
|
@@ -36,5 +52,13 @@ abstract class InAppPurchasePlatformAddition { | |
| /// } | ||
| /// } | ||
| /// ``` | ||
| static InAppPurchasePlatformAddition? instance; | ||
| static InAppPurchasePlatformAddition get instance => _instance; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why have you made this non-nullable? Why should a platform with no additions need to set this up?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah makes sense! Made this nullable. |
||
|
|
||
| /// Platform-specific plugins should set this with their own platform-specific | ||
| /// class that extends [InAppPurchasePlatformAddition] when they register themselves. | ||
| static set instance(InAppPurchasePlatformAddition instance) { | ||
| PlatformInterface.verifyToken(instance, _token); | ||
| assert(instance.runtimeType is! InAppPurchasePlatform); | ||
| _instance = instance; | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -11,22 +11,18 @@ void main() { | |
| TestWidgetsFlutterBinding.ensureInitialized(); | ||
|
|
||
| group('$InAppPurchasePlatform', () { | ||
| test('Default instance should return null', () { | ||
| expect(InAppPurchasePlatform.instance, null); | ||
| }); | ||
|
|
||
| test('Cannot be implemented with `implements`', () { | ||
| expect(() { | ||
| InAppPurchasePlatform.setInstance(ImplementsInAppPurchasePlatform()); | ||
| InAppPurchasePlatform.instance = ImplementsInAppPurchasePlatform(); | ||
| }, throwsNoSuchMethodError); | ||
| }); | ||
|
|
||
| test('Can be extended', () { | ||
| InAppPurchasePlatform.setInstance(ExtendsInAppPurchasePlatform()); | ||
| InAppPurchasePlatform.instance = ExtendsInAppPurchasePlatform(); | ||
| }); | ||
|
|
||
| test('Can be mocked with `implements`', () { | ||
| InAppPurchasePlatform.setInstance(MockInAppPurchasePlatform()); | ||
| InAppPurchasePlatform.instance = MockInAppPurchasePlatform(); | ||
| }); | ||
|
|
||
| test( | ||
|
|
@@ -124,6 +120,34 @@ void main() { | |
| ); | ||
| }); | ||
| }); | ||
|
|
||
| group('$InAppPurchasePlatformAddition', () { | ||
| test('Cannot be implemented with `implements`', () { | ||
| expect(() { | ||
| InAppPurchasePlatformAddition.instance = | ||
| ImplementsInAppPurchasePlatformAddition(); | ||
| }, throwsNoSuchMethodError); | ||
| }); | ||
|
|
||
| test('InAppPurchasePlatformAddition Can be extended', () { | ||
| InAppPurchasePlatformAddition.instance = | ||
| ExtendsInAppPurchasePlatformAddition(); | ||
| }); | ||
|
|
||
| test('Can be mocked with `implements`', () { | ||
| InAppPurchasePlatformAddition.instance = | ||
| MockInAppPurchasePlatformAddition(); | ||
| }); | ||
|
|
||
| test('Provider can provide', () { | ||
| ImplementsInAppPurchasePlatformAdditionProvider.register(); | ||
| final ImplementsInAppPurchasePlatformAdditionProvider provider = | ||
| ImplementsInAppPurchasePlatformAdditionProvider(); | ||
| final InAppPurchasePlatformAddition addition = | ||
| provider.getPlatformAddition(); | ||
| expect(addition.runtimeType, ExtendsInAppPurchasePlatformAddition); | ||
| }); | ||
| }); | ||
| } | ||
|
|
||
| class ImplementsInAppPurchasePlatform implements InAppPurchasePlatform { | ||
|
|
@@ -143,3 +167,32 @@ class ExtendsInAppPurchasePlatform extends InAppPurchasePlatform {} | |
| class MockPurchaseParam extends Mock implements PurchaseParam {} | ||
|
|
||
| class MockPurchaseDetails extends Mock implements PurchaseDetails {} | ||
|
|
||
| class ImplementsInAppPurchasePlatformAddition | ||
| implements InAppPurchasePlatformAddition { | ||
| @override | ||
| dynamic noSuchMethod(Invocation invocation) => super.noSuchMethod(invocation); | ||
| } | ||
|
|
||
| class MockInAppPurchasePlatformAddition extends Mock | ||
| with | ||
| // ignore: prefer_mixin | ||
| MockPlatformInterfaceMixin | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This shouldn't still be needed, right?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. done |
||
| implements | ||
| InAppPurchasePlatformAddition {} | ||
|
|
||
| class ExtendsInAppPurchasePlatformAddition | ||
| extends InAppPurchasePlatformAddition {} | ||
|
|
||
| class ImplementsInAppPurchasePlatformAdditionProvider | ||
| implements InAppPurchasePlatformAdditionProvider { | ||
| static void register() { | ||
| InAppPurchasePlatformAddition.instance = | ||
| ExtendsInAppPurchasePlatformAddition(); | ||
| } | ||
|
|
||
| @override | ||
| T getPlatformAddition<T extends InAppPurchasePlatformAddition>() { | ||
| return InAppPurchasePlatformAddition.instance as T; | ||
| } | ||
| } | ||
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.
This is no longer correct. Instead, you should document that it must be set before using.
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.
done