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

Async fetch configuration #1399

Draft
wants to merge 37 commits into
base: main
Choose a base branch
from
Draft

Async fetch configuration #1399

wants to merge 37 commits into from

Conversation

KunJeongPark
Copy link
Contributor

@KunJeongPark KunJeongPark commented Aug 27, 2024

Summary of changes

  • Implement async await fetchConfiguration and replace all usage of completion handler fetchOrReturnRemoteConfiguration with fetchConfiguration.

Replacing of completion handler version of fetchOrReturnRemoteConfiguration required wrapping the functions in Task blocks within functions that return results via completion handler to the merchants.

There were many changes to unit tests required as tests with mocks returned results synchronously and asynchronous nature of the Task blocks required slight delays for the assertions.

I think there are a lot of considerations -

Consider refactoring functions wrapped with withCheckedContinuation to true asynchronous/await syntax where possible

How far down do we refactor? Within the task blocks, there are many subsequent function calls that pass down the completion handlers. I am unsure about top down approach to the refactor where we have task block for top level tokenize function for instance, as opposed to bottom up, like the apiClient’s post function or the functions that tokenize calls.

We should do careful and extensive testing to make sure that there are no unintended consequences especially in app switching context. Major difference with creating a Task block is that it does not inherit the context of its caller by default. We should test thoroughly to ensure that there are no subtle bugs introduced like deadlocks and priority inversions.

I will open another ticket to continue to test and improve on the refactor started here.

Checklist

  • Added a changelog entry

Authors

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

jaxdesmarais and others added 30 commits August 19, 2024 16:04
# Conflicts:
#	Sources/BraintreeCard/BTCardClient.swift
# Conflicts:
#	Sources/BraintreePayPal/BTPayPalClient.swift
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants