-
Notifications
You must be signed in to change notification settings - Fork 130
Corrections in docs, auth and database subscription bug fixes for iOS/Android #284
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
Open
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
A couple mistakes in the docs that I tracked down in terms of keys and values for credentials.
Changed from facebook and google to facebook.com and google.com respectively. This is needed for cross platform apps. These strings are now consistent with the [iOS](https://github.com/fullstackreact/react-native-firestack/blob/v3/ios/Firestack/FirestackAuth.m#L444) and [Web](https://firebase.google.com/docs/reference/js/firebase.auth.AuthCredential) code. Docs should be updated too. [Here's my pull request](https://github.com/fullstackreact/react-native-firestack/pull/284/files) for that.
Solution discovered by @TheNarie in [this issue](fullstackreact#282) except I also corrected `accessToken` from the original suggestion. Docs also updated about that in same pull request.
@abcd-ca
abcd-ca
changed the title
(削除) Corrections in docs (削除ここまで)
(追記) Corrections in docs and bug fixes (追記ここまで)
Feb 13, 2017
The iOS and Android, `signInWithProvider` have different signatures. This works around that difference but ideally the native signatures should match each other (and the Web sdk's). Tested on iOS and Android.
needed for previous commit.
One of the needed parameters was named incorrectly – on Android this resulted in no data being retrieved. This fixes it.
created issue to reference this PR about ref.on('value'...) bug fix
@abcd-ca
abcd-ca
changed the title
(削除) Corrections in docs and bug fixes (削除ここまで)
(追記) Corrections in docs, auth and database subscription bug fixes for iOS/Android (追記ここまで)
Feb 13, 2017
v1kku
v1kku
reviewed
Feb 16, 2017
lib/modules/auth/index.js
Outdated
@@ -151,7 +151,12 @@ export default class Auth extends Base {
* @return {Promise} A promise resolved upon completion
*/
signInWithCredential(credential: any): Promise<Object> {
return promisify('signInWithProvider', FirestackAuth)(credential.provider, credential.accessToken, credential.secret);
signInWithCredential(credential: any): Promise<Object> {
@v1kku
v1kku
Feb 16, 2017
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@abcd-ca This line is duplicated.
Good catch, I fixed the duplicate line.
... On Feb 16, 2017, at 5:35 AM, Victor Munene ***@***.***> wrote:
@v1kku commented on this pull request.
In lib/modules/auth/index.js <#284 (comment)>:
> @@ -151,7 +151,12 @@ export default class Auth extends Base {
* @return {Promise} A promise resolved upon completion
*/
signInWithCredential(credential: any): Promise<Object> {
- return promisify('signInWithProvider', FirestackAuth)(credential.provider, credential.accessToken, credential.secret);
+ signInWithCredential(credential: any): Promise<Object> {
@abcd-ca <https://github.com/abcd-ca> This line is duplicated.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub <#284 (review)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABPG5ysoPvPP0iQYRn6lsYVN6YEMlWUTks5rdFCtgaJpZM4L7SE_>.
alexkendall
commented
Mar 2, 2017
+1
1 similar comment
JackThomson2
commented
Mar 18, 2017
+1
@abcd-ca @alexkendall @JackThomson2 this repo has generally been abandoned, I think this issue and a ton of other issues were fixed in my v3 version of firestack.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
A couple mistakes in the docs that I tracked down in terms of keys and values for credentials. I have facebook auth working in my app now as a result of these changes (I've verified)