Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
agektmr opened this issue Dec 20, 2016 · 10 comments
Open

Comments

@agektmr
Copy link

agektmr commented Dec 20, 2016

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
Copy link
Author

agektmr commented Dec 20, 2016

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

@blasten
Copy link

blasten commented Dec 20, 2016

cc @ebidel

@ebidel
Copy link
Contributor

ebidel commented Dec 20, 2016

@addyosmani @Zoramite are the maintainers.

@FluorescentHallucinogen

Any progress?

@Zoramite
Copy link
Contributor

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
Copy link
Contributor

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

@Zoramite, is there any ETA of next version?

@Zoramite
Copy link
Contributor

@FluorescentHallucinogen I don't have an ETA currently.

@agektmr
Copy link
Author

agektmr commented Jan 16, 2017

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

@FluorescentHallucinogen

@addyosmani @Zoramite Any progress?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants