-
Notifications
You must be signed in to change notification settings - Fork 316
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
Configuration
: log warning if attempting to use observer mode with StoreKit 2
#3066
Conversation
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #3066 +/- ##
==========================================
- Coverage 85.93% 85.89% -0.05%
==========================================
Files 233 233
Lines 16654 16662 +8
==========================================
Hits 14312 14312
- Misses 2342 2350 +8
☔ View full report in Codecov by Sentry. |
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.
I'm wondering about whether we need to undeprecate the storekit 2 setting... But I understand linking observer mode to the storeKit2 setting, so I think this is probably better
self.observerMode = observerMode | ||
self.storeKit2Setting = .init(useStoreKit2IfAvailable: storeKit2) |
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.
I'm wondering whether we really need to keep useStoreKit2IfAvailable
deprecated... Considering we still need to use it here. Also, devs can potentially do .with(observerMode: false, storeKit2: true)
, to bypass the deprecation and achieve the same result, so it's pretty much as if it wasn't deprecated...
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.
I'm wondering whether we really need to keep useStoreKit2IfAvailable deprecated...
@aboedo and I talked about this yesterday. We think it makes sense to keep it deprecated until we finish supporting SK2 in the backend, at which point we'll at least be following Apple's guidance.
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.
Also, devs can potentially do .with(observerMode: false, storeKit2: true)
Yeah maybe the method should have never been .with(observerMode: Bool)
but instead .enableObserverMode()
.
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.
Feels a little odd to have to select "storekit 2 false" now, like, I'm not sure that tells you all that much about what's happening.
How about
enum StoreKitVersion {
case StoreKit1, StoreKit2
}
.with(observerMode: Bool, storeKitVersion: StoreKitVersion) -> Builder {
(well, with changes for objc compatibility for the enum and stuff)
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.
Isn't that the same for usesStoreKit2IfAvailable
? We made that a Bool
too.
While we're at it, we could add an overload of that one to use this new enum. What do you think?
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.
I wouldn't add a new API that's already deprecated when we add it.
I was only suggesting that
.with(observerMode: false, storeKitVersion: storeKit1)
might be slightly more expressive than
.with(observerMode: false, storeKit2: false)
But I don't feel strongly about it
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.
That way the docstring can read something like
* Set `observerMode` with a corresponding `StoreKit` implementation
* - Parameter storeKitVersion: Set this to the StoreKit implementation
* your app uses for purchases.
* I.e.: if your app uses StoreKit 1, set it to `.storeKit1`,
* and if your app uses StoreKit 2, set it to `.storeKit2`.
* Apps using StoreKit 1 use `SKPaymentQueue` for transactions,
* whereas StoreKit 2 uses `StoreKit.Product.Purchase`.
Added some more text in case folks don't know which is which. Apple removed many of the SK1 docs, so I think it's likely that folks will start getting confused about this over time as SK1 slowly fades away
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.
I'd avoid adding a new enum + docs + Obj-C equivalent etc just for one API.
But I do think it makes sense to change the deprecated method (which arguably won't be once we fully support SK2) to use it as well.
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.
@aboedo changed!
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.
I'd avoid adding a new enum + docs + Obj-C equivalent etc just for one API.
But I do think it makes sense to change the deprecated method (which arguably won't be once we fully support SK2) to use it as well.
I missed this comment but that makes a lot of sense
@@ -129,9 +129,23 @@ import Foundation | |||
* Set `observerMode`. | |||
* - Parameter observerMode: Set this to `true` if you have your own IAP implementation and want to use only | |||
* RevenueCat's backend. Default is `false`. | |||
* | |||
* - Note: This assumes your IAP implementation uses StoreKit 1. | |||
* If you use StoreKit 2, use ``with(observerMode:storeKit2:)`` instead. |
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.
I wonder if we should deprecate this method and ask people to use the other one to make it more explicit...
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.
I think that would be a good way to notify developers that are currently using this function
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.
Yeah I'll do that.
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.
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.
If a developer is currently using this function, it's almost certain that they're using SK1, though, right? Like, it seems fairly unlikely that you:
- have an existing SK1 implementation that you don't want to touch, but you want RC, so you add observer mode
- decide that you do want to move into SK2, but still want to keep RC in observer mode
Right?
So I'd vote to just keep it without deprecation
@@ -129,9 +129,23 @@ import Foundation | |||
* Set `observerMode`. | |||
* - Parameter observerMode: Set this to `true` if you have your own IAP implementation and want to use only | |||
* RevenueCat's backend. Default is `false`. | |||
* | |||
* - Note: This assumes your IAP implementation uses StoreKit 1. | |||
* If you use StoreKit 2, use ``with(observerMode:storeKit2:)`` instead. |
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.
I think that would be a good way to notify developers that are currently using this function
90cabe0
to
8f5f709
Compare
Configuration
: added observerMode
overload that allows configuring SK2 settingConfiguration
: changed observerMode
method to select SK2 setting
@available(*, deprecated) | ||
func checkDeprecatedConfiguration(_ builder: Configuration.Builder) { | ||
_ = builder | ||
.with(usesStoreKit2IfAvailable: false) |
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 was missing from the API tester.
self.observerMode = observerMode | ||
self.storeKit2Setting = .init(useStoreKit2IfAvailable: storeKit2) |
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.
That way the docstring can read something like
* Set `observerMode` with a corresponding `StoreKit` implementation
* - Parameter storeKitVersion: Set this to the StoreKit implementation
* your app uses for purchases.
* I.e.: if your app uses StoreKit 1, set it to `.storeKit1`,
* and if your app uses StoreKit 2, set it to `.storeKit2`.
* Apps using StoreKit 1 use `SKPaymentQueue` for transactions,
* whereas StoreKit 2 uses `StoreKit.Product.Purchase`.
Added some more text in case folks don't know which is which. Apple removed many of the SK1 docs, so I think it's likely that folks will start getting confused about this over time as SK1 slowly fades away
2b43259
to
af0ea6c
Compare
af0ea6c
to
d5d78b3
Compare
Planning to ship this with the paywalls release since they both require a minor. |
Updated this per our conversation:
|
Configuration
: changed observerMode
method to select SK2 settingConfiguration
: new .with(observerMode:storeKitVersion:)
@@ -400,7 +400,7 @@ extension CustomerInfo { | |||
|
|||
public extension Configuration.Builder { | |||
|
|||
/// Set `usesStoreKit2IfAvailable`. If `true`, the SDK will use StoreKit 2 APIs internally. If disabled, it will use StoreKit 1 APIs instead. | |||
/// Set `storeKit2Setting`. If `true`, the SDK will use StoreKit 2 APIs internally. If disabled, it will use StoreKit 1 APIs instead. |
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 the name of the property in Configuration.Builder
.
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.
should we use double backticks to create a code reference then, so mismatches get caught at the docs generation step?
Changing this since we can't support observer mode + SK2. |
47e015f
to
75063bd
Compare
Configuration
: new .with(observerMode:storeKitVersion:)
Configuration
: log warning if attempting to use observer mode with StoreKit 2
75063bd
to
538f5db
Compare
4fd073c
to
17fce11
Compare
17fce11
to
6a82362
Compare
…StoreKit 2 See also RevenueCat/revenuecat-docs#299. Since #3032 developers using observer mode need to configure the SDK with the correct StoreKit 2 setting. This new API makes that explicit, while leaving `with(usesStoreKit2IfAvailable:)` still deprecated.
6a82362
to
5e7f906
Compare
Hey, is there any insights on availability of SK 2 support for the observer mode? Also is there any way to just manually call something like ‘trackPurchase(transaction)’? Thanks |
@slavasemeniuk |
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.
do we plan on doing the syncObserverModePurchase thing? Is this an intermediate step?
@@ -400,7 +400,7 @@ extension CustomerInfo { | |||
|
|||
public extension Configuration.Builder { | |||
|
|||
/// Set `usesStoreKit2IfAvailable`. If `true`, the SDK will use StoreKit 2 APIs internally. If disabled, it will use StoreKit 1 APIs instead. | |||
/// Set `storeKit2Setting`. If `true`, the SDK will use StoreKit 2 APIs internally. If disabled, it will use StoreKit 1 APIs instead. |
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.
should we use double backticks to create a code reference then, so mismatches get caught at the docs generation step?
Yup that's the plan (@MarkVillacampa do you think you can take on that? I think you're pretty familiar with the setup now, and I'm happy to support you in the process :)) This is an intermediate step. Once the API is available we can remove the warning and update the documentation |
See also RevenueCat/revenuecat-docs#299.
Since #3032 developers using observer mode need to configure the SDK with the correct StoreKit 2 setting.