Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion packages/google_sign_in/google_sign_in_web/CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,4 +1,8 @@
## 1.1.1
## 1.1.2

* Reverts "Throws a more actionable error when init is called more than once."
Comment on lines +1 to +3

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The changelog entry for version 1.1.2 correctly indicates the revert. This is in line with good release practices.


## 1.1.1 (withdrawn)

* Throws a more actionable error when init is called more than once.
* Updates minimum supported SDK version to Flutter 3.35/Dart 3.9.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -65,36 +65,8 @@ void main() {
await plugin.init(
const InitParameters(clientId: 'some-non-null-client-id'),
);
});

testWidgets('throws if init is called twice', (_) async {
await plugin.init(
const InitParameters(clientId: 'some-non-null-client-id'),
);

// Calling init() a second time should throw state error
expect(
() => plugin.init(
const InitParameters(clientId: 'some-non-null-client-id'),
),
throwsStateError,
);
});

testWidgets('throws if init is called twice synchronously', (_) async {
final Future<void> firstInit = plugin.init(
const InitParameters(clientId: 'some-non-null-client-id'),
);

// Calling init() a second time synchronously should throw state error
expect(
() => plugin.init(
const InitParameters(clientId: 'some-non-null-client-id'),
),
throwsStateError,
);

await firstInit;
expect(plugin.initialized, completes);
});
Comment on lines 68 to 70

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The removal of the tests for init being called twice is appropriate given the revert of the functionality that threw an error in such cases. The new expect(plugin.initialized, completes); correctly verifies the initialization.


testWidgets('asserts clientId is not null', (_) async {
Expand All @@ -113,6 +85,35 @@ void main() {
);
}, throwsAssertionError);
});

testWidgets('must be called for most of the API to work', (_) async {
expect(() async {
await plugin.attemptLightweightAuthentication(
const AttemptLightweightAuthenticationParameters(),
);
}, throwsStateError);

expect(() async {
await plugin.clientAuthorizationTokensForScopes(
const ClientAuthorizationTokensForScopesParameters(
request: AuthorizationRequestDetails(
scopes: <String>[],
userId: null,
email: null,
promptIfUnauthorized: false,
),
),
);
}, throwsStateError);

expect(() async {
await plugin.signOut(const SignOutParams());
}, throwsStateError);

expect(() async {
await plugin.disconnect(const DisconnectParams());
}, throwsStateError);
});
Comment on lines +89 to +116

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The new test must be called for most of the API to work is a good addition. It ensures that methods requiring initialization will throw a StateError if init has not been called, which is crucial for correct API usage.

});

group('support queries', () {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ class GoogleSignInPlugin extends GoogleSignInPlatform {
@visibleForTesting GisSdkClient? debugOverrideGisSdkClient,
@visibleForTesting
StreamController<AuthenticationEvent>? debugAuthenticationController,
}) : _debugOverrideGisSdkClient = debugOverrideGisSdkClient,
}) : _gisSdkClient = debugOverrideGisSdkClient,

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The change from _debugOverrideGisSdkClient to _gisSdkClient in the constructor initialization list is correct, as _gisSdkClient is now nullable and can be directly assigned the debug override.

