Adds deleteCurrentSession function #60
Conversation
| const jwtPayload = decodeSessionToken(matches[1]); | ||
| const jwtSessionId = ShopifyOAuth.getJwtSessionId(jwtPayload.dest.replace(/^https:\/\//, ''), jwtPayload.sub); | ||
| await Context.deleteSession(jwtSessionId); | ||
| return true; |
There was a problem hiding this comment.
If we're always throwing when errors occur, do we still need to return true on success?
There was a problem hiding this comment.
Do you have an opinion on a good alternative? Our base deleteSession also returns true for successful deletion right now.
There was a problem hiding this comment.
I think that makes sense for the session storage since we need to know if the operation succeeded. But here, success means not hitting an error, so it doesn't have to return anything IMHO.
There was a problem hiding this comment.
Ah, I was wrong, deleteSession returns true regardless, but I'm not sure how I feel about that.
@andyw8 can I trouble you for some thoughts around this? |
WHY are these changes introduced?
We have
deleteSessionbut this requires first grabbing the session you want to delete to pass to that function. This essentially wraps that and does both: regardless of session type (JWT or Cookie) it will find the current session based on the currentrequestand delete it from session storage for you.WHAT is this pull request doing?
Adds the
deleteCurrentSessionmethod and testing. This method follows a similar pattern toloadCurrentSession.Type of change