search

stripe / stripe-react-native

React Native library for Stripe.
https://stripe.dev/stripe-react-native
MIT License
1.27k stars 265 forks source link

Response of `handleNextPaymentAction` differs between iOS and Android #11

Closed thorsten-stripe closed 3 years ago

thorsten-stripe commented 3 years ago

Describe the bug The NoWebhookPayment example is working fine on iOS but on Android after the 3D Secure keeps spinning endlessly.

This is due to handleNextPaymentAction having different return values on iOS vs Android.

const response = await handleNextPaymentAction(clientSecret);
// Response on Android
{
   "amount":1400,
   "created":1610534695,
   "currency":"usd",
   "description":null,
   "id":"pi_1I96qRG7gfqYEkPOElpsrC0F",
   "receiptEmail":null,
   "status":"RequiresConfirmation"
}
// Response on iOS
{
   "currency":"usd",
   "description":"...",
   "id":"pm_1I96wnG7gfqYEkPOFrjhO6SK",
   "receiptEmail":"",
   "status":"RequiresConfirmation",
   "stripeId":"pi_1I96woG7gfqYEkPO3r9MhHII"
}

Also, stripeId is not a good name for the PaymentIntent ID. As per https://stripe.com/docs/js/payment_intents/handle_card_action it would be better to return

{ error: Error; paymentIntent: Stripe.PaymentIntent }

To Reproduce Steps to reproduce the behavior:

  1. Run yarn example start
  2. Run yarn example android
  3. Click "Card payment without webhook"
  4. Click "Pay"
  5. Click "Complete Authentication"
  6. See endless spinning of button

Expected behavior Same response shape across platforms.

arekkubaczkowski commented 3 years ago

Hi, thanks for investigating, I looked into it and I know where these differences came from. iOS sdk has little bit different PaymentIntent interface than android for example: android: override val id: String?, iOS: /// The Stripe ID of the PaymentIntent. @objc public let stripeId: String

We will try to unify these differences but I have one point regarding response interface, our tenet about responses in whole SDK is that we returns either Success data or Error data not like you suggested, so we would have to make this change in every response to keep consistency, so the choice is up to you.

thorsten-stripe commented 3 years ago

@bg-stripe can I get you to chime in here? Seems like there are quite some differences between Stripe.js, iOS, and Android, and we need to make a decision which DX we want to model after.

thorsten-stripe commented 3 years ago

Same with naming:

While I do think that handleNextPaymentAction is very descriptive, I don't like the idea of each SDK having a different name. I think we should make a decision which SDK we want RN to be closest to @bg-stripe @auchenberg-stripe

bg-stripe commented 3 years ago

Hm, yeah the iOS & Android responses should be the same. On iOS, we use stripeId because id is a reserved symbol. For React Native, we should just use id for both platforms.

I don't think we need to change the responses drastically here. It looks like the iOS response includes a "id":"pm_1I96wnG7gfqYEkPOFrjhO6SK", – I think we should exclude this payment method ID from the iOS response if it isn't included on Android.

bg-stripe commented 3 years ago

For handle action naming – I think we should align on handleCardAction, since that would let us use the diagram in the web tab of the docs (cc @davidme-stripe )

image

mshafrir-stripe commented 3 years ago

Do you have an example of the iOS SDK having a property/field that the Android SDK doesn't?

bg-stripe commented 3 years ago

@mshafrir-stripe yeah – based on https://github.com/stripe/stripe-react-native/issues/11#issue-785004477 – looks like there's a pm_ID in the iOS response, but not on Android...

mshafrir-stripe commented 3 years ago

This might be because the Android SDK is expanding the payment_method field, so that it is in the shape of a PaymentMethod object, not a payment method id.

See https://stripe.com/docs/api/payment_intents/object#payment_intent_object-payment_method

arekkubaczkowski commented 3 years ago

Yes, looks like the id is the only difference between both platforms but I've noticed also that setupFutureUsage on android is a private field while on iOS is public so I assume that we should omit this info?

btw. what is the decision about handleNextPaymentAction method name?

thorsten-stripe commented 3 years ago

After #41 the responses are:

I'm not entirely sure about the long description string on iOS that seems to be representing the whole object, @bg-stripe is this expected? Otherwise, this LGTM 👍

Regarding naming, we decided to go with handleCardAction as this is a method that doesn't exist for other payment methods: https://stripe.com/docs/js/payment_intents/handle_card_action, and we'll be able to reuse the graphic in the docs. Would you be able to incorporate the naming change into #41 ?

thorsten-stripe commented 3 years ago

@arekkubaczkowski quick question regarding

I have one point regarding response interface, our tenet about responses in whole SDK is that we returns either Success data or Error data

You're saying that you're either resolving with success data or rejecting with error data, correct? In Stripe.js we don't reject but rather return the error, see: https://github.com/stripe/stripe-js/blob/master/types/stripe-js/index.d.ts#L265-L267

I'll discuss with the team how we want to go about this in RN.

arekkubaczkowski commented 3 years ago

You're saying that you're either resolving with success data or rejecting with error data, correct?

yes, this is exactly what I meant

I'll discuss with the team how we want to go about this in RN.

ok cool