Add swift concurrency for auth flow #330
Conversation
Merge develop into main
Adds a new public protocol to back UberAuth static methods
|
|
luizgustavo09
changed the title
luizgustavo09
force-pushed
the
refactor/main/swift-concurrency
branch
from
a1e693b to
97fb0fe
Compare
| completion: @escaping (Result<Client, UberAuthError>) -> ()) | ||
|
|
||
| func execute(authDestination: AuthDestination, | ||
| prefill: Prefill?) async throws -> Client |
There was a problem hiding this comment.
I'm fine converting this to throwing, but we lose the typed error. Can we add some comments saying that it throws an UberAuthError.
There was a problem hiding this comment.
Good point I'll document it on the code
Sources/UberAuth/AuthProviding.swift
Outdated
| /// @mockable | ||
| public protocol AuthProviding { | ||
|
|
||
| @available(*, deprecated, message: "This method is deprecated. Use the async method instead.") |
There was a problem hiding this comment.
What's the deprecation plan? Is there a version in the future that we will remove these in? Any plan to keep the async version and closure version?
There was a problem hiding this comment.
If the plan is not to remove them, maybe we should explore a different set of protocols or maybe a separate asynchronous package so people can check out the one they need.
There was a problem hiding this comment.
That’s the plan. I want to eventually remove the completion-based methods, but I've kept them for this release to ensure stability. Since the async version hasn't been widely tested yet, this allows partners to keep using the SDK without being forced to downgrade if they run into bugs.
| public typealias AuthCompletion = (Result<Client, UberAuthError>) -> () | ||
|
|
||
| /// @mockable | ||
| public protocol UberAuthInterface { |
There was a problem hiding this comment.
How about we split these into two different interfaces? As it stands, conformers would need to implement the async methods even if they don't support them.
There was a problem hiding this comment.
Agreed. I’ll refactor these into two interfaces
luizgustavo09
changed the title
🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
luizgustavo09
force-pushed
the
refactor/main/swift-concurrency
branch
from
7ef52b7 to
c0d2d1f
Compare
luizgustavo09
changed the title
|
|
||
| /// Public interface for the uber-auth-ios library | ||
| public final class UberAuth: AuthManaging { | ||
| public final class UberAuth: UberAuthInterface, UberAuthAsyncInterface, AuthManaging { |
There was a problem hiding this comment.
Thanks for splitting it up. I think we may have the same problem here though, where a caller will need to support asynchronous/await to even use UberAuth.
I am fine with keeping this however if we're willing to make this a requirement for future upgrades.
There was a problem hiding this comment.
Basically since UberAuth conforms to the UberAuthInterface and UberAuthAsyncInterface it's basically the same.
To make them truly separate we'd need to either (1) create an UberAuth and a UberAuthAsync or (2) create a new separate UberAuthAsync framework in the Package.swift
There was a problem hiding this comment.
Discussed offline. Since we're only supporting 15+ now this should be fine
3 participants
Description
This PR adds modern async/await versions of all authentication methods while maintaining full backward compatibility with existing completion-based APIs.
New public API:
Key changes:
UberAuth,AuthProviding, andNetworkProviderTesting