Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Fix Bug where
fetchPaymentMethodNonces
Return Incorrect Type #1100Fix Bug where
fetchPaymentMethodNonces
Return Incorrect Type #1100Changes from 10 commits
28e36d2
9319995
a9a4057
c821e2e
a086353
6cbd5ed
1d58414
0310ecb
58da865
9cee196
4b89b34
c36f238
388c36a
bf0a676
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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've actually gotten multiple requests to move this back into core. We initially moved it to DropIn to get rid of circular dependencies, and we're planning to implement it the correct way in
v5
.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.
Ah interesting... we may have to rethink what we want to return to merchants if we move it back to core. Not sure if Android is capable of what we were doing in v5. We were using the
load
method to dynamically register types that don't exist in core (BTCardNonce
/PayPalNonce
/'VenmoNonce/
ApplePayNonce`) at compile time. We don't have the ability to do that hacky workaround in Swift and importing those modules into Core would cause a circular dependency. How are y'all planning to correctly implement it in v5?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 it was a hack in Android
v3
as well. We usedcompileOnly
in our Gradle files to basically create weak references in the dependency tree. That allowed Gradle to build successfully, but it caused some issues in the long run.By "correct" I'm thinking "in a non-hacky way." It's tough because there will probably be tradeoffs. An enum feels like the right way to me, but I'd be up to do some cross-platform pairing on this since we need a solution on Android too.
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.
The below logic mirrors the behavior of iOS v5 with regards to the type. Though this is with the constraints of not being able to cast to unknown nonce types in Swift. The load method used previously was a very hacky workaround and does not map to Swift - the method (
load
) in Obj-C is prohibited in Swift. Currently the JS SDK has the methodfetchPaymentMethodNonces
in Core while Android has this method in the Drop-in. We should consider where this method should live and align across all 3 platforms.