-
Notifications
You must be signed in to change notification settings - Fork 13
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] add should purchase promo product #25
Conversation
…hases-framework.sh after the carthage archive updates.
…duct, although they don't do anything yet.
…ature calling of the should purchase method
@@ -28,7 +28,7 @@ curl -sSL $URL > temp.zip | |||
# In some cases the temp folder can not be created by unzip, https://github.com/RevenueCat/react-native-purchases/issues/26 | |||
mkdir -p temp | |||
unzip -o temp.zip -d temp | |||
mv temp/Purchases.framework ./Purchases.framework | |||
mv temp/Carthage/Build/iOS/Purchases.framework ./Purchases.framework |
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.
new path in 3.0.2
@@ -22,6 +22,7 @@ | |||
- (void)createAlias:(CDVInvokedUrlCommand *)command; | |||
- (void)identify:(CDVInvokedUrlCommand *)command; | |||
- (void)reset:(CDVInvokedUrlCommand *)command; | |||
- (void)setupShouldPurchasePromoProductCallback:(CDVInvokedUrlCommand *)command; |
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 so that we get a different callback id, which we can store and use later when notifying javascript
return false; | ||
} | ||
|
||
private static setupShouldPurchasePromoProductCallback() { |
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 weren't using private functions here, but I figured it wouldn't hurt
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 love it
src/plugin/plugin.ts
Outdated
*/ | ||
public static addShouldPurchasePromoProductListener(shouldPurchasePromoProductListener: ShouldPurchasePromoProductListener): void { | ||
if (typeof shouldPurchasePromoProductListener !== "function") { | ||
throw new Error("addPurchaserInfoUpdateListener needs a 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.
Oops, fixing
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.
Cool. I commented on my review on this before seeing 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.
@vegaro https://github.com/RevenueCat/react-native-purchases/pull/98/files#diff-f41e9d04a45c83f3b6f6e630f10117feR546 LOL 😂 copy/paste kings over here. I'll make a PR for that one 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.
Why did src/ios/Purchases.framework/Purchases
get removed??
} | ||
|
||
private static setupShouldPurchasePromoProductCallback() { | ||
window.cordova.exec( |
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.
Doesn't Android crash because setupShouldPurchasePromoProductCallback
doesn't exist in the Android code?
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.
good catch. I'll fix it and update. I forgot to test on android (hides in shame)
return false; | ||
} | ||
|
||
private static setupShouldPurchasePromoProductCallback() { |
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 love it
src/plugin/plugin.ts
Outdated
private static setupShouldPurchasePromoProductCallback() { | ||
window.cordova.exec( | ||
(callbackResult: any) => { | ||
const callbackID = callbackResult.callbackID; |
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 two lines could be removed with:
({callbackID}:{callbackID: number}) => {
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.
cool! didn't know that! I'll update
I'm not sure what's going on with
I tried clean-cloning and checking out the branch and I do get the file, so I'm not sure what's going on. I'll do some further digging, but right now it seems that Github is the only one that sees the file as removed. |
you can even see the file on the branch: https://github.com/RevenueCat/cordova-plugin-purchases/blob/feature/addShouldPurchasePromoProduct/src/ios/Purchases.framework/Purchases So I really think this is a github bug. If I try removing the file on my local repo and check git diff, it does show that I'm removing it. |
@vegaro would you mind doing a sanity check by checking out the branch? |
@@ -156,6 +156,11 @@ private void setAutomaticAppleSearchAdsAttributionCollection(boolean enabled, Ca | |||
// NOOP | |||
} | |||
|
|||
@PluginAction(thread = ExecutionThread.WORKER, actionName = "setupShouldPurchasePromoProductCallback") | |||
private void setupShouldPurchasePromoProductCallback(CallbackContext callbackContext) { | |||
// NOOP |
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.
wild violation of Interface Segregation Principle, but I think it's okay? I copied it from line 156.
other ideas would be to have the javascript side decide (which isn't ideal), or to somehow split interfaces, which might be too much work for too little gain.
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 better too
@vegaro ready for another pass |
Description
https://trello.com/c/emjo2Nlf
Related PR: RevenueCat/react-native-purchases#98
This is still missing cleanup work, but it's functional.Changes summary:
Purchases.framework
in scriptmakeDeferredPurchase
, to be called by client apps when they're ready to make a promotional purchase.shouldPurchasePromoProduct
, called by the SDK when it detects that a promotional purchase should be made.addShouldPurchasePromoProductListener
andremoveShouldPurchasePromoProductListener
, to be called by client apps so they can set up listeners. They will get a function that they call to trigger the purchase on our side, which they can call whenever the app is ready to make the purchase.Code flow:
makeDeferredPurchase();
. listeners won't be executed until promo purchases are detected.shouldPurchasePromoProduct
. we store a callback id to identify the exact call.makeDeferredPurchase()
makeDeferredPurchase
gets the callback id to identify the exact call, and callsmakeDeferredPurchase
on the sdk.