[in_app_purchase] platform interface improvement

[cyanglaz]

Adds some improvements to the platform interface.

1. Make InAppPurchasePlatformAddition a subclass of PlatformInterface to take advantage of the existing token checking logic. (I also consider InAppPurchasePlatformAddition a PlatformInterface because it works in a similar way.
2. Make the instance variable late as we should never access it before setter.
3. Add tests for InAppPurchasePlatformAddition

part of flutter/flutter#78525

Pre-launch Checklist

• I read the Contributor Guide and followed the process outlined there for submitting PRs.
• I read the Tree Hygiene wiki page, which explains my responsibilities.
• I read and followed the Flutter Style Guide and the C++, Objective-C, Java style guides. (Note that unlike the flutter/flutter repo, the flutter/plugins repo does use dart format. See plugin_tool format)
• I signed the CLA.
• The title of the PR starts with the name of the plugin surrounded by square brackets, e.g. [shared_preferences]
• I listed at least one issue that this PR fixes in the description above.
• I updated pubspec.yaml with an appropriate new version according to the pub versioning philosophy.
• I updated CHANGELOG.md to add a description of the change.
• I updated/added relevant documentation (doc comments with ///).
• I added new tests to check the change I am making or feature I am adding, or Hixie said the PR is test exempt.
• All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

[cyanglaz]

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

[stuartmorgan]

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.

[cyanglaz]

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?

[mvanbeusekom]

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.

[cyanglaz]

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.

[stuartmorgan]

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

[cyanglaz]

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.

[stuartmorgan]

Sorry, I'm not following. Can you give a snippet of what that mistake would look like in code?

[cyanglaz]

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.

[stuartmorgan]

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.

[mvanbeusekom]

LGTM, only thing I am thinking about is adding another test to verify that accessing instance throws a LateInitializationError when accessing it before it is initialized.

[cyanglaz]

LateInitializationError

Sounds good, weird tho the code wasn't able to find LateInitializationError

[mvanbeusekom]

LateInitializationError

Sounds good, weird tho the code wasn't able to find LateInitializationError

I tested it on dartpad.dev since I wasn't sure what would happen ;)

[cyanglaz]

@mvanbeusekom Seems like the class LateInitializationError isn't accessible. I think it's ok to not test a default NNBD behavior.

[stuartmorgan]

This is no longer correct. Instead, you should document that it must be set before using.

[cyanglaz]

done

[stuartmorgan]

This should be either above or below the getter and setter, rather than between them.

[cyanglaz]

done

[stuartmorgan]

Nit: platform-specific

[cyanglaz]

done. Also fixed some other appearance in the plugin.

[stuartmorgan]

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.

[stuartmorgan]

Why have you made this non-nullable? Why should a platform with no additions need to set this up?

[cyanglaz]

Ah makes sense! Made this nullable.

[cyanglaz]

@stuartmorgan Updated per your comments. PTAL :)

[cyanglaz]

done

[cyanglaz]

done

[cyanglaz]

done. Also fixed some other appearance in the plugin.

[cyanglaz]

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?

[cyanglaz]

Ah makes sense! Made this nullable.

[cyanglaz]

@stuartmorgan Updated with review. comments about not making InAppPurchasePlatformAddition a platform interface.

[stuartmorgan]

LGTM with nits.

[stuartmorgan]

s/replace ... with/set ... to/ (by default there's no instance, so there's nothing to replace)

[cyanglaz]

done

[stuartmorgan]

s/implementers/implementations/

[cyanglaz]

done

[stuartmorgan]

"To avoid clients directly calling [InAppPurchasePlatform] APIs, an [InAppPurchasePlatformAddition] implementation should not be"

[cyanglaz]

done

[stuartmorgan]

Sets

[cyanglaz]

done

[stuartmorgan]

s/must/should/

[cyanglaz]

done

[stuartmorgan]

s/There's/There are/

[cyanglaz]

done

[stuartmorgan]

This shouldn't still be needed, right?

[cyanglaz]

done

[mvanbeusekom]

LGTM