_authenticationController =
debugAuthenticationController ??
StreamController<AuthenticationEvent>.broadcast() {
Expand All @@ -68,31 +68,51 @@ class GoogleSignInPlugin extends GoogleSignInPlatform {

// A future that completes when the JS loader is done.
late Future<void> _jsSdkLoadedFuture;

/// A completer used to track whether [init] has finished.
final Completer<void> _initCalled = Completer<void>();

/// A boolean flag to track if [init] has been called.
///
/// This is used to prevent race conditions when [init] is called multiple
/// times without awaiting.
bool _isInitCalled = false;
// A future that completes when the `init` call is done.
Completer<void>? _initCalled;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Changing _initCalled to a nullable Completer<void>? allows for its lazy initialization within the init method, which is a good pattern for ensuring it's only created when init is actually invoked.


// A StreamController to communicate status changes from the GisSdkClient.
final StreamController<AuthenticationEvent> _authenticationController;

// The instance of [GisSdkClient] backing the plugin.
// Using late final ensures it can only be set once and throws if accessed before initialization.
late final GisSdkClient _gisSdkClient;
GisSdkClient? _gisSdkClient;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Making _gisSdkClient nullable is necessary for its lazy initialization within the init method, allowing it to be set only after init is called.


// An optional override for the GisSdkClient, used for testing.
final GisSdkClient? _debugOverrideGisSdkClient;
// A convenience getter to avoid using ! when accessing _gisSdkClient, and
// providing a slightly better error message when it is Null.
GisSdkClient get _gisClient {
assert(
_gisSdkClient != null,
'GIS Client not initialized. '
'GoogleSignInPlugin::init() or GoogleSignInPlugin::initWithParams() '
'must be called before any other method in this plugin.',
);
return _gisSdkClient!;
}
Comment on lines +82 to +90

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The introduction of the _gisClient getter with an assertion for _gisSdkClient being non-null is a good practice. It provides a clear error message if the GIS client is accessed before init is called, improving developer experience and preventing null-pointer exceptions.


// This method throws if init or initWithParams hasn't been called at some
// point in the past. It is used by the [initialized] getter to ensure that
// users can't await on a Future that will never resolve.
void _assertIsInitCalled() {
if (_initCalled == null) {
throw StateError(
'GoogleSignInPlugin::init() or GoogleSignInPlugin::initWithParams() '
'must be called before any other method in this plugin.',
);
}
Comment on lines +95 to +101

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The _assertIsInitCalled method correctly enforces that init or initWithParams must be called before accessing the initialized future. This prevents awaiting a future that might never resolve if initialization hasn't occurred.

}

/// A future that resolves when the plugin is fully initialized.
///
/// This ensures that the SDK has been loaded, and that the `init` method
/// has finished running.
Future<void> get _initialized => _initCalled.future;
@visibleForTesting
Future<void> get initialized {
_assertIsInitCalled();
return Future.wait<void>(<Future<void>>[
_jsSdkLoadedFuture,
_initCalled!.future,
]);
Comment on lines +109 to +114

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The initialized getter now correctly uses _assertIsInitCalled and waits for both _jsSdkLoadedFuture and _initCalled!.future. This ensures that the plugin is fully ready before any operations are attempted.

}

/// Stores the client ID if it was set in a meta-tag of the page.
@visibleForTesting
Expand All @@ -105,14 +125,6 @@ class GoogleSignInPlugin extends GoogleSignInPlatform {

@override
Future<void> init(InitParameters params) async {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The removal of the _isInitCalled check and the StateError for calling init() multiple times is consistent with the revert. The new lazy initialization approach handles this scenario differently.

// Throw if init() is called more than once
if (_isInitCalled) {
throw StateError(
'init() has already been called. Calling init() more than once results in undefined behavior.',
);
}
_isInitCalled = true;

final String? appClientId = params.clientId ?? autoDetectedClientId;
assert(
appClientId != null,
Expand All @@ -126,27 +138,27 @@ class GoogleSignInPlugin extends GoogleSignInPlatform {
'serverClientId is not supported on Web.',
);

_initCalled = Completer<void>();

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Initializing _initCalled here ensures that the Completer is created only when init is actually invoked, supporting the lazy initialization pattern.


await _jsSdkLoadedFuture;

_gisSdkClient =
_debugOverrideGisSdkClient ??
GisSdkClient(
clientId: appClientId!,
nonce: params.nonce,
hostedDomain: params.hostedDomain,
authenticationController: _authenticationController,
loggingEnabled: kDebugMode,
);

_initCalled.complete();
_gisSdkClient ??= GisSdkClient(
clientId: appClientId!,
nonce: params.nonce,
hostedDomain: params.hostedDomain,
authenticationController: _authenticationController,
loggingEnabled: kDebugMode,
);
Comment on lines +145 to +151

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Using _gisSdkClient ??= for initialization is a good pattern. It ensures that the GisSdkClient is only created once, even if init is called multiple times, without throwing an error.


_initCalled!.complete(); // Signal that `init` is fully done.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Completing _initCalled here correctly signals that the initialization process is finished, allowing any awaiting futures to resolve.

}

@override
Future<AuthenticationResults?>? attemptLightweightAuthentication(
AttemptLightweightAuthenticationParameters params,
) {
_initialized.then((void value) {
_gisSdkClient.requestOneTap();
initialized.then((void value) {
_gisClient.requestOneTap();
Comment on lines +160 to +161

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The change from _initialized to initialized and _gisSdkClient to _gisClient correctly uses the new public getter and the safe access pattern for the GIS client.

});
// One tap does not necessarily return immediately, and may never return,
// so clients should not await it. Return null to signal that.
Expand All @@ -171,26 +183,26 @@ class GoogleSignInPlugin extends GoogleSignInPlatform {

@override
Future<void> signOut(SignOutParams params) async {
await _initialized;
await initialized;

await _gisSdkClient.signOut();
await _gisClient.signOut();
Comment on lines +186 to +188

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Consistent use of initialized and _gisClient ensures that signOut operations are performed only after the plugin is properly initialized and the GIS client is safely accessed.

}

@override
Future<void> disconnect(DisconnectParams params) async {
await _initialized;
await initialized;

await _gisSdkClient.disconnect();
await _gisClient.disconnect();
Comment on lines +193 to +195

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Consistent use of initialized and _gisClient ensures that disconnect operations are performed only after the plugin is properly initialized and the GIS client is safely accessed.

}

@override
Future<ClientAuthorizationTokenData?> clientAuthorizationTokensForScopes(
ClientAuthorizationTokensForScopesParameters params,
) async {
await _initialized;
await initialized;
_validateScopes(params.request.scopes);

final String? token = await _gisSdkClient.requestScopes(
final String? token = await _gisClient.requestScopes(
Comment on lines +202 to +205

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Consistent use of initialized and _gisClient ensures that clientAuthorizationTokensForScopes operations are performed only after the plugin is properly initialized and the GIS client is safely accessed.

params.request.scopes,
promptIfUnauthorized: params.request.promptIfUnauthorized,
userHint: params.request.userId,
Expand All @@ -204,7 +216,7 @@ class GoogleSignInPlugin extends GoogleSignInPlatform {
Future<ServerAuthorizationTokenData?> serverAuthorizationTokensForScopes(
ServerAuthorizationTokensForScopesParameters params,
) async {
await _initialized;
await initialized;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Consistent use of initialized ensures that serverAuthorizationTokensForScopes operations are performed only after the plugin is properly initialized.

_validateScopes(params.request.scopes);

// There is no way to know whether the flow will prompt in advance, so
Expand All @@ -213,9 +225,7 @@ class GoogleSignInPlugin extends GoogleSignInPlatform {
return null;
}

final String? code = await _gisSdkClient.requestServerAuthCode(
params.request,
);
final String? code = await _gisClient.requestServerAuthCode(params.request);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Consistent use of _gisClient ensures that requestServerAuthCode operations are performed only after the GIS client is safely accessed.

return code == null
? null
: ServerAuthorizationTokenData(serverAuthCode: code);
Expand All @@ -237,8 +247,8 @@ class GoogleSignInPlugin extends GoogleSignInPlatform {
Future<void> clearAuthorizationToken(
ClearAuthorizationTokenParams params,
) async {
await _initialized;
return _gisSdkClient.clearAuthorizationToken(params.accessToken);
await initialized;
return _gisClient.clearAuthorizationToken(params.accessToken);
Comment on lines +250 to +251

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Consistent use of initialized and _gisClient ensures that clearAuthorizationToken operations are performed only after the plugin is properly initialized and the GIS client is safely accessed.

}

@override
Expand Down Expand Up @@ -268,13 +278,13 @@ class GoogleSignInPlugin extends GoogleSignInPlatform {
configuration ?? GSIButtonConfiguration();
return FutureBuilder<void>(
key: Key(config.hashCode.toString()),
future: _initialized,
future: initialized,

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The FutureBuilder now correctly uses the public initialized getter, which includes the necessary checks for plugin readiness.

builder: (BuildContext context, AsyncSnapshot<void> snapshot) {
if (snapshot.hasData) {
return FlexHtmlElementView(
viewType: 'gsi_login_button',
onElementCreated: (Object element) {
_gisSdkClient.renderButton(element, config);
_gisClient.renderButton(element, config);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Using _gisClient here ensures that renderButton is called on a properly initialized GIS client, preventing potential runtime errors.

},
);
}
Expand Down
2 changes: 1 addition & 1 deletion packages/google_sign_in/google_sign_in_web/pubspec.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ description: Flutter plugin for Google Sign-In, a secure authentication system
for signing in with a Google account on Android, iOS and Web.
repository: https://github.com/flutter/packages/tree/main/packages/google_sign_in/google_sign_in_web
issue_tracker: https://github.com/flutter/flutter/issues?q=is%3Aissue+is%3Aopen+label%3A%22p%3A+google_sign_in%22
version: 1.1.1
version: 1.1.2

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The version number has been correctly updated to 1.1.2, reflecting the revert and new release.


environment:
sdk: ^3.9.0
Expand Down
Loading