-
Notifications
You must be signed in to change notification settings - Fork 408
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
[iOS] Fix issue when paymentToken
is not present on real iOS device.
#37
Conversation
Other changes: - this commit unifies responses for iOS simulator and real iOS device - `serializedPaymentData` is no longer present. Data is always deserialised. - add some Flow types and fix some (e.g `object` -> `Object`)
paymentToken
is not present on real iOS device.paymentToken
is not present on real iOS device.
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.
@Andreyco Nice work! I left a few comments. Once those changes are in, we'll be set for a merge.
@@ -54,6 +54,18 @@ import { | |||
SUPPORTED_METHOD_NAME | |||
} from './constants'; | |||
|
|||
type PaymentDetailsIOS = { |
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 call
Let's move these to the already existing types.js
file.
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
const { | ||
paymentData: serializedPaymentData, |
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.
Let's not refer to serializedPaymentData
as paymentData
even if we'll have a type for it -- It's likely to stump developers not using flow
.
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.
Not sure I understand your advice here... Could you elaborate?
|
||
return { | ||
paymentData: isSimulator ? null : JSON.parse(serializedPaymentData), |
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 could see this stumping some people. Could you update the README to warn users of the behaviour in TEST.
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.
Sure, documentation is improved.
Sorry for being unresponsive in past days. Will take a look at those during this weekend. Appreciate your time here. |
…n device vs simulator
…uction and real device.
any word on this pr? |
naoufal#37) * Fix issue when `paymentToken` is not present on real iOS device. Other changes: - this commit unifies responses for iOS simulator and real iOS device - `serializedPaymentData` is no longer present. Data is always deserialised. - add some Flow types and fix some (e.g `object` -> `Object`) * Move type definition into dedicated file. * [Docs] Mention `paymentData` inconsistencies when running Apple Pay on device vs simulator * [Docs] Add `Testing Payments` section and advise user to test in production and real device.
naoufal#37) * Fix issue when `paymentToken` is not present on real iOS device. Other changes: - this commit unifies responses for iOS simulator and real iOS device - `serializedPaymentData` is no longer present. Data is always deserialised. - add some Flow types and fix some (e.g `object` -> `Object`) * Move type definition into dedicated file. * [Docs] Mention `paymentData` inconsistencies when running Apple Pay on device vs simulator * [Docs] Add `Testing Payments` section and advise user to test in production and real device.
Other changes:
serializedPaymentData
is no longer present. Data is always deserialised.object
->Object
)Fixes #30