FirebaseExtended / polymerfire

Polymer Web Components for Firebase
https://www.webcomponents.org/element/firebase/polymerfire
MIT License
459 stars 142 forks source link

Add more properties and providers to <firebase-auth> #251

Open FluorescentHallucinogen opened 7 years ago

FluorescentHallucinogen commented 7 years ago

I have 2 ideas (suggestions):

Idea 1

Currently, provider property of <firebase-auth> element is used only for OAuth. Current accepted values are google, facebook, github, twitter.

provider property can remain undefined when attempting to sign in anonymously or using email and password or using phone number.

What about extending the list of accepted values for provider property by adding non-OAuth providers such as anonymous, email and phone for consistency purposes? Something like:

<firebase-auth
  provider="email"
  user="{{user}}">
</firebase-auth>
<firebase-auth
  provider="phone"
  user="{{user}}">
</firebase-auth>

Since <firebase-auth> element is just a wrapper of Firebase Authentication SDK, does adding support of non-OAuth providers values to provider property of <firebase-auth> require changes to the SDK?

Idea 2

createUserWithEmailAndPassword, sendPasswordResetEmail, signInWithEmailAndPassword methods of <firebase-auth> element require email (and password) parameters.

What about adding email and password properties to <firebase-auth> element?

It helps to use data binding to make specifying email and password parameters more easy. Something like:

<gold-email-input
  value="{{email}}">
</gold-email-input>

<paper-input
  type="password"
  value="{{password}}">
</paper-input>

<firebase-auth
  provider="email"
  email="[[email]]"
  password="[[password]]"
  user="{{user}}">
</firebase-auth>

Are these good ideas?

FluorescentHallucinogen commented 7 years ago

@mbleigh @tjmonsi WDYT?

FluorescentHallucinogen commented 7 years ago

This idea 1 is not good, because signInWithPopup and signInWithRedirect methods in PolymerFire are just wrappers of the same functions in Firebase JS SDK API. The provider argument in signInWithPopup and signInWithRedirect methods must be an OAuth provider. Non-OAuth providers like firebase.auth.EmailAuthProvider and firebase.auth.PhoneAuthProvider will throw an error.

The idea 2, IMO, is good, because it allows to declaratively set using properties and dynamically change using data binding the email, password, token and credential, and then just call the corresponding methods like signInWithEmailAndPassword, createUserWithEmailAndPassword, sendPasswordResetEmail, signInWithCustomToken and signInWithCredential without any arguments.

See the implementation of idea 2 in #265.

tjmonsi commented 6 years ago

Looking at it quickly, although it's a good helper, but it definitely changes how signInWithEmailAndPassword and others using email and password behaves and diverges from the firebase docs on signInWithEmailAndPassword.

Following the firebase docs I think should still be the primary driver of adding features here. We could add helper tags though that wraps around the original spec of firebase-auth (which follows the fireabse.auth docs).

FluorescentHallucinogen commented 6 years ago

@tjmonsi

See my previous comment. I abandoned the idea 1 of extending the list of accepted values for provider property by adding non-OAuth providers.

The idea 2 is just to add 4 properties to <firebase-auth> Polymer element. This is not a breaking change and this does not change how signInWithEmailAndPassword and others methods behaves. PTAL at #265.

tjmonsi commented 6 years ago

Yeah I see that, but it still allows behaviors from people to not put email and password as essential parameters. Although that's also the idea of the provider attribute, which is a shortcut for signInWithPopup and redirect. I will consider this though and will review the code this weekend. Thanks :)

FluorescentHallucinogen commented 6 years ago

@tjmonsi

but it still allows behaviors from people to not put email and password as essential parameters

But please note that in PolymerFire users can to not put provider as required parameter in signInWithPopup and signInWithRedirect methods (users can set it using the <firebase-auth>'s property), while in Firebase JS SDK API provider is a required argument (value must not be null) for signInWithPopup and signInWithRedirect methods.

My patch (#265) just allows the same behavior (set email, password, token and credential using the <firebase-auth>'s properties) for signInWithEmailAndPassword, createUserWithEmailAndPassword, sendPasswordResetEmail, signInWithCustomToken and signInWithCredential methods.