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

Add google_sign_in_web plugin. #2280

Merged
ditman merged 6 commits into from
Nov 19, 2019
Merged

Add google_sign_in_web plugin. #2280

ditman merged 6 commits into from
Nov 19, 2019

Conversation

@ditman
Copy link
Member

@ditman ditman commented Nov 15, 2019 •
edited
Loading

Description

Adds a web implementation of the google_sign_in plugin, via JS interop with the gapi object.

The JS interop facade is auto-generated with the dart-lang/js_facade_gen tool, from the TypeScript definitions (gapi, gapi.auth2)

Related Issues

flutter/flutter#43774

Missing stuff

  • (More) Unit tests

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.

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • My PR includes unit or integration tests for all changed/updated/fixed behaviors (See Contributor Guide).
  • All existing and new tests are passing.
  • I updated/added relevant documentation (doc comments with ///).
  • The analyzer (flutter analyze) does not report any problems on my PR.
  • I read and followed the Flutter Style Guide.
  • The title of the PR starts with the name of the plugin surrounded by square brackets, e.g. [shared_preferences]
  • I updated pubspec.yaml with an appropriate new version according to the pub versioning philosophy.
  • I updated CHANGELOG.md to add a description of the change.
  • I signed the CLA.
  • I am willing to follow-up on review comments in a timely manner.

Breaking Change

Does your PR require plugin users to manually update their apps to accommodate your change?

  • Yes, this is a breaking change (please indicate a breaking change in CHANGELOG.md and increment major revision).
  • No, this is not a breaking change.

CaseyHillers
CaseyHillers reviewed Nov 15, 2019
View reviewed changes
Copy link

@CaseyHillers CaseyHillers left a comment

Choose a reason for hiding this comment

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

Thanks for doing this David! Just some quick catches.

@CaseyHillers CaseyHillers mentioned this pull request Nov 15, 2019
Merged
harryterkelsen
harryterkelsen approved these changes Nov 15, 2019
View reviewed changes
Copy link
Contributor

@harryterkelsen harryterkelsen left a comment

Choose a reason for hiding this comment

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

LGTM!

@SaadArdati
Copy link

SaadArdati commented Nov 15, 2019

I was literally looking for this minutes ago and then found this pull request by chance, Thank you SO much for this work and I hope it gets merged and updated very soon!

@ditman
Copy link
Member Author

ditman commented Nov 15, 2019

Me gustaría ayudarte y aprender sobre todo esto

@jr942 a pull request is not the best place to have a personal conversation. If you want to help with the project, you can start taking a look at the documentation: Contributing to Flutter

@jr942 una pull request no es el mejor sitio para tener una conversación personal. Si quieres ayudar al proyecto, puedes empezar echando un ojo a la documentación: Contributing to Flutter

@ditman ditman force-pushed the federated_google_sign_in_web branch from f33c04a to bb4fd7d Compare November 16, 2019 02:11
collinjackson
collinjackson reviewed Nov 18, 2019
View reviewed changes
Copy link
Contributor

@collinjackson collinjackson left a comment

Choose a reason for hiding this comment

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

lgtm with some minor testing nits

packages/google_sign_in/google_sign_in_web/lib/google_sign_in_web.dart
' or pass clientId when calling init()');

assert(
!scopes.any((String scope) => scope.contains(' ')),
Copy link
Contributor

@collinjackson collinjackson Nov 18, 2019

Choose a reason for hiding this comment

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

Maybe this assertion should happen in the app-facing package? Aren't spaces a problem in the mobile version of the plugin as well?

Copy link
Member Author

@ditman ditman Nov 19, 2019

Choose a reason for hiding this comment

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

They're a problem here because we use the space as a "join" character to pass the scopes to JS. In the mobile versions, the List is preserved all the way through.

I'm guessing a scope name with a space character is invalid there as well (scopes are URLs, after all), but I couldn't find any actual validation performed on the Android or IOS side. It will just probably... not work, or you'll get a bad request error or similar when authenticating.

This was added here to be helpful with the user.

Copy link
Member Author

@ditman ditman Nov 19, 2019

Choose a reason for hiding this comment

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

(No validation on the app facing interface either)

@ditman ditman force-pushed the federated_google_sign_in_web branch from d5d5b60 to c3b12da Compare November 19, 2019 00:49
@ditman ditman merged commit dd9eb8b into flutter:master Nov 19, 2019
@ditman ditman deleted the federated_google_sign_in_web branch November 19, 2019 19:39
@arctouch-felipefontes
Copy link

arctouch-felipefontes commented Nov 20, 2019

Do you guys know when it will be available in pub?

@CaseyHillers
Copy link

CaseyHillers commented Nov 20, 2019

Do you guys know when it will be available in pub?

@arctouch-felipefontes it's available in pub already at https://pub.dev/packages/google_sign_in_web

@arctouch-felipefontes
Copy link

arctouch-felipefontes commented Nov 20, 2019

Hey i'm trying to use it. and i got this error after the login. and signin never returns.

Screen Shot 2019-11-20 at 14 16 50

GoogleSignIn _googleSignIn = GoogleSignIn(
      scopes: ['openid', 'email', 'profile'], hostedDomain: 'arctouch.com');

var googleUser = await _googleSignIn.signIn();

Where exactly should i open the issue here?
https://github.com/flutter/flutter/issues

@ditman
Copy link
Member Author

ditman commented Nov 20, 2019

@arctouch-felipefontes go here, click "Get Started" on the "I have a problem with my Flutter application." section, fill in as much detail as you can on the issue, and then add the platform-web and p: google_sign_in labels (and other labels you may consider relevant, of course).

sungmin-park pushed a commit to sungmin-park/flutter-plugins that referenced this pull request Dec 17, 2019
@ditman
Add google_sign_in_web (0.8.0) plugin. (flutter#2280)
a14476f 
* Look at the README.md of this package for usage instructions. Please file issues for platform: web so we see them!
@ditman ditman mentioned this pull request May 22, 2025
Merged
11 tasks
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@cyanglaz cyanglaz Awaiting requested review from cyanglaz

3 more reviewers

@collinjackson collinjackson collinjackson left review comments

@CaseyHillers CaseyHillers CaseyHillers left review comments

@harryterkelsen harryterkelsen harryterkelsen approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

cla: yes

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

8 participants

@ditman @SaadArdati @arctouch-felipefontes @CaseyHillers @collinjackson @harryterkelsen @jr942 @googlebot