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

Convert Delegation Pattern to Async-Await #292

Closed
wants to merge 14 commits into from

Conversation

KunJeongPark
Copy link
Collaborator

@KunJeongPark KunJeongPark commented Oct 14, 2024

Summary of changes

  • convert from returning results via delegate functions to merchants to async await functions that return results or errors directly.
  • provide completion handler version of public functions

Discussion:

  • Created a separate PR for completion handler first version that offers async await wrapper functions.

    Thinking ahead to including app switch features, the completion handler version has been tested in Braintree
    iOS SDK. I did put up a POC branch
    with async await pattern and saw it work by resuming continuation.

    Pros of using async await pattern first is setting a pattern where we don't have to refactor bottom up from completion
    handler version in the future. Code is much easier to read and unit tests are much easier to implement.

    Cons I've experienced working on this PR:

  1. ASWebAuthenticationSession returns results via completion handlers, so we are having to wrap a
    significant portion of the public functions in withCheckedThrowingContinuation.
  2. Higher risk of continuation resume errors. Continuations require strict guarantees that they will only be resumed once.
  3. Possibly more complex scenarios with states with app switch failure
  4. As I stated above, app switch pattern has been tested with completion handler in Braintree iOS SDK in production, not with async/await's continuation.

I am closing this one in favor of completion handler version

For the merchant, the integration experience will be the same with completion handler first or async await first approach.
Both versions will offer both completion handler and async await versions of the public functions.

Eliminating old delegation pattern with either approach definitely simplifies integration and future development for us.

Checklist

  • Added a changelog entry

Authors

List GitHub usernames for everyone who contributed to this pull request.

@KunJeongPark KunJeongPark marked this pull request as ready for review October 23, 2024 19:59
@KunJeongPark KunJeongPark requested a review from a team as a code owner October 23, 2024 19:59
Copy link
Collaborator

@sshropshire sshropshire left a comment

Choose a reason for hiding this comment

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

I do really like async/await where possible. It makes the code really clean from a maintenance perspective.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants