-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
[Auth]: Add auth state stream utility #14628
base: main
Are you sure you want to change the base?
Conversation
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist is currently in preview and may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments to provide feedback. |
/gemini review |
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.
Code Review
This pull request introduces a utility for observing authentication state changes using Swift's AsyncStream
. The AuthState
struct and the authStateChanges
property provide a convenient way to handle authentication events asynchronously. Overall, the changes seem well-structured and address the stated goal of adding concurrency to Auth
.
Summary of Findings
- Naming consistency: Consider renaming
AuthStateChanges
toauthStateChangesStream
to clearly indicate that it is a stream ofAuthState
objects. - Open access modifier: The
open
access modifier is used for both theAuthState
struct and theauthStateChanges
property. While this allows for subclassing and overriding, it might be more appropriate to usepublic
if external modification is not intended.
Merge Readiness
The changes introduce a useful utility for observing authentication state changes asynchronously. While the code appears functional, addressing the naming consistency and access modifier considerations would further improve its clarity and maintainability. I am unable to approve this pull request, and recommend that others review and approve this code before merging. I recommend that the author address the comments before merging.
open var authStateChanges: AsyncStream<AuthState> { | ||
return AsyncStream { continuation in | ||
let listener = self.addStateDidChangeListener { auth, user in | ||
continuation.yield(AuthState(auth: auth, user: user)) | ||
} | ||
|
||
continuation.onTermination = { _ in | ||
self.removeStateDidChangeListener(listener) | ||
} | ||
} | ||
} |
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.
Consider renaming this property to authStateChangesStream
to clearly indicate that it is a stream of AuthState
objects. This would improve readability and make the intent more explicit.
open var authStateChanges: AsyncStream<AuthState> { | |
return AsyncStream { continuation in | |
let listener = self.addStateDidChangeListener { auth, user in | |
continuation.yield(AuthState(auth: auth, user: user)) | |
} | |
continuation.onTermination = { _ in | |
self.removeStateDidChangeListener(listener) | |
} | |
} | |
} | |
open var authStateChangesStream: AsyncStream<AuthState> { | |
return AsyncStream { continuation in | |
let listener = self.addStateDidChangeListener { auth, user in | |
continuation.yield(AuthState(auth: auth, user: user)) | |
} | |
continuation.onTermination = { _ in | |
self.removeStateDidChangeListener(listener) | |
} | |
} | |
} |
4acb04c
to
0d91e31
Compare
0d91e31
to
c940336
Compare
Thanks for the PR, @nidegen! Are there use cases where you were intending to use the stream API? As a 1:1 replacement for the callback listener or perhaps in SwiftUI views? |
Just as a 1:1 replacement for the callback listener initially but definitely with SwiftUI tasks in mind:) Not a big pain point, but I thought it might be welcome! |
Hi there!
Not sure if API extensions are also not allowed, but I figured this little helper could be useful to add some concurrency in
Auth
.The struct
AuthState
is only there to avoid concurrency issues that appear when using a tuple instead.