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

Add phone number sign-in #250

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

FluorescentHallucinogen
Copy link
Contributor

Fix #228.

@FluorescentHallucinogen
Copy link
Contributor Author

Thanks to @AVStarikovich! 😉

@FluorescentHallucinogen
Copy link
Contributor Author

@mbleigh @tjmonsi Please review this pull request.

@FluorescentHallucinogen
Copy link
Contributor Author

Please note that reCAPTCHA doesn't work inside Shadow DOM.

@FluorescentHallucinogen
Copy link
Contributor Author

@mbleigh @tjmonsi PTAL.

Copy link
Collaborator

@tjmonsi tjmonsi left a comment

Choose a reason for hiding this comment

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

This can be problematic. It means it needs something that has an id of a recaptcha-container.

You can change this with saying...

signInWithPhoneNumber: function(phoneNumber, verifier) {
   var appVerifier
   if (verifier instanceof HTMLElement) {
      appVerifier = verifier
   } else if (typeof verifier === 'string') {
      appVerifier = this.shadowRoot.querySelector('#recaptcha-container')
   }

   if (!appVerifier) return withError()

@FluorescentHallucinogen
Copy link
Contributor Author

@tjmonsi Done. Only two questions:

  1. Is it a good idea to use the window object to pass confirmationResult from signInWithPhoneNumber function to confirmCode function? If not, is it possible to somehow avoid this at all?

  2. What about moving creation of firebase.auth.RecaptchaVerifier from signInWithPhoneNumber function to another separate function e.g. createRecaptchaVerifier? Something like this:

createRecaptchaVerifier: function(container, parameters, app) {
  return new firebase.auth.RecaptchaVerifier(container, parameters, app);
},

signInWithPhoneNumber: function(phoneNumber, applicationVerifier) {
  return this._handleSignIn(this.auth.signInWithPhoneNumber(phoneNumber, applicationVerifier)
    .then(function(confirmationResult) {
      window.confirmationResult = confirmationResult;
    }));
},

On the one hand, I believe we should keep PolymerFire API as close to Firebase JavaScript SDK API as possible. In the Firebase JavaScript SDK the signInWithPhoneNumber function takes 2 arguments — phoneNumber and applicationVerifier. On the other hand, this means that the user needs somewhere (outside the signInWithPhoneNumber function) to create the applicationVerifier to pass it to signInWithPhoneNumber function.

@tjmonsi
Copy link
Collaborator

tjmonsi commented Sep 8, 2017

On the one hand, I believe we should keep PolymerFire API as close to Firebase JavaScript SDK API as possible. In the Firebase JavaScript SDK the signInWithPhoneNumber function takes 2 arguments — phoneNumber and applicationVerifier. On the other hand, this means that the user needs somewhere (outside the signInWithPhoneNumber function) to create the applicationVerifier to pass it to signInWithPhoneNumber function.

You are right on point here. It will really need the user to create an application verifier.
I haven't tried yet the phone number flow but I'll look into it this weekend with my weekend pass on polymerfire issues and will get back to you on it

@FluorescentHallucinogen
Copy link
Contributor Author

@tjmonsi The weekend has passed. Any news?

@tjmonsi
Copy link
Collaborator

tjmonsi commented Sep 11, 2017

I am stuck. Was working on the devfest site the whole weekend so that I can make a release today. Sorry If I have to ask for an extension :(

@FluorescentHallucinogen
Copy link
Contributor Author

@tjmonsi Any progress?

@tjmonsi
Copy link
Collaborator

tjmonsi commented Sep 21, 2017

I'm doing the issues and requests one by one :( I'll get into this tomorrow. Just got time to be free now

@JGSolutions
Copy link

JGSolutions commented Nov 2, 2017

Any progress? Will this feature get released anytime soon?

@merlinnot
Copy link
Contributor

Guys, don’t you think that we should discuss what we really want to accomplish here before reviewing the code itself? I’m asking because I’ve implemented phone auth myself in a Polymer app and I took completely different approach. Most concerning thing for me in the current PR is recreating recaptcha every time the function is being called. It can be done once, i.e. in a connected callback.

@JGSolutions
Copy link

So will this get released?

@merlinnot
Copy link
Contributor

I don’t think it should be released in the current state, but I’d love to continue working on this feature. As I’ve mentioned in the previous comment, it can be done cleaner. I won’t be able to make a PR with my approach until next month, I have a lot on my plate right now.

@FluorescentHallucinogen
Copy link
Contributor Author

Ping.

@FluorescentHallucinogen
Copy link
Contributor Author

@merlinnot

it can be done cleaner. I won’t be able to make a PR with my approach until next month

Any progress?

@merlinnot
Copy link
Contributor

Copy-pasted from one of my projects:

constructor:

this._shadowHelper = document.createElement('div');
document.querySelector('body').appendChild(this._shadowHelper);

window.recaptchaVerifier = new firebase.auth.RecaptchaVerifier(
  this._shadowHelper,
  {
    size: 'invisible',
    callback: () => this._recaptchaReady = true,
  },
);
window.recaptchaWidgetId = await window.recaptchaVerifier.render();

Then you can use:

grecaptcha.reset(window.recaptchaWidgetId);

or

firebase.auth().signInWithPhoneNumber(number, window.recaptchaVerifier);

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

Successfully merging this pull request may close these issues.

4 participants