[google_sign_in] PR 3/4 Migrate ViewProvider and GID SDK wrappers from Objective-C to Swift - #12657
Conversation
There was a problem hiding this comment.
Code Review
This pull request migrates the ViewProvider and GID SDK wrappers from Objective-C to Swift within the iOS Google Sign-In plugin. It removes the legacy Objective-C files and introduces equivalent Swift protocols, wrappers, and view providers, while updating the plugin implementation and tests accordingly. The review feedback recommends avoiding force-unwrapping of non-optional properties (such as user, accessToken, and refreshToken) in the wrapper classes by implementing non-failable initializers for GIDGoogleUserWrapper and GIDTokenWrapper.
| var user: GIDGoogleUserProtocol { | ||
| return GIDGoogleUserWrapper(user: result.user)! | ||
| } |
There was a problem hiding this comment.
Since result.user is a non-optional GIDGoogleUser, we can avoid force-unwrapping here by providing a non-failable initializer in GIDGoogleUserWrapper.
| var user: GIDGoogleUserProtocol { | |
| return GIDGoogleUserWrapper(user: result.user)! | |
| } | |
| var user: GIDGoogleUserProtocol { | |
| return GIDGoogleUserWrapper(user: result.user) | |
| } |
References
- Swift safety guidelines discourage the use of force-unwrapping (!) because it bypasses the compiler's type safety checks and can lead to runtime crashes. (link)
There was a problem hiding this comment.
Good catch, fixed. That unwrap was only needed because GIDGoogleUserWrapper had a failable init. Added a non-failable init(user:) for the non-optional SDK value and dropped the !
| init?(user: GIDGoogleUser?) { | ||
| guard let user else { return nil } | ||
| self.user = user | ||
| } |
There was a problem hiding this comment.
Provide a non-failable initializer for non-optional GIDGoogleUser instances, and make the failable initializer a convenience initializer. This allows wrapping non-optional users without force-unwrapping.
| init?(user: GIDGoogleUser?) { | |
| guard let user else { return nil } | |
| self.user = user | |
| } | |
| init(user: GIDGoogleUser) { | |
| self.user = user | |
| } | |
| convenience init?(user: GIDGoogleUser?) { | |
| guard let user else { return nil } | |
| self.init(user: user) | |
| } |
References
- Swift safety guidelines discourage the use of force-unwrapping (!) because it bypasses the compiler's type safety checks and can lead to runtime crashes. (link)
There was a problem hiding this comment.
Done. init(user: GIDGoogleUser) is the designated initializer, and convenience init?(user: GIDGoogleUser?) remains for the optional completion-handler values
| var accessToken: GIDTokenProtocol { | ||
| return GIDTokenWrapper(token: user.accessToken)! | ||
| } | ||
|
|
||
| var refreshToken: GIDTokenProtocol { | ||
| return GIDTokenWrapper(token: user.refreshToken)! | ||
| } |
There was a problem hiding this comment.
Since user.accessToken and user.refreshToken are non-optional, we can avoid force-unwrapping them by using a non-failable initializer in GIDTokenWrapper.
| var accessToken: GIDTokenProtocol { | |
| return GIDTokenWrapper(token: user.accessToken)! | |
| } | |
| var refreshToken: GIDTokenProtocol { | |
| return GIDTokenWrapper(token: user.refreshToken)! | |
| } | |
| var accessToken: GIDTokenProtocol { | |
| return GIDTokenWrapper(token: user.accessToken) | |
| } | |
| var refreshToken: GIDTokenProtocol { | |
| return GIDTokenWrapper(token: user.refreshToken) | |
| } |
References
- Swift safety guidelines discourage the use of force-unwrapping (!) because it bypasses the compiler's type safety checks and can lead to runtime crashes. (link)
There was a problem hiding this comment.
Agreed and fixed. accessToken and refreshToken are non-optional on GIDGoogleUser, so they now use the non-failable GIDTokenWrapper initializer. idToken is still optional, so that call site keeps the failable init
| init?(token: GIDToken?) { | ||
| guard let token else { return nil } | ||
| self.token = token | ||
| } |
There was a problem hiding this comment.
Provide a non-failable initializer for non-optional GIDToken instances, and make the failable initializer a convenience initializer. This allows wrapping non-optional tokens without force-unwrapping.
| init?(token: GIDToken?) { | |
| guard let token else { return nil } | |
| self.token = token | |
| } | |
| init(token: GIDToken) { | |
| self.token = token | |
| } | |
| convenience init?(token: GIDToken?) { | |
| guard let token else { return nil } | |
| self.init(token: token) | |
| } |
References
- Swift safety guidelines discourage the use of force-unwrapping (!) because it bypasses the compiler's type safety checks and can lead to runtime crashes. (link)
There was a problem hiding this comment.
Done, same pattern as the user wrapper: non-failable init(token:) plus convenience init?(token:) for optional tokens
f4ccb35 to
34f5240
Compare
661e862 to
0e55e56
Compare
34f5240 to
9dce861
Compare
b2aa6db to
f89ebdd
Compare
| code: 0, | ||
| userInfo: [ | ||
| NSLocalizedDescriptionKey: "No host view available to present Google Sign-In." | ||
| ])) |
There was a problem hiding this comment.
optional: this indentation pyramid is a bit too much. can you manually clean it up a bit?
There was a problem hiding this comment.
Done! Pulled the NSError into missingPresenterError so requirePresenter is just the nil check and completion
| } | ||
|
|
||
| func signOut() { | ||
| signIn.signOut() |
There was a problem hiding this comment.
the signIn ivar name is odd here.
There was a problem hiding this comment.
Agreed. signIn.signOut() / signIn.signIn(...) was confusing next to the protocol methods. Renamed the stored GIDSignIn to gidSignIn
| NSLocalizedDescriptionKey: "No host view available to present Google Sign-In." | ||
| ]) | ||
|
|
||
| private func requirePresenter<Presenter>( |
There was a problem hiding this comment.
this func is confusing. I would just inline it.
There was a problem hiding this comment.
Inlined it. The generic helper was hiding a simple nil check; signIn/addScopes now guard the presenter and complete with missingPresenterError directly
9dce861 to
93fbe9d
Compare
48c44fb to
3832e69
Compare
93fbe9d to
392df80
Compare
bc6b4a3 to
878c0f9
Compare
|
From triage: Please rebase off main so that tests will pass |
…bjective-C to Swift.
…ignInWrapper. Introduces a new `requirePresenter` function to handle cases where the presenter is nil, preventing crashes and returning appropriate errors. Adds unit tests to verify behavior when signing in without a presenter, ensuring robust error reporting for the Google Sign-In process.
…riable name and improve error handling. Renames the `signIn` variable to `gidSignIn` for clarity and consistency. Introduces a private `missingPresenterError` to streamline error handling in the `requirePresenter` function, enhancing code readability and maintainability.
Removes the `requirePresenter` function and directly checks for the presence of the presenter in the `signIn` methods. This change enhances code clarity and maintains consistent error handling for missing presenters, ensuring robust error reporting during the Google Sign-In process.
878c0f9 to
bf080a0
Compare
Migrates
ViewProviderand the GID SDK wrappers from Objective-C to Swift. The plugin class from PR 2/4 now talks to Swift protocols instead ofFSIViewProvider/FSIGIDSignIn.The Swift GID API takes a non-optional presenter. Obj-C
nonnullwas not enforced at runtime, so a nil registrar view (headless / deallocated engine) was a silent no-op. Force-unwrapping that in Swift would crash. This PR completes with anNSErrorinstead, mapped to a Flutter error like other unexpected failures.Adds unit tests for the missing-presenter path (
signIn/addScopeson iOS and macOS).Bumps
google_sign_in_iosto 6.3.4.PR 3/4 of the Obj-C → Swift migration. Depends on PR 2/4 (plugin class). Continues flutter/flutter#119103
Pre-Review Checklist
[shared_preferences]///).If you need help, consider asking for advice on the #hackers-new channel on Discord.
Note: The Flutter team is currently trialing the use of Gemini Code Assist for GitHub. Comments from the
gemini-code-assistbot should not be taken as authoritative feedback from the Flutter team. If you find its comments useful you can update your code accordingly, but if you are unsure or disagree with the feedback, please feel free to wait for a Flutter team member's review for guidance on which automated comments should be addressed.Footnotes
Regular contributors who have demonstrated familiarity with the repository guidelines only need to comment if the PR is not auto-exempted by repo tooling. ↩ ↩2