Retrieving verificationId and code from credential object

[manuelsc]

See discussion @ #756

This basically retrieves the verificationId and code from the onVerificationCompleted credential. Meaning we can use verificationId and code to finally solve the instant verification issue.

var code = credential.instantVerification ? credential.code : inputField.value.toString() ;
If instantVerification is true we get the code directly from firebase without the need to go through an sms process. Otherwise use the sms code entered by the user.

• API doc needs to be updated accordingly
• It does work with the single line above and retrieving a valid credential. But I'm not a javascript developer, there might be an even better way to use this to create your own javascript credential object and skip the "firebase.auth.PhoneAuthProvider.credential" part altogether and directly call signInWithCredential. Feedback appreciated

[briantq]

Thanks for taking the time to submit a pull request! I left some comments in the proposed code. Do we need the same changes on iOS?

[manuelsc]

@briantq iOS changes are not needed, instant verification only works on Android according to the Firebase API: https://firebase.google.com/docs/auth/ios/phone-auth

[briantq]

@manuelsc There are definitely half a dozen ways to do thing. I personally would look for the known list of fields, verify they exist, then get their value rather than trying to guess on the length of their values. I think trying to rely on value length could be more brittle than field names. That being said, if you think that is a better solution you put the effort into creating the PR so I don't mind approving.

[manuelsc]

@briantq I mean it's tricky and I understand your concern. But I think relying on the length is somewhat more reliable than relying on an obfuscated name. Those names could change with any release while using the size is constant. Otherwise we could find ourself in updating the obfuscated names after every firebase update. Sure length is not an absolute viable option either but we would only need to change this if other string fields get included that share a similar length. That's far more unlikely than the obfuscated names being changed.

[briantq]

@manuelsc to start with, I want to make sure to reiterate, I am ok merging with the implementation.

For the acedemic discussion, I think the obfucsated name is better because

1. Performance (grab 2 fields instead of all, especially since Reflection is notoriously slow). Very minor I admit, I have been known to over optimized code.
2. The keys are likely stored in their source code. If they update the keys, they will need to update their client code otherwise they risk break existing clients. I think it is unlikely these keys would change, but they could.
3. New fields are added without warning which could cause problems
   3a. If you encounter issues, you wil get unpredicable behavior. Depending on how the keys are ordered, you might get random behavior where it works some times and not other. And when it doesn't work, it will appear like it is working since the method is returning a value. Alternatively, if the field changes the code will break in an expected way that should be evident within minutes of debugging.

In the end, I think changes will be rare which is why I am ok with the implementation. The big advantage to me is 3a.

[soumak77]

@manuelsc please update the API docs to reflect these changes. Once the docs are updated I will release a new version.