-
Notifications
You must be signed in to change notification settings - Fork 61
DOCSP-4883 Document OAuth2 login with React Native #253
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
Conversation
| // import {Stitch, GoogleCredential} from 'mongodb-stitch-react-native-sdk' | ||
| // import {GoogleSignin, GoogleSigninButton} from 'react-native-google-signin' | ||
| class GoogleLogin extends React.Component { | ||
| // ... |
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.
nit: maybe we should be more explicit here?
e.g. ... other component methods
nlarew
left a comment
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.
This is a great addition! Seems like it'll make it way easier to get up and running with RN.
Left a few small suggestions, mainly around beefing up some of the code comments, but otherwise LGTM
| componentDidMount() { | ||
| // Configure react-native-google-signin | ||
| GoogleSignin.configure({ | ||
| webClientId: '<id>', // client ID of type WEB from Stitch |
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.
should this be "from Stitch"? In the SO post you mention that it's the Client ID value from the Stitch UI. Since we don't have a screenshot handy here, maybe we should say that it's the client ID that you create as part of google project setup?
| // Configure react-native-google-signin | ||
| GoogleSignin.configure({ | ||
| webClientId: '<id>', // client ID of type WEB from Stitch | ||
| offlineAccess: true, // allows Stitch service to use credential |
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.
I think I know what you mean here but I'm not sure. Is it something like: Must be `true` for the GoogleCredential to be valid
| // Retrieve the server auth code | ||
| const {serverAuthCode} = userInfo | ||
| if (serverAuthCode === null) { | ||
| throw new Error('Failed to get serverAuthCode!') |
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.
Does serverAuthCode === null imply that the login failed or was cancelled? Could add a comment clarifying the case that would cause the error.
| @@ -0,0 +1,98 @@ | |||
| function example() {} function untested({View, Text, Button, React, GoogleSignin, Stitch, GoogleCredential, GoogleSigninButton}) { // too much boilerplate to be able to test this | |||
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.
I get the reasoning for this change, but it seems just a bit incomplete to not include the anonymous and email/password (any maybe others) login snippets here too.
Maybe later down the road we can look into some kind of tabbed experience in the SDK docs to pick the credential you want an example for so that we can include them here as well as in StitchAuth.
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.
Don't worry, those snippets are still available elsewhere in the docs!
| * Typically, the [[Stitch.initializeDefaultAppClient]] method is all you need | ||
| * to instantiate the client: | ||
| * to instantiate the client (note that, unlike the other JS SDKs, this method returns | ||
| * a Promise): |
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.
nit: maybe make it a bit more explicit which SDK isn't included in "the other JS SDKs"
"unlike in the other JS SDKs, this method returns a Promise in the React Native SDK"?
dkaminsky
left a comment
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.
lgtm
This adds an example of how to use a 3rd-party module to work with Google auth and Stitch. Calls out differences between working with Browser SDK (RedirectCredential) and server/RN. Attempt to explain the general idea of how to work with OAuth2 auth providers.
6a6864f to
7028b35
Compare
This adds an example of how to use a 3rd-party module to
work with Google auth and Stitch.
Calls out differences between working with Browser SDK (RedirectCredential)
and server/RN.
Attempt to explain the general idea of how to work with OAuth2 auth
providers.
Staged:
https://docs-mongodbcom-staging.corp.mongodb.com/stitch/bush/docsp-4883-rn-google-login/sdk/js-server/index.html
https://docs-mongodbcom-staging.corp.mongodb.com/stitch/bush/docsp-4883-rn-google-login/sdk/react-native/index.html