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

update readme #486

Closed
wants to merge 1 commit into from
Closed

update readme #486

wants to merge 1 commit into from

Conversation

Edgpaez
Copy link
Contributor

@Edgpaez Edgpaez commented May 13, 2019

related to #423

addAdditionalSuccessPurchaseListenerIOS needs to be called prior to buyProduct if one expects it to catch successful purchases.

This PR reflects this on the example usage of buyProduct

addAdditionalSuccessPurchaseListenerIOS needs to be called prior to buyProduct if one expects it to catch successful purchases
@Edgpaez Edgpaez mentioned this pull request May 13, 2019
@vafada
Copy link

vafada commented May 13, 2019

I haven't tested this but it looks like for a normal flow (not the StokeKit flow), this.handlePurchaseSuccess(purchase); will be called twice

@hyochan hyochan added the 📖 documentation Improvements or additions to documentation label May 13, 2019
@hyochan
Copy link
Member

hyochan commented May 13, 2019

@Edgpaez I partially agree with @vafada. You should only subscribe to the listener when there was a failure in the purchase unless you should put any other condition inside the listener that would avoid recording double purchases.

In addition, the react-native-iap won't affect on duplicate purchase just by adding listener. The side effect on doing this is receiving the same result twice. This may cause duplication when recording successful result in your database (if you are doing so).

Copy link
Member

@hyochan hyochan left a comment

Choose a reason for hiding this comment

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

Please review my comment above and improve your code separately. If you want the better approach, please preserve the previous code then post other approaches below.

@Edgpaez
Copy link
Contributor Author

Edgpaez commented May 14, 2019

@vafada, yes. This is not related to storekit flow. TBH, I don't understand how to address your comment.

@hyochan I don't think you can subscribe AFTER you get the error. Not if you want to be sure you'll catch the error.
Can we be sure the second purchaseProcess is called after the user caught the error and called addAdditionalSuccessPurchaseListenerIOS ?

I think we need to be clear to users that this.handlePurchaseSuccess(purchase); can be called more than once.
What do you think?

@hyochan
Copy link
Member

hyochan commented May 16, 2019

@Edgpaez I am not sure what you are trying to achieve here. I think I've explained everything previously.

@Edgpaez
Copy link
Contributor Author

Edgpaez commented May 18, 2019

What I'm trying to do is fix the documentation since I believe the example code has a bug.

The bug (I believe) is that addAdditionalSuccessPurchaseListenerIOS won't catch all success purchases in the storekit flow.

Current Implementation
Internally addAdditionalSuccessPurchaseListenerIOS relies on NativeEventEmitter to listen for the iap-purchase-event event.

Inside RNIapIos (the class handling the purchases natively) when the iap-purchase-event event is emitted there is no guarantee that addAdditionalSuccessPurchaseListenerIOS has already registered to listen that event.

So, what's the problem?
I think that means that there is no way to be sure that when the iap-purchase-event is emitted natively, there is a listener registered on JS.
That means there is a possibility that users successfully pay but JS doesn't acknowledge the payment.

The fix?
Registering the listener for the iap-purchase-event event before making a purchase. (which is the intention of this PR)

I hope that's clearer @hyochan

@hyochan
Copy link
Member

hyochan commented May 20, 2019

@Edgpaez Thanks for your through explanation and I could understand you much better! I've not yet reproduced any missing event in my other app. Also, the listener is registered when there was failure in the purchase as in line of code. I am not quite sure tough whether there will be any hole doing the implementation this way since this was just the work around described in #348. If there is any clear reproducible senario, I would like to try to consider this once again.

@vafada
Copy link

vafada commented May 20, 2019

@hyochan

@Edgpaez has a point. There is NO GUARANTEE that the listener would be added before the second success transaction comes to the queue.

So this flow is possible:

  1. Failed transaction enters the Payment Queue
  2. Success transaction enters the Payment Queue
  3. code goes to catch block
  4. listener gets added

At this point, the success transaction is now lost.

@hyochan
Copy link
Member

hyochan commented May 20, 2019

@vafada The code goes to catch block right after failed transaction and starts the listener. Then the success transaction enters the payment queue and listener will receive it.

If we use the updated code, the listener will receive the success queue at the same time with the result of buyProduct. We are getting two results for ios for every success results which we need more codes to prevent receiving the same results and doing things twice.

@vafada
Copy link

vafada commented May 20, 2019

@hyochan

We don't know how fast or slow StoreKit will put the success transaction in the queue.

It's all async so it is not safe to assume that the success transaction will always come in after adding the listener.

I agree also with the listener getting two event for the same successful transaction.

This is not an easy problem 😄

I'm now using this fork https://github.com/superandrew213/react-native-in-app-utils/tree/listen-for-purchase-event#helpers

And here's my pseudo code to deal with StoreKit flow:

chirag04/react-native-in-app-utils#202 (comment)

@Edgpaez
Copy link
Contributor Author

Edgpaez commented May 20, 2019

@hyochan IMHO it's better if users handle purchases more than once instead of having no guarantee that all successful purchases are handled. Since transactions have IDs users have data available to avoid duplicates on their end.

I know it's not ideal but IMHO it's the best suggestion we can give users until we can come up with a better API that hides all these details.

@vafada I'll take a look at your fork 👍

@hyochan
Copy link
Member

hyochan commented May 20, 2019

@Edgpaez If you'd like to support a better approach to the readme please do not erase the previous one and add to another paragraph for your approach and add the description on why it'd be better.
Since current approaches are not either perfect it'd be better to have more references.

Thanks for all your support in advance.

@hyochan
Copy link
Member

hyochan commented Jun 13, 2019

@vafada Since version 3 is out today. This doc is no longer needed. I may go to deprecated readme.

@hyochan hyochan closed this Jun 13, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
📖 documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants