GoogleWebComponents / google-signin

Google Sign-in web component
https://elements.polymer-project.org/elements/google-signin
Other
279 stars 89 forks source link

[Major change proposal] Decoupling `google-signin` and `google-signin-aware` #156

Open agektmr opened 7 years ago

agektmr commented 7 years ago

Hi,

I use Google Sign-In in my implementation of Polymer shop which I'm requesting a pull. Though I was suggested to use this google-signin component, I'm reluctant to do so for some reasons:

  1. google-signin contains both view and logic (google-signin-aware). While I agree it's intuitive to use a single component to show a Google Sign-In button, there are use cases where only logic is needed.
  2. google-signin-aware can handle multiple of itself. It's natural consequence if you assume to put multiple Google Sign-In buttons in a single page, but why is it needed? I only see it as an effort to resolve contradiction when putting multiple google-signin buttons.
  3. There is an event for notifying successful authorization, but no events for authentication exists.
  4. Though auth status changes can be propagated through events, there is no way to associate a user action to a successful sign-in with resulting id_token being passed to the invoker.

Here's my proposal:

  1. Make AuthEngine as google-signin-aware itself and add iron-meta as a behavior so google-signin components can refer to a single google-signin-aware in a page. (This means implementer needs to put both google-signin-aware and google-signin in the same page.)
  2. google-signin-aware-success event means "authorization" succeeded. Make it google-signin-aware-authz-success and add google-signin-aware-authn-success to indicate "authentication" success.
  3. Return a promise on signIn() invocation. That way resolving function can receive result object and pass id_token or auth code to the server.

Let me know what you think.

agektmr commented 7 years ago

This is a proof of concept implementation: https://github.com/agektmr/google-signin/tree/v2

blasten commented 7 years ago

cc @ebidel

ebidel commented 7 years ago

@addyosmani @Zoramite are the maintainers.

FluorescentHallucinogen commented 7 years ago

Any progress?

Zoramite commented 7 years ago

The idea behind allowing multiple google-signin-aware elements in a single page is to allow a separation of concern between different components. If you are writing a component that requires a specific scope you can just add the scope using the google-signin-aware component and not worry about the UI.

The google-signin-aware doesn't care about when or how many sign in buttons are on the page which is by design. The page need to be able to handle however many buttons exists regardless. You might have one sign in button in the header and one in the footer and when someone tries to use something that they need extra permissions for then a modal might also have a google-signin button.

The events can definitely be improved. It has been a while since I have worked directly on this project, but if you have some idea for better events I'd be happy to look over some pull requests.

Zoramite commented 7 years ago

I just finished looking through the changes that you were using as your concept. There are some changes that we'll probably make later when we do the next version. I may have a project coming up soon that I can work on updating the signin components at the same time.

FluorescentHallucinogen commented 7 years ago

@Zoramite, is there any ETA of next version?

Zoramite commented 7 years ago

@FluorescentHallucinogen I don't have an ETA currently.

agektmr commented 7 years ago

@Zoramite sounds good. Let me know if there's anything I can be help of.

FluorescentHallucinogen commented 6 years ago

@addyosmani @Zoramite Any progress?