Add example for Authorization Code grant. #9
Conversation
|
@jankapunkt not sure if you find this too much for a sample. I tried to be as concise as possible, but please let me know if I can change something! |
authorization-code/client/index.js
Outdated
| app.use(express.static("public")); | ||
|
|
||
| const authServer = "http://localhost:8080"; | ||
| const clientId = process.env.CLIENT_ID || "testclient"; |
There was a problem hiding this comment.
It would be nice if the sample worked just as is without any envs. So I added a default, but we can discuss the options.
There was a problem hiding this comment.
I think envs are fine, you can also use .env if you like, it's a good common practice
There was a problem hiding this comment.
I did add both options. Use the values from the defaults if the env does not exist. But I am happy to remove the defaults if you prefer it to come only from the env.
There was a problem hiding this comment.
Or should we commit a .env file with the defaults? Not like it contains secretive stuff... The intention being the samples work with as minimal effort as possible.
There was a problem hiding this comment.
common practice is to gitignore .env but provide a .env.example that users can copy. This avoids ever checking in real .env (in case users build their project upon the example)
There was a problem hiding this comment.
or do not check in a .env at all but tell users to create one with example data. My concerns are only regarding users cloning and continuing to use it until production.
There was a problem hiding this comment.
I added a .env.example file. Seems to be a good balance incase they use it till production.
There was a problem hiding this comment.
@jankapunkt FYI I got the example to a runnable state. Would you be able to run this when you have some availability?
There was a problem hiding this comment.
@shrihari-prakash yes I will test it and leave comments/review if needed
|
Ouch I thought it was about to be squashed and merged. I see that I pollluted the commit history of main branch. I shall keep this in mind and keep the commit history clean next time around :) cc: @jankapunkt |
|
Hey @shrihari-prakash sorry that was my bad, I plain merged instead of squash. Will also keep it in mind next time. |
We could also settle on squashing or bringing the commits into main and follow it in all future PRs. On one hand, the details about the author who made the fix is lost when you squash which possibly makes it hard to keep track of contributors. Perhaps it is better to keep the commit history clean from the time of opening the PR (which I should have probably done lol).... |
No description provided.