-
Notifications
You must be signed in to change notification settings - Fork 210
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
Hporutiu update node oauth examples #185
Conversation
horeaporutiu
commented
Sep 5, 2023
- update README to ensure user will add in ngrok forwarding address to redirect URL, otherwise the app doesn't work
- add demo video
- use template for README
- fix links for the base template
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
examples/node-oauth/README.md
Outdated
boardId=<MIRO_BOARD_ID> | ||
clientID=12345678910 | ||
clientSecret=12345678910abcdefg | ||
redirectURL=https://01-11-11-2011-74.ngrok-free.app |
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 localhost?
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.
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.
When I originally created this app, I used ngrok, but maybe that was before localhost was supported.
IMO it's good to have at least one example that uses something other than localhost, but don't have a super strong opinion :)
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've just tested this and it works fine on both localhost and with ngrok.
I think generally it's best to have the most frictionless path up front (localhost) and then we can also have a separate guide for ngrok since it takes a bit more work to setup.
I can work on this and just add in one more commit later today.
I wonder if it's worth having a separate name for a sample app which is using a "hosted" solution so that it can easily be identified from the other 20+ sample apps we have. Otherwise I'm not quite sure how a developer would know that the node-oauth sample is the one which has a guide for ngrok / non-localhost redirect URL.
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.
That sounds good to me @horeaporutiu . I agree localhost is easier.
We may not even need to have an ngrok example, but could add an item to our backlog to create a guide around it in the future if we think it's valuable
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.
Since I already have the demo videos and instructions for ngrok, I will just add it as an optional step in the node-oauth guide if that works for you both?
And then later on we can decide if we want to have a separate sample for this or if we just want to leave it as is. If a developer asks about using a hosted URL we can always point to this optional part of the guide. Since it is probably the most straight forward sample w/ least amount of code we have I think this is a good candidate to show both localhost and ngrok.
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.
Sounds good to me 👍
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.
examples/node-oauth/README.md
Outdated
# See https://developers.miro.com/docs/app-manifest on how to use this | ||
appName: OAuth2.0 with NodeJS | ||
redirectUris: | ||
- https://01-11-11-2011-74.ngrok-free.app |
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.
localhost?
c03898b
to
3735957
Compare
Co-authored-by: Mettin Parzinski <[email protected]>