-
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
[Final] universal purchases #183
Conversation
@@ -22,7 +22,7 @@ func resolveTargets() -> [Target] { | |||
.target(name: "Purchases", | |||
dependencies: [], | |||
path: ".", | |||
sources: ["Purchases", "Purchases/Public", "Purchases/Caching"], | |||
sources: ["Purchases", "Purchases/Public"], |
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 this is a wrong conflict resolution
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 honestly not sure what to do here.
in theory, sources is supposed to recursively add subfolders, so you should only really need ["Purchases"]
.
However, if I remove "Public", I get a compilation error, undefined symbol.
For the Caching
folder, though, it works fine if you don't add it here.
Ideally I'd like to figure out why the reference to Public is needed, and if possible remove it.
Purchases.podspec
Outdated
@@ -17,6 +17,8 @@ Pod::Spec.new do |s| | |||
|
|||
s.ios.deployment_target = '9.0' | |||
s.osx.deployment_target = '10.12' | |||
s.watchos.deployment_target = '6.2' | |||
s.tvos.deployment_target = '13.4' |
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.
don't we support older versions?
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 was honestly not sure about this. Since there wasn't a test app and tvOS wasn't a part of the target device families, I didn't even know whether it compiled on tvOS before this.
I meant to test further on tvOS but I got distracted with other stuff.
I just tried a new tvOS app on 3.0.4 and it does compile and seems to work. I don't have a physical apple tv so I can't get much further than that, unfortunately.
I'll change the target to tvOS 9 since it's the first one that's compatible with StoreKit. Maybe we could sync up later about how best to test this?
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.
chatted about this on slack. Since we currently don't specify the target, nothing stops you right now from using purchases-ios with tvOS 9+. It won't compile with anything before that because StoreKit wasn't included in tvOS until v9.
we're going to go with deployment_target
= 9 to reflect this, so that at least we have an explicit value to reference.
Purchases/Public/RCPurchases.h
Outdated
@@ -399,6 +399,14 @@ NS_SWIFT_NAME(entitlements(_:)) RC_UNAVAILABLE("entitlements: has been replaced | |||
*/ | |||
- (void)invalidatePurchaserInfoCache; | |||
|
|||
#if TARGET_OS_WATCH | |||
/** | |||
Used to notify Purchses that the app has become active. WatchOS doesn't implement the ApplicationDidBecomeActive |
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.
typo "Purchses"
Purchases/RCAttributionFetcher.m
Outdated
@@ -46,7 +46,7 @@ - (nullable NSString *)advertisingIdentifier | |||
|
|||
- (nullable NSString *)identifierForVendor | |||
{ | |||
#if TARGET_OS_IPHONE | |||
#if TARGET_OS_IOS |
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.
what about tv os? because TARGET_OS_IPHONE also includes Apple TVs right?
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.
world's most confusing macros (the tv is iPhone but it isn't iOS?). Good catch, I'll update
@@ -150,7 +150,7 @@ static RCPurchasesErrorCode RCPurchasesErrorCodeFromRCBackendErrorCode(RCBackend | |||
return RCUnknownError; | |||
} | |||
|
|||
#if TARGET_OS_IPHONE | |||
#if TARGET_OS_IOS |
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.
what about tv os?
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.
updated. This class actually merits more thought (maybe on a separate PR) because some of these error codes are now mac compatible
Purchases/RCStoreKitWrapper.m
Outdated
@@ -82,7 +82,7 @@ - (void)paymentQueue:(SKPaymentQueue *)queue removedTransactions:(NSArray<SKPaym | |||
} | |||
} | |||
|
|||
#if TARGET_OS_IPHONE && !TARGET_OS_MACCATALYST | |||
#if TARGET_OS_IOS && !TARGET_OS_MACCATALYST |
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.
same thing about Apple TV here
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.
👍
backend support has been deployed, removing the [Hold] |
01c0b85
to
c81bc54
Compare
c81bc54
to
0b26feb
Compare
@fnazarios changes were integrated here, 4648bf7. |
5424ae5
to
da9298f
Compare
@vegaro bump |
|
||
#if TARGET_OS_IPHONE | ||
#import <UIKit/UIKit.h> | ||
#elif TARGET_OS_OSX |
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 used to be TARGET_OS_MAC
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, but the thing is that TARGET_OS_MAC
= 1 on iOS. well, it's true for pretty much everything apple. this was working before because we were checking for iPhone first and this was the else variant, which would be true for everything.
TARGET_OS_WIN32 - Generated code will run under 32-bit Windows
TARGET_OS_UNIX - Generated code will run under some Unix (not OSX)
TARGET_OS_MAC - Generated code will run under Mac OS X variant
TARGET_OS_OSX - Generated code will run under OS X devices
TARGET_OS_IPHONE - Generated code for firmware, devices, or simulator
TARGET_OS_IOS - Generated code will run under iOS
TARGET_OS_TV - Generated code will run under Apple TV OS
TARGET_OS_WATCH - Generated code will run under Apple Watch OS
TARGET_OS_BRIDGE - Generated code will run under Bridge devices
TARGET_OS_MACCATALYST - Generated code will run under macOS
TARGET_OS_SIMULATOR - Generated code will run under a simulator
Purchases/RCAttributionFetcher.m
Outdated
@@ -56,7 +56,7 @@ - (nullable NSString *)identifierForVendor | |||
|
|||
- (void)adClientAttributionDetailsWithCompletionBlock:(void (^)(NSDictionary<NSString *, NSObject *> * _Nullable attributionDetails, NSError * _Nullable error))completionHandler | |||
{ | |||
#if TARGET_OS_IPHONE | |||
#if TARGET_OS_IOS |
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.
TARGET_OS_TV
won't enter here and it was before, is that expected
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.
ADClient
isn't available on apple tv https://developer.apple.com/documentation/iad/adclient?language=objc
(actually, the entire iAd isn't available on TV for whatever reason)
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.
thinking about this more, maybe the ideal way to do this is to set up a couple of macros that define:
PLATFORM_INCLUDES_IAD
and stuff like that so that the intention is very clear. thoughts?
Package.swift
Outdated
@@ -48,7 +49,7 @@ func resolveTargets() -> [Target] { | |||
let package = Package( | |||
name: "Purchases", | |||
platforms: [ | |||
.macOS(.v10_12), .iOS(.v9) | |||
.macOS(.v10_12), .iOS(.v9), .watchOS("6.2"), .tvOS("13.4") |
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.
We have some tvOS apps already, will this change affect them?
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 updating this one per the conversation we had a while back, I'd forgot to perform the update here.
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 not sure they were able to use Purchases through SPM though - note that the TV platform wasn't defined for it before this PR
@@ -17,6 +17,8 @@ Pod::Spec.new do |s| | |||
|
|||
s.ios.deployment_target = '9.0' | |||
s.osx.deployment_target = '10.12' | |||
s.watchos.deployment_target = '6.2' | |||
s.tvos.deployment_target = '9.0' |
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 one differs with the one in the Package.swift
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 correct one, I'll update Package.swift
…n rule error in xcode 11.4 beta
…s compilation errors anyway
… "iOS" from watchOS until the backend supports the new platform.
…app became active.
…of xcode 11.4 beta 3 warning
da9298f
to
4596416
Compare
@@ -17,6 +17,8 @@ Pod::Spec.new do |s| | |||
|
|||
s.ios.deployment_target = '9.0' | |||
s.osx.deployment_target = '10.12' | |||
s.watchos.deployment_target = '6.2' | |||
s.tvos.deployment_target = '9.0' |
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 correct one, I'll update Package.swift
Purchases/RCAttributionFetcher.m
Outdated
@@ -56,7 +56,7 @@ - (nullable NSString *)identifierForVendor | |||
|
|||
- (void)adClientAttributionDetailsWithCompletionBlock:(void (^)(NSDictionary<NSString *, NSObject *> * _Nullable attributionDetails, NSError * _Nullable error))completionHandler | |||
{ | |||
#if TARGET_OS_IPHONE | |||
#if TARGET_OS_IOS |
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.
ADClient
isn't available on apple tv https://developer.apple.com/documentation/iad/adclient?language=objc
(actually, the entire iAd isn't available on TV for whatever reason)
|
||
#if TARGET_OS_IPHONE | ||
#import <UIKit/UIKit.h> | ||
#elif TARGET_OS_OSX |
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, but the thing is that TARGET_OS_MAC
= 1 on iOS. well, it's true for pretty much everything apple. this was working before because we were checking for iPhone first and this was the else variant, which would be true for everything.
TARGET_OS_WIN32 - Generated code will run under 32-bit Windows
TARGET_OS_UNIX - Generated code will run under some Unix (not OSX)
TARGET_OS_MAC - Generated code will run under Mac OS X variant
TARGET_OS_OSX - Generated code will run under OS X devices
TARGET_OS_IPHONE - Generated code for firmware, devices, or simulator
TARGET_OS_IOS - Generated code will run under iOS
TARGET_OS_TV - Generated code will run under Apple TV OS
TARGET_OS_WATCH - Generated code will run under Apple Watch OS
TARGET_OS_BRIDGE - Generated code will run under Bridge devices
TARGET_OS_MACCATALYST - Generated code will run under macOS
TARGET_OS_SIMULATOR - Generated code will run under a simulator
#define PLATFORM_HEADER @"iOS" | ||
#elif TARGET_OS_MAC | ||
#elif TARGET_OS_OSX |
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.
here's where using MAC
would be problematic - it'd always be true and we'd never go into the other cases
Package.swift
Outdated
@@ -48,7 +49,7 @@ func resolveTargets() -> [Target] { | |||
let package = Package( | |||
name: "Purchases", | |||
platforms: [ | |||
.macOS(.v10_12), .iOS(.v9) | |||
.macOS(.v10_12), .iOS(.v9), .watchOS("6.2"), .tvOS("13.4") |
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 updating this one per the conversation we had a while back, I'd forgot to perform the update here.
Package.swift
Outdated
@@ -48,7 +49,7 @@ func resolveTargets() -> [Target] { | |||
let package = Package( | |||
name: "Purchases", | |||
platforms: [ | |||
.macOS(.v10_12), .iOS(.v9) | |||
.macOS(.v10_12), .iOS(.v9), .watchOS("6.2"), .tvOS("13.4") |
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 not sure they were able to use Purchases through SPM though - note that the TV platform wasn't defined for it before this PR
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.
Feel free to merge after solving that question about PURCHASES_INITIATED_FROM_APP_STORE_AVAILABLE
Adds support for tvOS and watchOS.
To do:
Platforms:
Other:
(watchOS only) Add a public method for the app to notify when it has become active, since didBecomeActive notification isn't available on watchOS.edit: this was replaced by the correct call for when the extension has become active on watchOS thanks to @fnazariosHOLD
This PR is on hold until the khepri side is deployed.
Future changes