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

Broadcast initilized event #121

Closed
filipproch opened this issue Feb 1, 2016 · 11 comments
Closed

Broadcast initilized event #121

filipproch opened this issue Feb 1, 2016 · 11 comments

Comments

@filipproch
Copy link

It's not always best for us to observe on signedIn, because you are setting it initially to false before it is really determined. In my application, I display loading screen which should wait to detect whether user is authenticated a then display screen accordingly, which is by observing signedIn not easy to handle. I suggest adding event which is emmited in handleUserUpdate if it is the first time this method is called, after changing the signedIn property so we can handle above case properly.

@soonick
Copy link

soonick commented Jan 16, 2017

Hello @atotic, @Zoramite,

I just started trying to use this component and found myself having the same problem as @filipproch. I see that there has been a pull request that can solve the issue for a couple months but no comments have been made on it. Do you think you can help us look at the pull request and let us know how to proceed?

@atotic
Copy link
Contributor

atotic commented Jan 17, 2017

Apologies for not looking at this, I am no longer working on google-signin, so I can't help you. @ebidel ?

I understand your problem, you want to know when signedIn value has been initialized to its stable state.
The problem is caused by signedIn being a boolean, while it really can be in 3 states: unknown, signedIn, or signedOut.
The proposed patch adds another property: initialized.
Personally, I'd prefer just to make signedIn a Number, representing three states,

But that is a compatibility-breaking change. Maybe add a signedInVerbose property that is tri-state? 1 for signedIn, 0 for signedOut, -1 for uninitialized, and point new API users towards using it instead of signedIn?

@filipproch
Copy link
Author

The usage of another property approach is also used in Polymerfire library, where you have "statusKnown" property. (https://github.com/firebase/polymerfire/blob/master/firebase-auth.html)

As it would be breaking changes to change the type of signedIn, I personally think it would be better to add another property than to introduce another confusing property and thus having 'signedIn' and 'signedInVerbose'.

Anyway, any solution to this problem would be appreciated

@soonick
Copy link

soonick commented Jan 19, 2017

@ebidel can you help us with this?

@soonick
Copy link

soonick commented Jan 24, 2017

@ebidel sorry for pinging you again, but I would really like to use this component and I can't use it without this feature. Can you help us?

@filipproch
Copy link
Author

@soonick Then fork it, modify it and use your modified component, you can also send a pull request if you think it would be appropriate

@soonick
Copy link

soonick commented Jan 24, 2017

@filipproch. I want to leave forking it as a last resort. We already have a pull request: #153

@filipproch
Copy link
Author

@soonick I know, just saying you don't have to wait for @ebidel to merge the pull request, if you really need it, that's what open source gives you

@atotic
Copy link
Contributor

atotic commented Jan 24, 2017

Change has been merged.

@atotic atotic closed this as completed Jan 24, 2017
@soonick
Copy link

soonick commented Jan 25, 2017

Thank you :)

@atotic
Copy link
Contributor

atotic commented Jan 25, 2017

👍

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

No branches or pull requests

3 participants