Skip to content
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

Extracted PurchasesType and PurchasesSwiftType #1912

Merged
merged 3 commits into from
Sep 21, 2022
Merged

Conversation

NachoSoto
Copy link
Contributor

@NachoSoto NachoSoto commented Sep 20, 2022

This is needed to fix RevenueCat/purchases-hybrid-common#229

purchases-hybrid-common had a hacky implementation of MockPurchases that subclassed Purchases.
Because of that, it had 100+ lines of code to initialize the instance, which was never used because all (most) methods were overridden.

This allows a much better way of doing that, which also enables any SDK user to mock Purchases.

Another benefit of this is that we can see at a glance what the public API is.
Also, all docstrings are now extracted into the protocol, which makes the implementation file shorter. The docs get inherited by the type itself.

All our API testers pass with no changes, and I've also added the updated RevenueCat-Swift.h to verify that no public API changed.

This was missed in #1879. Without this, we might have been forwarding some private errors instead of ensuring that we only sent `CodeError`s
This is needed to fix RevenueCat/purchases-hybrid-common#229

[`purchases-hybrid-common`](https://github.com/RevenueCat/purchases-hybrid-common) had a [hacky implementation of `MockPurchases`](https://github.com/RevenueCat/purchases-hybrid-common/blob/4.2.1/ios/PurchasesHybridCommon/PurchasesHybridCommonTests/Mocks/MockPurchases.swift) that subclassed `Purchases`.
Because of that, it had 100+ lines of code to initialize the instance, which was never used because all (most) methods were overridden.

This allows a much better way of doing that, **which also enables any SDK user** to mock `Purchases`.

Another benefit of this is that we can see at a glance what the public API is.
Also, all docstrings are now extracted into the protocol, which makes the implementation file shorter. The docs get inherited by the type itself.

All our API testers pass with no changes, and I've also added the updated `RevenueCat-Swift.h` to verify that no public API changed.
@NachoSoto NachoSoto changed the title [WIP] PurchasesType Extracted PurchasesType and PurchasesSwiftType Sep 20, 2022
@NachoSoto NachoSoto requested a review from a team September 20, 2022 21:07
@NachoSoto NachoSoto changed the base branch from main to non-async-public-error September 20, 2022 21:07
@NachoSoto NachoSoto marked this pull request as ready for review September 20, 2022 21:08
NachoSoto added a commit to RevenueCat/purchases-hybrid-common that referenced this pull request Sep 20, 2022
Copy link
Contributor

@tonidero tonidero left a comment

Choose a reason for hiding this comment

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

I'm assuming the documentation was only moved and didn't change? (I didn't read all the documentation).

My main concern as someone not familiar with swift is whether the change from a class to a protocol can in any way break user's implementations or tests... From looking at the changes in RevenueCat/purchases-hybrid-common#230, looks like we had to remove the override keyword in the tests, and the same might happen to users in their tests if they are trying to mock Purchases right? So I'm wondering if this would be considered a breaking change...

Sources/Purchasing/Purchases/PurchasesType.swift Outdated Show resolved Hide resolved
@NachoSoto
Copy link
Contributor Author

Yeah the docs are just a copy.

As for the override, that's a good question. Because the class wasn't open, it meant that it was implicitly final outside the module , so users couldn't even subclass it.

That's why this does allow users to mock the type, but without the dangers of subclassing.

Co-authored-by: Toni Rico <antonio.rico.diez@revenuecat.com>
Base automatically changed from non-async-public-error to main September 21, 2022 16:36
Copy link
Contributor

@tonidero tonidero left a comment

Choose a reason for hiding this comment

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

Ahh ok sounds good! Then this LGTM!

@aboedo
Copy link
Member

aboedo commented Sep 21, 2022

yeah the tests thing is more just fixing the terrible hack we used to have to do because we were doing inherit + replace instead of using a public interface for mocking in tests

@aboedo
Copy link
Member

aboedo commented Sep 21, 2022

And we were also using a @testable import, which does a few things specifically for testing (like making stuff open instead of final).
So... this would affect folks who were using @testable import Purchases in their own implementations, I suppose. But no one using in production as far as I understand it

@NachoSoto
Copy link
Contributor Author

So... this would affect folks who were using @testable import Purchases in their own implementations, I suppose. But no one using in production as far as I understand it

The class was already made final already so it could become Sendable. That's a requirement for it.

@NachoSoto NachoSoto merged commit 16b874e into main Sep 21, 2022
@NachoSoto NachoSoto deleted the purchases-type branch September 21, 2022 19:51
NachoSoto added a commit that referenced this pull request Sep 27, 2022
**This is an automatic release.**

### New Features
* 🚨 `StoreKit 2` is now enabled by default 🚨 (#1922) via NachoSoto
(@NachoSoto)
* Extracted `PurchasesType` and `PurchasesSwiftType` (#1912) via
NachoSoto (@NachoSoto)
### Bugfixes
* `StoreKit 1`: changed result of cancelled purchases to be consistent
with `StoreKit 2` (#1910) via NachoSoto (@NachoSoto)
* `PaymentQueueWrapper`: handle promotional purchase requests from App
Store when SK1 is disabled (#1901) via NachoSoto (@NachoSoto)
### Other Changes
* Fixed iOS 12 tests (#1936) via NachoSoto (@NachoSoto)
* `CacheableNetworkOperation`: fixed race condition in new test (#1932)
via NachoSoto (@NachoSoto)
* `BasePurchasesTests`: changed default back to SK1 (#1935) via
NachoSoto (@NachoSoto)
* `Logger`: refactored default `LogLevel` definition (#1934) via
NachoSoto (@NachoSoto)
* `AppleReceipt`: refactored declarations into nested types (#1933) via
NachoSoto (@NachoSoto)
* `Integration Tests`: relaunch tests when retrying failures (#1925) via
NachoSoto (@NachoSoto)
* `CircleCI`: downgraded release jobs to Xcode 13.x (#1927) via
NachoSoto (@NachoSoto)
* `ErrorUtils`: added test to verify that `PublicError`s can be
`catch`'d as `ErrorCode` (#1924) via NachoSoto (@NachoSoto)
* `StoreKitIntegrationTests`: print `AppleReceipt` data whenever
`verifyEntitlementWentThrough` fails (#1929) via NachoSoto (@NachoSoto)
* `OperationQueue`: log debug message when requests are found in cache
and skipped (#1926) via NachoSoto (@NachoSoto)
* `GetCustomerInfoAPI`: avoid making a request if there's any
`PostReceiptDataOperation` in progress (#1911) via NachoSoto
(@NachoSoto)
* `PurchaseTester`: allow HTTP requests and enable setting `ProxyURL`
(#1917) via NachoSoto (@NachoSoto)
NachoSoto added a commit to RevenueCat/purchases-hybrid-common that referenced this pull request Sep 27, 2022
Squashed commit of the following:

commit 24c6093
Author: NachoSoto <ignaciosoto90@gmail.com>
Date:   Wed Sep 21 10:22:07 2022 -0700

    Fixed indentation

commit b2baf12
Author: NachoSoto <ignaciosoto90@gmail.com>
Date:   Tue Sep 20 14:52:13 2022 -0700

    Removed `@testable`

commit e3fc55a
Author: NachoSoto <ignaciosoto90@gmail.com>
Date:   Tue Sep 20 14:10:46 2022 -0700

    Changed `MockPurchases` to use `PurchasesType`

    See RevenueCat/purchases-ios#1912
    This fixes the failures in #229
NachoSoto added a commit that referenced this pull request Oct 25, 2022
Fixes [CSDK-508].

#1912 moved all the docs to the `protocol`. The generated docs locally
were correct, so I didn't notice that [the online
docs](https://revenuecat.github.io/purchases-ios-docs/4.13.2/documentation/revenuecat/purchases/offerings())
were wrong.

I checked and Xcode was generating them with an extra parameter:
<img width="1000" alt="Screenshot 2022-10-25 at 12 04 38"
src="https://user-images.githubusercontent.com/685609/197862253-2afc66a3-bfcf-40af-a0c2-32e2d793c759.png">

This is also
[documented](https://www.swift.org/documentation/docc/documenting-a-swift-framework-or-package),
and it works like magic.

`--enable-inherited-docs` doesn't seem to be needed to fix this, but it
doesn't hurt to be explicit.

[CSDK-508]:
https://revenuecats.atlassian.net/browse/CSDK-508?atlOrigin=eyJpIjoiNWRkNTljNzYxNjVmNDY3MDlhMDU5Y2ZhYzA5YTRkZjUiLCJwIjoiZ2l0aHViLWNvbS1KU1cifQ
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr:feat A new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants