This repository was archived by the owner on Apr 11, 2024. It is now read-only.
Centralize logic on getting the current session id #88
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
paulomarg
force-pushed
the
refactor_delete_current_session
branch
from
6da677d to
0233f84
Compare
thecodepixi
reviewed
Jan 27, 2021
| * @param request HTTP request object | ||
| * @param response HTTP response object | ||
| */ | ||
| getCurrentSessionId(request: http.IncomingMessage, response: http.ServerResponse): string | undefined { |
There was a problem hiding this comment.
I'm wondering if it really makes sense for these functions (getOfflineSessionId and getCurrentSessionId) to live inside of OAuth class. Should we move them to somewhere within src/auth/session?
There was a problem hiding this comment.
I was actually torn about that myself. What made me keep them here (for now) was the fact that the OAuth function is basically what determines what session ids we use for which scenarios (offline vs online, embedded or not). But then again, they are all more deeply connected to Sessions than they are to OAuth, so I can see this going either way.
thecodepixi
reviewed
Jan 27, 2021
| ): Promise<boolean | never> { | ||
| Context.throwIfUninitialized(); | ||
|
|
||
| if (Context.IS_EMBEDDED_APP) { |
There was a problem hiding this comment.
I love all of these deleted lines 😍
thecodepixi
reviewed
Jan 27, 2021
paulomarg
force-pushed
the
refactor_delete_current_session
branch
from
0233f84 to
ea183a3
Compare
mkevinosullivan
approved these changes
Jan 27, 2021
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.
WHY are these changes introduced?
The previous implementation of
deleteCurrentSessionessentially replicated a previous iteration ofloadCurrentSession, which caused errors due to them being out of sync.WHAT is this pull request doing?
Extracted the logic to obtain the current session id from the request into a shared method, that is now called from both
loadCurrentSessionanddeleteCurrentSession, so they should always behave consistently.Type of change