[google_sign_in] PR 3/4 Migrate ViewProvider and GID SDK wrappers from Objective-C to Swift - #12657
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
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
…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.
661e862 to
0e55e56
Compare
1 participant
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