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] Mark *Credential classes as Sendable #14100

Merged
merged 1 commit into from
Nov 12, 2024
Merged

Conversation

ncooke3
Copy link
Member

@ncooke3 ncooke3 commented Nov 12, 2024

These classes have no mutable state so they should meet the semantic requirements of Sendable. In Firebase 12, we should refactor to checked Sendable. We can't currently do so without a breaking change because of the fact that the classes are open (Sendable classes may not be open).

Resolves:
Screenshot 2024-11-12 at 4 45 07 PM

Screenshot 2024-11-12 at 4 45 25 PM

Because the APIs return the AuthCredential superclass, AuthCredential needs to be marked unchecked Sendable. All of it's subclasses need to then be marked unchecked Sendable or a warning will occur that the sendability of the superclass means the subclass should also explicitly mark itself as unchecked Sendable.

I added some API improvement ideas to the Firebase 12 gdoc.

#no-changelog

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.

These classes have no mutable state so they should meet the semantic requirements of Sendable. In Firebase 12, we should refactor to checked Sendable. We can't currently do so without a breaking change because of the fact that the classes are open (Sendable classes may not be open).

Only AuthCredential and OAuthCredential, PhoneAuthCredential are open. Ah, all are subclasses of an open class!

LGTM.

@ncooke3
Copy link
Member Author

ncooke3 commented Nov 12, 2024

These classes have no mutable state so they should meet the semantic requirements of Sendable. In Firebase 12, we should refactor to checked Sendable. We can't currently do so without a breaking change because of the fact that the classes are open (Sendable classes may not be open).

Only AuthCredential and OAuthCredential, PhoneAuthCredential are open. Ah, all are subclasses of an open class!

LGTM.

Yes, I think one approach to consider in the future is making them structs that conform to a common protocol (that conforms to Sendable). And for ObjC compat, make an ObjC only wrapper to preserve the current API.

@ncooke3 ncooke3 merged commit 6ecea25 into main Nov 12, 2024
55 checks passed
@ncooke3 ncooke3 deleted the nc/sendable-credential branch November 12, 2024 22:02
@firebase firebase locked and limited conversation to collaborators Dec 13, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants