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
➕ Add the success/failure handling from Dart side for Apple Pay #200
base: main
Are you sure you want to change the base?
➕ Add the success/failure handling from Dart side for Apple Pay #200
Changes from 6 commits
a3b1b81
78dfe7b
fcab163
0d8c189
23b63f8
cddc099
1323120
4a48cc8
a06e1ec
cd07e7c
87268b8
8e8e194
8d236bb
122d7ba
b09fd8c
73fa61c
747ff50
b39bddd
15fb1aa
7b64e53
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.
As you probably noted, this plugin offers two integration paths.
ApplePayButton
): This approach is great for folks who are happy with a default-based configuration and prefer to think of their apps as a collection of components.Pay
client directly to issue calls, and showing the raw button components in the UI when necessary.To be consistent with that, we'd want to update the
StatefulWidget
underpay/lib/src/widgets/apple_pay_button.dart
to handle the new logic too, and use it in the example.One way to do that would be to offer an additional result callback in the Apple Pay button widget in the
pay
library that includes a handler (eg.:paymentConfirmation
) that lets folks update the payment result to the iOS native end.Let me know what you think
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.
Oh I didn't notice the the two integration paths before 😅.
Regarding the addition of a new result callback, since we renamed the method
updatePaymentStatus
toupdatePaymentResult
, I think it will be confusing to users who seeonPaymentResult
and might link it toupdatePaymentResult
.We can do a breaking a change to the API and make the
onPaymentResult
signature:And then the user can do their backend call here and return the result of the payment from this callback, which is required.
We can also make the return type of the callback nullable so existing users of the package who are not interested don't need to touch their code, but then there becomes two definitions of success,
null
ortrue
.null
would be the default state if the user didn't return anything.What do you think? Should we introduce a new callback, or use the existing one?
I think using onPaymentResult makes more sense here since the names are related and we can do something like this in the
PayButton
:and then we can keep the Google button signature the same, but have it return null for example when providing it to the super.
and for Apple button, we can expose the new signature for users.
What do you think?
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.
Thank you for the proposal @YazeedAlKhalaf.
Your suggestion certainly supposed the least resistance path into getting the functionality out. I see three main disadvantages:
Using an additional callback for Apple Pay doesn't have any of the disadvantages above, although this approach also has some drawbacks:
late final
, since it now needs to be overriden in the Apple button child class to call both callbacks if set. This can't be done in the initializer block because the second callback needs thePay
client to talk to the iOS native end, which is a class member. A way to circumvent that would be to create a secondPay
client for that call only. To me, thelate final
demotion seems more appropriate.I've put together a prototypical proposal that adds an extra callback for Apple Pay and lets developers choose the one that is best suited for them (the change may seem a little larger than necessary, since it includes a couple of type definitions and extra modifiers that can be now added). This is how the change would look for those who decide to use the confirm flow:
I have a mild preference for the latter approach, and would like to hear your thoughts on it.
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.
Looks great tbh, easier for the developers 👍.
I'll try to bundle it in this PR and hopefully we can see what to do next!
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.
Thank you so much @JlUgia for your suggestion and PR!
I have implemented your changes with a small change, can you please look at it?
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 will fail silently if executed from Android. Since this can only happen at development time I'd let the
Error
surface back for the developer to fix their code.Alternatively, and if you think the error is not informative enough, we can craft our own exception/error.
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.
okay, i think making a custom exception would be better in this case. can you check it?