Skip to content
This repository was archived by the owner on Feb 22, 2023. It is now read-only.
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -24,15 +24,16 @@ abstract class InAppPurchasePlatform extends PlatformInterface {
/// The instance of [InAppPurchasePlatform] to use.
///
/// Defaults to `null`.
Copy link
Contributor

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

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;
Copy link
Contributor

Choose a reason for hiding this comment

The 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: platform-specific

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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 {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@stuartmorgan

I'm making the InAppPurchasePlatformAddition also a PlatformInterface.
We didn't talk about this offline but I think this makes sense. Please share your thoughts.

Copy link
Contributor

Choose a reason for hiding this comment

The 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 PlatformInterface they would be applicable to these addition classes.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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 PlatformInterface.
I couldn't think of a scenario where we need to add some methods in the "PlatformAddition" that will not be applicable to all addition classes.

Also, Now thinking about the usage of other "PlatformInterface"s, I'm not confident with InAppPurchasePlatformAddition being a "PlatformInterface". Maybe for this plugin i'll just duplicate the token verification code instead of extending PlatformInterface.

WDYT?

Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't the verifyToken meant to enforce users extend the class instead of implementing it?

Is this a concern for the InAppPurchasePlatformAddition class?

I don't think we will need to be afraid of the InAppPurchasePlatformAddition class to have any methods. Meaning I don't expect any methods to be added in the future that would break implementing classes.

Copy link
Contributor Author

@cyanglaz cyanglaz Apr 23, 2021

Choose a reason for hiding this comment

The 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 InAppPurchasePlatform.

Make sure an addition class to extend InAppPurchasePlatformAddition is one of the solutions.

Copy link
Contributor

Choose a reason for hiding this comment

The 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 InAppPurchasePlatform.

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).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If the addition is a type of InAppPurchasePlatform, the plugin user would easily make a mistake to access the InAppPurchasePlatform object (e.g. InAppPurchasePlatformAndroid) and call the unified API there.

Copy link
Contributor

Choose a reason for hiding this comment

The 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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let's say we allow an InAppPurchasePlatform to also be a type of InAppPurchasePlatformAddition, the implementer could potentially create a class like this:

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.

Copy link
Contributor

Choose a reason for hiding this comment

The 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 InAppPurchaseGoogleplayAdditions public instead of private would be. I.e., it's the implementor of the addition class designing their own extension API badly.

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
Expand All @@ -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) {
Expand All @@ -36,5 +52,13 @@ abstract class InAppPurchasePlatformAddition {
/// }
/// }
/// ```
static InAppPurchasePlatformAddition? instance;
static InAppPurchasePlatformAddition get instance => _instance;
Copy link
Contributor

Choose a reason for hiding this comment

The 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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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
Expand Up @@ -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(
Expand Down Expand Up @@ -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 {
Expand All @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

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

This shouldn't still be needed, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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;
}
}