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

[Auth] Convert *Response classes to structs #14012

Merged
merged 10 commits into from
Nov 4, 2024
Merged

[Auth] Convert *Response classes to structs #14012

merged 10 commits into from
Nov 4, 2024

Conversation

ncooke3
Copy link
Member

@ncooke3 ncooke3 commented Nov 1, 2024

I want to experiment with an alternative approach to #14006 where AuthBackend is instead made an actor. One of the first errors I hit when doing so is that the T.Response types need to be Sendable. Making them structs satisfies that constraint. I'm breaking this up into it's own PR as it's a prereq to exploring an alternative approach to #14006 and will be needed in either approach.

One important consideration when doing this is to ensure nothing depends on these new structures having reference semantics, else they may be left referencing an outdated copy. I audited all usages of AuthBackend.call(request: Request) -> Request.Response and the returned response was always a locally captured let, so if the response would have been mutated, the response would have been assigned to a var. Additionally, I would have expected to see something like: self.savedResponse = response, etc.

#no-changelog

@ncooke3 ncooke3 marked this pull request as ready for review November 1, 2024 23:32
Copy link
Contributor

@andrewheard andrewheard left a comment

Choose a reason for hiding this comment

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

LGTM. In a follow-up PR, let's investigate if https://github.com/firebase/firebase-ios-sdk/pull/14012/files#diff-870dc0117add1bfe1fac98e8c6559777840cef238c3c14f22c9a2c0dcfa356f0R254-R308 can be reasonably refactored to set the fields at creation time to avoid the need for AuthRPCResponse.setFields to be mutating.

@ncooke3 ncooke3 merged commit cdaa05f into main Nov 4, 2024
83 checks passed
@ncooke3 ncooke3 deleted the nc/auth-single branch November 4, 2024 18:21
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.

3 participants