feat(ui_oauth_google)!: migrate to google_sign_in 7 - #700
bjrochem72 wants to merge 4 commits into
Conversation
Migrates GoogleProvider to the google_sign_in 7 API, which adopts the UIScene lifecycle on iOS (google_sign_in_ios 6.3.0) and Swift Package Manager (6.3.3). Fixes the deprecated application lifecycle warning tracked in firebase#673. - GoogleSignIn is now the shared instance and is initialized once before the first sign-in. - Authentication and authorization are separate steps. The provider reuses an existing authorization for the requested scopes and prompts for consent when one is not available, so the credential still carries an access token when scopes are requested. - A cancelled sign-in surfaces as AuthCancelledException, matching the previous flow reset behaviour. - Adds an optional serverClientId parameter to GoogleProvider, GoogleSignInButton and GoogleSignInIconButton for Android apps that do not use google-services.json. - Updates the integration test mocks to the new API. BREAKING CHANGE: consumers must follow the google_sign_in 7 platform integration steps: GIDClientID in Info.plist on iOS, and on Android a web OAuth client entry in google-services.json or an explicit serverClientId. When no scopes are requested the credential now contains only an ID token.
Chain the retry-reset catchError directly onto initialize() instead of threading an intermediate local, and add a @VisibleForTesting debugReset() that clears the static one-time-init future so each test starts from a clean state with its freshly injected mock. Addresses review feedback on firebase#689.
![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 upgrades the google_sign_in dependency to version 7.1.0 and updates the GoogleProvider implementation to align with the new API, which requires explicit initialization and the use of authenticate instead of signIn. Additionally, support for serverClientId is added, and the test suite is updated to mock the new initialization and authentication methods. A review comment suggests improving error handling in _ensureInitialized by using a try-catch block with rethrow to preserve stack traces during debugging.
| Future<void> _ensureInitialized() { | ||
| return _initialization ??= provider | ||
| .initialize( | ||
| clientId: _ignoreClientId() ? null : clientId, | ||
| serverClientId: serverClientId, | ||
| ) | ||
| .catchError((Object err) { | ||
| // Allow a later sign-in attempt to retry initialization instead of | ||
| // rethrowing the same stale error forever. | ||
| _initialization = null; | ||
| throw err; | ||
| }); | ||
| } |
There was a problem hiding this comment.
Using .catchError and then throwing the error with throw err resets the stack trace to the current line, which makes debugging production errors much harder because the original stack trace of the initialization failure is lost.
Instead, use a standard try-catch block with rethrow to preserve the original stack trace. This is also more idiomatic in Dart than chaining .catchError on a future.
Future<void> _ensureInitialized() async {
if (_initialization != null) {
await _initialization;
return;
}
try {
_initialization = provider.initialize(
clientId: _ignoreClientId() ? null : clientId,
serverClientId: serverClientId,
);
await _initialization;
} catch (err) {
_initialization = null;
rethrow;
}
}There was a problem hiding this comment.
Good catch, thanks. Fixed in 27ea07d — kept the memoised retry-reset structure and swapped the bare throw err for Error.throwWithStackTrace(err, stackTrace), which preserves the original initialization stack trace.
Use Error.throwWithStackTrace in the initialize() catchError so a failed initialization keeps its original stack trace instead of resetting it with a bare throw.
|
Hi Russell, thanks for the heads-up, no problem at all. I have reopened the change here with the same commits, rebased onto current main, so it is ready to merge after today's release whenever suits. I also rebased the small sibling PR #676 (a non-breaking one-line widening of the |
1 participant
Re-opening the change from #689. That PR was merged and then reverted from
mainwhile breaking changes are held for the upcoming release. Same changes here, ready to merge after today's normal release.Description
Migrates
firebase_ui_oauth_googlefrom google_sign_in 6.x to 7.x.google_sign_in 6.x pins apps to google_sign_in_ios 5.x, which still uses the deprecated UIApplication delegate lifecycle. Moving to 7.x resolves google_sign_in_ios 6.3.x, which adopted the UIScene lifecycle in 6.3.0 and added a Swift Package Manager target in 6.3.3. This clears the FLTGoogleSignInPlugin deprecation warning that iOS apps currently log on startup.
What changed:
GoogleProvidernow uses the sharedGoogleSignIn.instanceand initializes it once before the first sign-in.authorizationForScopesand falls back toauthorizeScopes, so the resulting Firebase credential still carries an access token as it did in 6.x. When no scopes are requested the credential contains only an ID token, which is all Firebase Auth needs.GoogleSignInExceptionCode.canceled) is surfaced asAuthCancelledException, so the auth flow resets exactly as before.serverClientIdparameter onGoogleProvider,GoogleSignInButtonandGoogleSignInIconButton. On Android an ID token requires a server client ID. Apps that usegoogle-services.jsoncontaining a web OAuth client entry need no change; other apps can now pass it explicitly.The constraint is
^7.1.0because 7.1.0 added theGoogleSignInExceptionCodeexport needed for structured cancel handling.Verified locally on the workspace:
dart analyzereports no problems,firebase_ui_oauth(7/7) andfirebase_ui_auth(69/69) test suites pass, and thetestspackage compiles against the migrated API.Related Issues
Fixes #673. Related to the wider UIScene migration alongside #672 / #676.
Checklist
Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes (
[x]).This will ensure a smooth and quick review process. Updating the
pubspec.yamland changelogs is not required.///).melos run analyze) does not report any problems on my PR.melos run test:unit:alldoesn't fail).