Skip to content
This repository has been archived by the owner on Feb 22, 2023. It is now read-only.

[in_app_purchase] Implementation of the app facing package #3877

Merged
merged 12 commits into from
May 12, 2021

Conversation

mvanbeusekom
Copy link
Contributor

@mvanbeusekom mvanbeusekom commented May 11, 2021

Implementation of the app facing package. This is the last part of the migration to the federated plugin architecture.

This PR is marked as draft and needs the following tasks to be completed:

Related issues:

If you had to change anything in the flutter/tests repo, include a link to the migration guide as per the breaking change policy.

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 cyanglaz marked this pull request as ready for review May 11, 2021 21:22
@cyanglaz cyanglaz requested a review from LHLL as a code owner May 11, 2021 21:22
Copy link
Contributor

@cyanglaz cyanglaz left a comment

Choose a reason for hiding this comment

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

Code mostly looks good. Also since there are API changes, don't we need to update README and example/README?

As part of implementing federated architecture and making the interface compatible for other platform this version contains the following **breaking changes**:

* Changes to the platform agnostic interface:
* The `InAppPurchaseConnection` class has been renamed to `InAppPurchase`. To access the generic InApp Purchase API use `InAppPurchase.instance`;
Copy link
Contributor

Choose a reason for hiding this comment

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

nits: maybe we can make this clearer such as

If you used inAppPurchaseConnection.instance to access generic In App Purchase APIs, please use InAppPurchase.instance instead.

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


* Changes to the platform agnostic interface:
* The `InAppPurchaseConnection` class has been renamed to `InAppPurchase`. To access the generic InApp Purchase API use `InAppPurchase.instance`;
* The `purchaseUpdatedStream` has been renamed to `purchaseStream`. Restored purchases are now also emitted on this stream and are marked with a `status` of `PurchaseStatus.restored`;
Copy link
Contributor

Choose a reason for hiding this comment

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

We don't have to mention "restored purchases are now also emitted on this stream and are marked with a status of PurchaseStatus.restored;"
Instead, we should give more details in the item below:

The InAppPurchaseConnection.queryPurchases method has been removed. Instead, you should use InAppPurchase.restorePurchases. This method emits each restored purchase on the InAppPurchase.purchaseStream, the purchase object will be marked with a status of PurchaseStatus.restored

Also, for each API, mention its class, for example, InAppPurchaseConnection.purchaseUpdatedStream, and InAppPurchase.purchaseStream

Ditto below for all the other items.

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

@@ -21,6 +21,7 @@ dependencies:
flutter:
sdk: flutter

json_annotation: ^4.0.1
Copy link
Contributor

Choose a reason for hiding this comment

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

Are the changes in in_app_purchase_android and in_app_purchase_ios here intentional?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, they have been solved with PR #3879 which wasn't merge at that time. Now that it is merged and I rebased the changes have been removed from this PR.

@mvanbeusekom
Copy link
Contributor Author

Code mostly looks good. Also since there are API changes, don't we need to update README and example/README?

Yes that is why this PR is still in draft (also see checklist in the description)

Copy link
Contributor

@cyanglaz cyanglaz left a comment

Choose a reason for hiding this comment

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

LGTM

@cyanglaz cyanglaz added the waiting for tree to go green (Use "autosubmit") This PR is approved and tested, but waiting for the tree to be green to land. label May 12, 2021
@fluttergithubbot
Copy link

This pull request is not suitable for automatic merging in its current state.

  • The status or check suite build-ipas+drive-examples has failed. Please fix the issues identified (or deflake) before re-applying this label.
  • This commit has empty status or empty checks. Please check the Google CLA status is present and Flutter Dashboard application has multiple checks.

@fluttergithubbot fluttergithubbot removed the waiting for tree to go green (Use "autosubmit") This PR is approved and tested, but waiting for the tree to be green to land. label May 12, 2021
@mvanbeusekom
Copy link
Contributor Author

Tree went red because the Xcode project (part of the in_app_purchase package) still referenced tests that had been moved to the in_app_purchase_ios package.

I have removed these tests as they don't belong here.

@mvanbeusekom mvanbeusekom merged commit d496845 into flutter:master May 12, 2021
@mvanbeusekom mvanbeusekom deleted the app_facing_package branch May 12, 2021 20:48
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request May 19, 2021
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request May 19, 2021
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request May 19, 2021
fotiDim pushed a commit to fotiDim/plugins that referenced this pull request Sep 13, 2021
)

* Start with app-facing package

* Update to pub version of platform interface

* Update CHANGELOG with feedback from PR

* Fix some spelling mistakes

* Update CHANGELOG with feedback from PR

* Update README with new features

* Update dependencies and links in documentation

* Remove iOS test from example project

* Remove test target from Podfile

* Remove test from Xcode scheme
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.