Skip to content
This repository has been archived by the owner on Feb 22, 2023. It is now read-only.

[google_sign_in] Migrate to nnbd #3329

Merged
merged 7 commits into from
Jan 15, 2021
Merged

[google_sign_in] Migrate to nnbd #3329

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
cca5e58
[google_sign_in] Migrate to nnbd
mehmetf Dec 14, 2020
df46cab
Update quiver dep
mehmetf Dec 17, 2020
9b52a79
Review comments
mehmetf Dec 19, 2020
f0065a0
Fix wrong import
mehmetf Jan 9, 2021
105a6df
Update mockito version
mehmetf Jan 13, 2021
91ff05c
Fix failures
mehmetf Jan 15, 2021
c76338c
Wrong place for language version
mehmetf Jan 15, 2021
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
4 changes: 4 additions & 0 deletions packages/google_sign_in/google_sign_in/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,7 @@
## 5.0.0-nullsafety

* Migrate to nnbd.

## 4.5.9

* Update the example app: remove the deprecated `RaisedButton` and `FlatButton` widgets.
Expand Down
2 changes: 2 additions & 0 deletions packages/google_sign_in/google_sign_in/integration_test/google_sign_in_test.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
// @dart = 2.9

import 'package:integration_test/integration_test.dart';
import 'package:flutter_test/flutter_test.dart';
import 'package:google_sign_in/google_sign_in.dart';
Expand Down
56 changes: 28 additions & 28 deletions packages/google_sign_in/google_sign_in/lib/google_sign_in.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,13 +22,13 @@ class GoogleSignInAuthentication {
final GoogleSignInTokenData _data;

/// An OpenID Connect ID token that identifies the user.
String get idToken => _data.idToken;
String? get idToken => _data.idToken;

/// The OAuth2 access token to access Google services.
String get accessToken => _data.accessToken;
String? get accessToken => _data.accessToken;

/// Server auth code used to access Google Login
String get serverAuthCode => _data.serverAuthCode;
String? get serverAuthCode => _data.serverAuthCode;

@override
String toString() => 'GoogleSignInAuthentication:$_data';
Expand Down Expand Up @@ -57,7 +57,7 @@ class GoogleSignInAccount implements GoogleIdentity {
static const String kUserRecoverableAuthError = 'user_recoverable_auth';

@override
final String displayName;
final String? displayName;

@override
final String email;
Expand All @@ -66,9 +66,9 @@ class GoogleSignInAccount implements GoogleIdentity {
final String id;

@override
final String photoUrl;
final String? photoUrl;

final String _idToken;
final String? _idToken;
final GoogleSignIn _googleSignIn;

/// Retrieve [GoogleSignInAuthentication] for this account.
Expand Down Expand Up @@ -105,7 +105,7 @@ class GoogleSignInAccount implements GoogleIdentity {
///
/// See also https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Authorization.
Future<Map<String, String>> get authHeaders async {
final String token = (await authentication).accessToken;
final String? token = (await authentication).accessToken;
return <String, String>{
"Authorization": "Bearer $token",
"X-Goog-AuthUser": "0",
Expand All @@ -117,7 +117,7 @@ class GoogleSignInAccount implements GoogleIdentity {
/// If client runs into 401 errors using a token, it is expected to call
/// this method and grab `authHeaders` once again.
Future<void> clearAuthCache() async {
final String token = (await authentication).accessToken;
final String token = (await authentication).accessToken!;
await GoogleSignInPlatform.instance.clearAuthCache(token: token);
}

Expand Down Expand Up @@ -174,7 +174,7 @@ class GoogleSignIn {
/// Factory for creating default sign in user experience.
factory GoogleSignIn.standard({
List<String> scopes = const <String>[],
String hostedDomain,
String? hostedDomain,
}) {
return GoogleSignIn(
signInOption: SignInOption.standard,
Expand Down Expand Up @@ -212,22 +212,22 @@ class GoogleSignIn {
final List<String> scopes;

/// Domain to restrict sign-in to.
final String hostedDomain;
final String? hostedDomain;

/// Client ID being used to connect to google sign-in. Only supported on web.
final String clientId;
final String? clientId;

StreamController<GoogleSignInAccount> _currentUserController =
StreamController<GoogleSignInAccount>.broadcast();
StreamController<GoogleSignInAccount?> _currentUserController =
StreamController<GoogleSignInAccount?>.broadcast();

/// Subscribe to this stream to be notified when the current user changes.
Stream<GoogleSignInAccount> get onCurrentUserChanged =>
Stream<GoogleSignInAccount?> get onCurrentUserChanged =>
_currentUserController.stream;

// Future that completes when we've finished calling `init` on the native side
Future<void> _initialization;
Future<void>? _initialization;

Future<GoogleSignInAccount> _callMethod(Function method) async {
Future<GoogleSignInAccount?> _callMethod(Function method) async {
await _ensureInitialized();

final dynamic response = await method();
Expand All @@ -237,7 +237,7 @@ class GoogleSignIn {
: null);
}

GoogleSignInAccount _setCurrentUser(GoogleSignInAccount currentUser) {
GoogleSignInAccount? _setCurrentUser(GoogleSignInAccount? currentUser) {
if (currentUser != _currentUser) {
_currentUser = currentUser;
_currentUserController.add(_currentUser);
Expand All @@ -258,7 +258,7 @@ class GoogleSignIn {
}

/// The most recently scheduled method call.
Future<void> _lastMethodCall;
Future<void>? _lastMethodCall;

/// Returns a [Future] that completes with a success after [future], whether
/// it completed with a value or an error.
Expand All @@ -279,15 +279,15 @@ class GoogleSignIn {
/// The optional, named parameter [canSkipCall] lets the plugin know that the
/// method call may be skipped, if there's already [_currentUser] information.
/// This is used from the [signIn] and [signInSilently] methods.
Future<GoogleSignInAccount> _addMethodCall(
Future<GoogleSignInAccount?> _addMethodCall(
Function method, {
bool canSkipCall = false,
}) async {
Future<GoogleSignInAccount> response;
Future<GoogleSignInAccount?> response;
if (_lastMethodCall == null) {
response = _callMethod(method);
} else {
response = _lastMethodCall.then((_) {
response = _lastMethodCall!.then((_) {
Copy link

Choose a reason for hiding this comment

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

is the analyzer able to detect the _lastMethodCall == null check earlier?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am a bit annoyed by this as well but I have seen it in many other places. My interpretation is that flow analysis only catches variables that are local to current scope. For instance:

void main() {
  String? foo;
  if (foo == null) {
    print('is null');
  } else {
    print(foo.length);
  }
}

is fine. However:

String? foo;

void main() {
  if (foo == null) {
    print('is null');
  } else {
    print(foo.length);
  }
}

is not. You need the bang operator.

Copy link

Choose a reason for hiding this comment

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

I see, thanks. I learned something new. I wonder if @munificent has any thoughts about whether this is the intended design or a temporary limitation

Copy link

Choose a reason for hiding this comment

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

It's an intended design but a known pain point that we'd like to improve if we can figure out how. The problem is that promoting fields isn't sound. In many cases, the compiler can't prove that there is not a class elsewhere that implements that class's interface and overrides with the field with a getter. Or there could be some re-entrant method call on the instance that changes the field's value between when it is checked for null and when it is later used.

Copy link

Choose a reason for hiding this comment

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

ah. that makes sense. The re-rentrant method call is interesting. Could that occur even if the method that is checking for null isn't async?

Copy link

Choose a reason for hiding this comment

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

Yeah. "Re-entrant" is maybe not the best word, but just consider any case between checking a field for null and accessing it where you could end up running other code on the same instance. This is contrived, but should give you the idea:

class Foo {
  int? field;

  method(Foo someFoo) {
    if (field != null) {
      someFoo.other();
      print(field + 1);
    }
  }

  other() {
    field = null;
  }
}

main() {
  var foo = Foo();
  foo.method(foo);
}

// If after the last completed call `currentUser` is not `null` and requested
// method can be skipped (`canSkipCall`), re-use the same authenticated user
// instead of making extra call to the native side.
Expand All @@ -303,8 +303,8 @@ class GoogleSignIn {
}

/// The currently signed in account, or null if the user is signed out.
GoogleSignInAccount get currentUser => _currentUser;
GoogleSignInAccount _currentUser;
GoogleSignInAccount? get currentUser => _currentUser;
GoogleSignInAccount? _currentUser;

/// Attempts to sign in a previously authenticated user without interaction.
///
Expand All @@ -323,7 +323,7 @@ class GoogleSignIn {
/// one of [kSignInRequiredError] (when there is no authenticated user) ,
/// [kNetworkError] (when a network error occurred) or [kSignInFailedError]
/// (when an unknown error occurred).
Future<GoogleSignInAccount> signInSilently({
Future<GoogleSignInAccount?> signInSilently({
bool suppressErrors = true,
}) async {
try {
Expand Down Expand Up @@ -354,21 +354,21 @@ class GoogleSignIn {
/// a Future which resolves to the same user instance.
///
/// Re-authentication can be triggered only after [signOut] or [disconnect].
Future<GoogleSignInAccount> signIn() {
final Future<GoogleSignInAccount> result =
Future<GoogleSignInAccount?> signIn() {
final Future<GoogleSignInAccount?> result =
_addMethodCall(GoogleSignInPlatform.instance.signIn, canSkipCall: true);
bool isCanceled(dynamic error) =>
error is PlatformException && error.code == kSignInCanceledError;
return result.catchError((dynamic _) => null, test: isCanceled);
}

/// Marks current user as being in the signed out state.
Future<GoogleSignInAccount> signOut() =>
Future<GoogleSignInAccount?> signOut() =>
_addMethodCall(GoogleSignInPlatform.instance.signOut);

/// Disconnects the current user from the app and revokes previous
/// authentication.
Future<GoogleSignInAccount> disconnect() =>
Future<GoogleSignInAccount?> disconnect() =>
_addMethodCall(GoogleSignInPlatform.instance.disconnect);

/// Requests the user grants additional Oauth [scopes].
Expand Down
4 changes: 2 additions & 2 deletions packages/google_sign_in/google_sign_in/lib/src/common.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,10 +28,10 @@ abstract class GoogleIdentity {
/// The display name of the signed in user.
///
/// Not guaranteed to be present for all users, even when configured.
String get displayName;
String? get displayName;

/// The photo url of the signed in user if the user has a profile picture.
///
/// Not guaranteed to be present for all users, even when configured.
String get photoUrl;
String? get photoUrl;
}
18 changes: 9 additions & 9 deletions packages/google_sign_in/google_sign_in/lib/testing.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ class FakeSignInBackend {
/// This does not represent the signed-in user, but rather an object that will
/// be returned when [GoogleSignIn.signIn] or [GoogleSignIn.signInSilently] is
/// called.
FakeUser user;
late FakeUser user;

/// Handles method calls that would normally be sent to the native backend.
/// Returns with the expected values based on the current [user].
Expand All @@ -42,7 +42,7 @@ class FakeSignInBackend {
// do nothing
return null;
case 'getTokens':
return <String, String>{
return <String, String?>{
'idToken': user.idToken,
'accessToken': user.accessToken,
};
Expand Down Expand Up @@ -72,24 +72,24 @@ class FakeUser {
});

/// Will be converted into [GoogleSignInUserData.id].
final String id;
final String? id;

/// Will be converted into [GoogleSignInUserData.email].
final String email;
final String? email;

/// Will be converted into [GoogleSignInUserData.displayName].
final String displayName;
final String? displayName;

/// Will be converted into [GoogleSignInUserData.photoUrl].
final String photoUrl;
final String? photoUrl;

/// Will be converted into [GoogleSignInTokenData.idToken].
final String idToken;
final String? idToken;

/// Will be converted into [GoogleSignInTokenData.accessToken].
final String accessToken;
final String? accessToken;

Map<String, String> get _asMap => <String, String>{
Map<String, String?> get _asMap => <String, String?>{
'id': id,
'email': email,
'displayName': displayName,
Expand Down
29 changes: 8 additions & 21 deletions packages/google_sign_in/google_sign_in/lib/widgets.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@

import 'dart:typed_data';

import 'package:flutter/foundation.dart';
import 'package:flutter/material.dart';

import 'src/common.dart';
Expand All @@ -23,7 +22,7 @@ class GoogleUserCircleAvatar extends StatelessWidget {
/// in place of a profile photo, or a default profile photo if the user's
/// identity does not specify a `displayName`.
const GoogleUserCircleAvatar({
@required this.identity,
required this.identity,
this.placeholderPhotoUrl,
this.foregroundColor,
this.backgroundColor,
Expand All @@ -42,13 +41,13 @@ class GoogleUserCircleAvatar extends StatelessWidget {
/// The color of the text to be displayed if photo is not available.
///
/// If a foreground color is not specified, the theme's text color is used.
final Color foregroundColor;
final Color? foregroundColor;

/// The color with which to fill the circle. Changing the background color
/// will cause the avatar to animate to the new color.
///
/// If a background color is not specified, the theme's primary color is used.
final Color backgroundColor;
final Color? backgroundColor;

/// The URL of a photo to use if the user's [identity] does not specify a
/// `photoUrl`.
Expand All @@ -57,7 +56,7 @@ class GoogleUserCircleAvatar extends StatelessWidget {
/// then this widget will attempt to display the user's first initial as
/// determined from the identity's [displayName] field. If that is `null` a
/// default (generic) Google profile photo will be displayed.
final String placeholderPhotoUrl;
final String? placeholderPhotoUrl;

@override
Widget build(BuildContext context) {
Expand All @@ -68,46 +67,34 @@ class GoogleUserCircleAvatar extends StatelessWidget {
);
}

/// Adds correct sizing information to [photoUrl].
///
/// Falls back to the default profile photo if [photoUrl] is [null].
static String _sizedProfileImageUrl(String photoUrl, double size) {
if (photoUrl == null) {
// If the user has no profile photo and no display name, fall back to
// the default profile photo as a last resort.
return 'https://lh3.googleusercontent.com/a/default-user=s${size.round()}-c';
}
return fife.addSizeDirectiveToUrl(photoUrl, size);
}

Widget _buildClippedImage(BuildContext context, BoxConstraints constraints) {
assert(constraints.maxWidth == constraints.maxHeight);

// Placeholder to use when there is no photo URL, and while the photo is
// loading. Uses the first character of the display name (if it has one),
// or the first letter of the email address if it does not.
final List<String> placeholderCharSources = <String>[
final List<String?> placeholderCharSources = <String?>[
identity.displayName,
identity.email,
'-',
];
final String placeholderChar = placeholderCharSources
.firstWhere((String str) => str != null && str.trimLeft().isNotEmpty)
.firstWhere((String? str) => str != null && str.trimLeft().isNotEmpty)!
.trimLeft()[0]
.toUpperCase();
final Widget placeholder = Center(
child: Text(placeholderChar, textAlign: TextAlign.center),
);

final String photoUrl = identity.photoUrl ?? placeholderPhotoUrl;
final String? photoUrl = identity.photoUrl ?? placeholderPhotoUrl;
if (photoUrl == null) {
return placeholder;
}

// Add a sizing directive to the profile photo URL.
final double size =
MediaQuery.of(context).devicePixelRatio * constraints.maxWidth;
final String sizedPhotoUrl = _sizedProfileImageUrl(photoUrl, size);
final String sizedPhotoUrl = fife.addSizeDirectiveToUrl(photoUrl, size);

// Fade the photo in over the top of the placeholder.
return SizedBox(
Expand Down
18 changes: 6 additions & 12 deletions packages/google_sign_in/google_sign_in/pubspec.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ name: google_sign_in
description: Flutter plugin for Google Sign-In, a secure authentication system
for signing in with a Google account on Android and iOS.
homepage: https://github.com/flutter/plugins/tree/master/packages/google_sign_in/google_sign_in
version: 4.5.9
version: 5.0.0-nullsafety

flutter:
plugin:
Expand All @@ -16,27 +16,21 @@ flutter:
default_package: google_sign_in_web

dependencies:
google_sign_in_platform_interface: ^1.1.1
google_sign_in_platform_interface: ^2.0.0-nullsafety
flutter:
sdk: flutter
meta: ^1.0.4
# The design on https://flutter.dev/go/federated-plugins was to leave
# this constraint as "any". We cannot do it right now as it fails pub publish
# validation, so we set a ^ constraint.
# TODO(amirh): Revisit this (either update this part in the design or the pub tool).
# https://github.com/flutter/flutter/issues/46264
google_sign_in_web: ^0.9.1
Copy link
Member

Choose a reason for hiding this comment

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

This pubspec is declaring a 'default_package: google_sign_in_web' for the 'web' platform (line 16), however it's removing the dependency on the web package here. I don't think this'll work for web users. This should be documented in the CHANGELOG.md entry.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We had discussed this somewhere in this PR and decided to remove the default package. I am not familiar with how that needs to be executed upstream. Could you please help with that David?

Copy link
Member

Choose a reason for hiding this comment

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

I'm not really sure if this is used on-demand or validated somehow; I think the web endorsement should be removed (otherwise we're cheating pub.dev into reporting "web" support for the null-safe version, which is not true)

Copy link
Member

Choose a reason for hiding this comment

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

Tracking here: flutter/flutter#72239

meta: ^1.3.0-nullsafety.6

dev_dependencies:
http: ^0.12.0
flutter_driver:
sdk: flutter
flutter_test:
sdk: flutter
pedantic: ^1.8.0
pedantic: ^1.10.0-nullsafety.1
integration_test:
path: ../../integration_test

environment:
sdk: ">=2.1.0 <3.0.0"
flutter: ">=1.12.13+hotfix.4"
sdk: ">=2.12.0-0 <3.0.0"
flutter: ">=1.12.13+hotfix.5"
Loading