Repository navigation
[google_sign_in] PR 2/4 Migrate the plugin class from Objective-C to Swift. - #12655
Conversation
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
There was a problem hiding this comment.
Code Review
This pull request migrates the google_sign_in_ios plugin class from Objective-C to Swift, replacing FLTGoogleSignInPlugin with GoogleSignInPlugin and adding an Objective-C exception catcher helper. Feedback on the changes suggests simplifying the sanitizedUserInfo function by using a non-optional parameter, updating flutterError to handle the optional error mapping, and correcting a signature mismatch in the scene(_:openURLContexts:) delegate method to return Void instead of Bool while reusing the handleURLs helper.
| private func sanitizedUserInfo(_ value: Any?) -> Any { | ||
| switch value { | ||
| case let error as NSError: | ||
| return [ | ||
| "domain": error.domain, | ||
| "code": "\(error.code)", | ||
| "localizedDescription": error.localizedDescription, | ||
| "userInfo": sanitizedUserInfo(error.userInfo), | ||
| ] | ||
| case let string as String: | ||
| return string | ||
| case let url as URL: | ||
| return url.absoluteString | ||
| case let number as NSNumber: | ||
| return number | ||
| case let array as [Any]: | ||
| return array.map { sanitizedUserInfo($0) } | ||
| case let dict as [AnyHashable: Any]: | ||
| var safeValues: [AnyHashable: Any] = [:] | ||
| safeValues.reserveCapacity(dict.count) | ||
| for (key, nestedValue) in dict { | ||
| safeValues[key] = sanitizedUserInfo(nestedValue) | ||
| } | ||
| return safeValues | ||
| default: | ||
| if let value { | ||
| return "[Unsupported type: \(String(describing: type(of: value)))]" | ||
| } | ||
| return "[Unsupported type: nil]" | ||
| } | ||
| } |
There was a problem hiding this comment.
sanitizedUserInfo can be simplified by making its parameter non-optional Any. Since userInfo dictionaries and nested values are typically non-optional when traversed, we can avoid the awkward default case check for nil and make the code more robust.
private func sanitizedUserInfo(_ value: Any) -> Any {
switch value {
case let error as NSError:
return [
"domain": error.domain,
"code": "\(error.code)",
"localizedDescription": error.localizedDescription,
"userInfo": sanitizedUserInfo(error.userInfo),
]
case let string as String:
return string
case let url as URL:
return url.absoluteString
case let number as NSNumber:
return number
case let array as [Any]:
return array.map { sanitizedUserInfo($0) }
case let dict as [AnyHashable: Any]:
var safeValues: [AnyHashable: Any] = [:]
safeValues.reserveCapacity(dict.count)
for (key, nestedValue) in dict {
safeValues[key] = sanitizedUserInfo(nestedValue)
}
return safeValues
default:
return "[Unsupported type: \(String(describing: type(of: value)))]"
}
}There was a problem hiding this comment.
Thanks, going to keep Any? here. The Obj-C helper (FSISanitizedUserInfo) takes a nullable id and is called with a nil NSError (and thus nil userInfo) from FSIFlutterErrorForNSError. The default nil branch is that path, not leftover optionality. Tightening this to Any would require changing how nil errors are mapped, which we don't want in this port
c2332fb to
2cf54ae
Compare
f4ccb35 to
34f5240
Compare
2cf54ae to
b306c56
Compare
34f5240 to
9dce861
Compare
b306c56 to
a6041e6
Compare
9dce861 to
93fbe9d
Compare
93fbe9d to
392df80
Compare
cbracken
left a comment
There was a problem hiding this comment.
Overall looks good -- mostly just nits.
| for url in urls { | ||
| _ = signIn.handle(url) | ||
| } | ||
| } |
There was a problem hiding this comment.
Where is this called from? It looks like maybe it was intended to be called from
public func scene(_ scene: UIScene, openURLContexts urlContexts: Set<UIOpenURLContext>) -> Boolbut that code effectively inlines the logic, or rather slightly different logic since this discards the result. You could either update this and use it or delete.
There was a problem hiding this comment.
Good catch, it was only used from tests and discarded the handle result. It now returns Bool (true if GIDSignIn handled any URL), and scene(_:openURLContexts:) calls it. Also added a test that the return value matches signIn.handle
There was a problem hiding this comment.
Not the end of the world but handleURLs() and handleOpen() are now the same implementation. You could probably just call handleOpen() in both cases since it's already public, then delete handleURLs and tweak the #else part of this and make it just an #if around the application(...) above.
There was a problem hiding this comment.
Done. scene and tests call handleOpen. handleURLs is removed. The #if os(iOS) now only wraps application(_:open:options:)
| return .canceled | ||
| case GIDSignInError.hasNoAuthInKeychain.rawValue: | ||
| return .noAuthInKeychain | ||
| case -6: // kGIDSignInErrorCodeEMM; not imported as a Swift enum case. |
There was a problem hiding this comment.
GIDSignInError.EMM looks imported to me. Is there any reason we can't use GIDSignInError.EMM.rawValue like the others?
There was a problem hiding this comment.
You’re right, .EMM is imported (the tests already used GIDSignInError.EMM.rawValue). Updated the mapping to use that instead of -6
There was a problem hiding this comment.
Awesome 🎉. Please update the commit comment to remove the reference to the -6.
There was a problem hiding this comment.
Already gone in the current revision
| ) { | ||
| if let userID = user.userID { | ||
| usersByIdentifier[userID] = user | ||
| } |
There was a problem hiding this comment.
If userID is nil, we never populate this, but then we default to "" below and tell Dart that sign in succeeded instead of erroring out.
We should do this as:
guard let userID = userID else { completion(nil, ...) }
usersByIdentifier[userID] = userThen no need for the defaulting it below since userID is non-nil.
The old obj-c code would have crashed assigning a nil key to the dictionary, completing with an error is definitely better.
There was a problem hiding this comment.
Agreed that completing with an error is better than crashing on a nil dictionary key, and better than reporting success with userId: "". Left this out of this PR because it changes the Dart-visible path (success → error). Happy to do it as a follow-up if you’d rather have it here
There was a problem hiding this comment.
The current code changes the behaviour as well -- previously we would crash will a null deref, but now we return success. That seems like a bigger change than having it correctly return an error.
There was a problem hiding this comment.
Agreed, silent success is worse than the old crash. didSignIn now guards userID and completes with a FlutterError instead of userId: ""
| if let value { | ||
| return "[Unsupported type: \(String(describing: type(of: value)))]" | ||
| } | ||
| return "[Unsupported type: nil]" |
There was a problem hiding this comment.
| if let value { | |
| return "[Unsupported type: \(String(describing: type(of: value)))]" | |
| } | |
| return "[Unsupported type: nil]" | |
| guard let value else { | |
| return "[Unsupported type: (null)]" | |
| } | |
| return "[Unsupported type: \(String(describing: type(of: value)))]" |
There was a problem hiding this comment.
You mentioned this in your reply to gemini below re: Any? vs Any but since NSStringFromClass(Nil) returns nil %@ renders that as "(null)" so the above should match the old behaviour.
There was a problem hiding this comment.
Done. Nil now sanitizes to [Unsupported type: (null)] to match NSStringFromClass(Nil) / %@
cbracken
left a comment
There was a problem hiding this comment.
Just a couple really minor things overall LGTM. If I'm not around after the fixes, anyone else should go for a re-approve :)
… Sign-In - Updated `handleURLs` method to return a boolean indicating if any URLs were handled by Google Sign-In. - Added tests to verify the correct handling of URLs and the return value of `handleURLs`. - Updated CHANGELOG to reflect the changes in URL handling behavior.
- Returns an error when Google Sign-In reports a user without a user ID. - Updated tests to verify behavior when user ID is nil and when neither user nor error is present. - Refactored URL handling method name from `handleURLs` to `handleOpen` for clarity. - Updated CHANGELOG to reflect the new error handling behavior.
243873d to
0e8cc7c
Compare
…er#193289) Roll Packages from c2b58e1a97fb to 431ea69a42e2 (56 revisions) flutter/packages@c2b58e1...431ea69 2026-09-24 10687576+bparrishMines@users.noreply.github.com [cross_file] Updates cross_file to a package separated federated plugin (flutter/packages#11010) 2026-09-23 21270878+elliette@users.noreply.github.com [material_ui] Remove static access of `copyWith` from `dart fix` golden tests for `ThemeData/TextTheme` (flutter/packages#12994) 2026-09-23 happytoday83@naver.com [camera_avfoundation] Replace deprecated high-resolution capture APIs (flutter/packages#12372) 2026-09-23 tarrinneal@gmail.com [pigeon] update nullish checks to use new isNullish method (flutter/packages#12985) 2026-09-23 katelovett@google.com [ci] Assign batch release PR approver as reviewer on sync-back PR (flutter/packages#12976) 2026-09-23 kf013099@gmail.com [image_picker] Fix scaling 10-bit images (flutter/packages#12557) 2026-09-23 kevmoo@users.noreply.github.com [material_ui] Improve Autocomplete, DrawerHeader, and Tooltip accessibility (flutter/packages#12918) 2026-09-23 katelovett@google.com [google_fonts] Decouple GoogleFontsLite for full tree-shaking and add full feature parity (flutter/packages#12830) 2026-09-23 21270878+elliette@users.noreply.github.com [file_selector] Ignore flakey `file_selector_android` tests (flutter/packages#12992) 2026-09-23 31859944+LongCatIsLooong@users.noreply.github.com [CI] Remove bringup from `Mac_arm64 build_all_packages` targets (flutter/packages#12978) 2026-09-22 1961493+harryterkelsen@users.noreply.github.com [material_ui] Unskip 5 passing web tests in bottom_app_bar_test and text_field_test (flutter/packages#12983) 2026-09-22 1961493+harryterkelsen@users.noreply.github.com [cupertino_ui] Unskip 5 passing web tests in adaptive_text_selection_toolbar_test and text_field_test (flutter/packages#12984) 2026-09-22 me@davidmiguel.com [go_router] Fix ShellRoute chrome dropped from semantics tree by ModalBarrier (flutter/packages#12353) 2026-09-22 36861262+QuncCccccc@users.noreply.github.com [material_ui] Add contrastLevel for M3 ColorScheme (flutter/packages#12743) 2026-09-22 themis.chatzie@gmail.com [go_router] Preserve nested pushes during config updates (flutter/packages#12611) 2026-09-22 36861262+QuncCccccc@users.noreply.github.com [material_ui] Migrate M3 Chip template to use new gen_defaults (flutter/packages#12847) 2026-09-22 50643541+Mairramer@users.noreply.github.com [material_ui] Fix LocalHistoryEntry leak when double tapping Drawer scrim (flutter/packages#12552) 2026-09-22 katelovett@google.com Update suggested reviewers (flutter/packages#12974) 2026-09-22 fluttergithubbot@gmail.com Sync release-material_ui-1.4.0 to main (flutter/packages#12966) 2026-09-22 fluttergithubbot@gmail.com Sync release-cupertino_ui-1.1.1 to main (flutter/packages#12965) 2026-09-21 21270878+elliette@users.noreply.github.com [ci] Update repo for 3.47 stable release (flutter/packages#12959) 2026-09-21 engine-flutter-autoroll@skia.org Manual roll Flutter from 27fec0e to 4fcd90b (1 revision) (flutter/packages#12904) 2026-09-21 katelovett@google.com Revert "[go_router_builder] Migrate to material_ui" (flutter/packages#12962) 2026-09-21 31859944+LongCatIsLooong@users.noreply.github.com [material_ui] Update `material_ui` tests to prevent them from failing when framework `TextStyle` changes (flutter/packages#12728) 2026-09-21 katelovett@google.com [go_router_builder] Migrate to material_ui (flutter/packages#12913) 2026-09-21 dkwingsmt@users.noreply.github.com [material_ui, cupertino_ui] Bump Flutter version from to 3.47 (flutter/packages#12944) 2026-09-21 katelovett@google.com [cupertino_ui] Work around dart2wasm optional parameter inference bug in example checkbox tests (flutter/packages#12961) 2026-09-21 katelovett@google.com [cupertino_ui] Work around dart2wasm optional parameter inference bug in checkbox_test.dart (flutter/packages#12956) 2026-09-21 victor.orozco@cloudsufi.com [google_sign_in] PR 3/4 Migrate ViewProvider and GID SDK wrappers from Objective-C to Swift (flutter/packages#12657) 2026-09-21 victor.orozco@cloudsufi.com [google_sign_in] PR 2/4 Migrate the plugin class from Objective-C to Swift. (flutter/packages#12655) 2026-09-20 10687576+bparrishMines@users.noreply.github.com [cross_file_darwin] iOS/macOS implementation of `cross_file` (flutter/packages#12910) 2026-09-20 10687576+bparrishMines@users.noreply.github.com [cross_file_web] Web implementation of `cross_file` (flutter/packages#12908) 2026-09-20 36861262+QuncCccccc@users.noreply.github.com [material_ui] Migrate M3 `InputChip` template to use new gen_defaults (flutter/packages#12850) 2026-09-20 36861262+QuncCccccc@users.noreply.github.com [material_ui] Migrate M3 Checkbox defaults to new gen_defaults (flutter/packages#12816) 2026-09-20 36861262+QuncCccccc@users.noreply.github.com [material_ui] Migrate M3 `FilterChip` template to use new gen_defaults (flutter/packages#12849) 2026-09-19 zengyan@88.com [material_ui] Announce PopupMenuButton with a child as a button (flutter/packages#12585) 2026-09-19 engine-flutter-autoroll@skia.org Roll Flutter (stable) from 9584c67 to 6a19cca (7 revisions) (flutter/packages#12950) 2026-09-18 21270878+elliette@users.noreply.github.com [material_ui] Reduce the `material_ui` non-wasm web test suite (flutter/packages#12763) 2026-09-18 31859944+LongCatIsLooong@users.noreply.github.com Replace `Mac_x64` builders with `Mac_arm64` ones (flutter/packages#12941) 2026-09-18 32538273+ValentinVignal@users.noreply.github.com [in_app_purchase_storekit] Use `_$SKPaymentDiscountWrapperToJson` (flutter/packages#12721) 2026-09-18 gerardo.morales@cloudsufi.com [In_app _purchase] README excerpt examples and changelog (flutter/packages#12739) 2026-09-18 brunocorona.alcantar@gmail.com [material_ui] Add alternative input method for `RangeSlider` in `NavigationMode.directional` (flutter/packages#12630) 2026-09-18 10687576+bparrishMines@users.noreply.github.com [cross_file_android] Android implementation of `cross_file` (flutter/packages#12909) 2026-09-18 50643541+Mairramer@users.noreply.github.com [camera_platform_interface] Adds videoOutputPath support to startVideoRecording (flutter/packages#12667) 2026-09-17 dkwingsmt@users.noreply.github.com [cupertino_ui] Migrate a snippet in `CupertinoCheckbox`'s API doc to `{@example}` and add unit tests (flutter/packages#12180) 2026-09-17 36861262+QuncCccccc@users.noreply.github.com [material_ui] Default gen_defaults color helper prefix (flutter/packages#12848) ...
Migrates
FLTGoogleSignInPluginfrom Objective-C to Swift (GoogleSignInPlugin.swift). GID SDK wrappers andViewProviderstay Obj-C for this PR.Intended as a 1:1 move of configure, restorePreviousSignIn, signIn, refreshedAuthorizationTokens, addScopes, signOut, disconnect, error-code mapping, URL handling, and the iOS view-controller / macOS window presentation paths.
presentingViewController/presentingWindowon the Obj-C wrapper protocol become nullable. That matches existing runtime behavior:FSIViewProvider.viewControllerwas already nullable.Bumps
google_sign_in_iosto 6.3.3.PR 2/4 of the Obj-C → Swift migration. Depends on PR 1/4 (SPM packaging). 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