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.
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
Refresh token impl #2212
Refresh token impl #2212
Changes from 28 commits
8151dd7
3d3dc18
69de38b
a12863b
5a8ec33
ac77fd2
e43062b
a37e843
c007564
4a55109
89d930b
57966af
4fa259b
f0c2f2b
a992c69
b918cb1
60e82ee
76217bc
5afc8b0
3c09189
2608397
4af397b
9586476
5d4b8dd
9c5ab8c
78a6317
e450e6e
2f050ea
1a36669
e11cb65
aff1acb
d19cac7
a7e8493
6fbeeda
2df9b52
2e42655
48c110b
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Any chance of avoiding a singleton module here?
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.
We need the one time tokens to allow the client to exchange one time codes for session IDs. If the server and client were on the same domain, it would be possible. This is not the case right now, so we need this kind of a singleton that keeps track of the tokens. This is not introduced in this PR btw. so I wouldn't change it in this PR irregardless.
At some point in the future, we'll need to figure out the cookies story so we can avoid 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.
Oh, I understand we need the single instance that keeps the tokens. What I meant was "Can we not use a singleton pattern for it?"
It's perfectly fine not to do it now. FYI, here are two good posts I have bookmarked that will help us avoid singletons in the future (although I admit I sometimes break this rule myself):
Good thing we're not testing anything so the singletons aren't causing any problems 🥲
Feel free to resolve.
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.
It's not a singleton in a sense that there can only exist only one instance of the object. It's true that we create a single instance that we have to use in both places where it's needed. In the case of unit tests, we could make the token store a param of the which ever fn we wanted to unit test.