Skip to content
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

BREAKING: add getSessionData() #295

Draft
wants to merge 17 commits into
base: main
Choose a base branch
from
Draft

BREAKING: add getSessionData() #295

wants to merge 17 commits into from

Conversation

iuioiua
Copy link
Collaborator

@iuioiua iuioiua commented Jan 22, 2024

Previously, the dev would use getSessionId() to check whether the user is signed in. They'd then use the sessionId value to perform database operations, such as getting some session/user object.

Now, this is all done in a single step. The dev configures which data is saved to the database within handleCallback(). getSessionData() then returns that data from a given request.

Reviews, concerns and suggestions are welcome.

@iuioiua iuioiua changed the title BREAKING: add getSessionObject() BREAKING: add getSessionData() Feb 1, 2024
@iuioiua iuioiua marked this pull request as ready for review February 5, 2024 06:36
lib/handle_callback.ts Outdated Show resolved Hide resolved
lib/handle_callback.ts Outdated Show resolved Hide resolved
@iuioiua iuioiua marked this pull request as draft February 12, 2024 07:49
@iuioiua iuioiua removed the request for review from lucacasonato February 12, 2024 07:49
@iuioiua iuioiua marked this pull request as ready for review February 18, 2024 23:50
Copy link
Member

@kt3k kt3k left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@iuioiua iuioiua requested a review from kt3k February 19, 2024 09:51
Copy link
Member

@kt3k kt3k left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM again

sessionId,
tokens,
};
return response;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it may be useful to still return the tokens from this function. Either that pass the whole token object to tokenCallback?

This used to be built into this module but got removed, which I understand the reasoning for, but I feel like some people may want the ability to ensure sessions are still valid and be able to refresh the access token in the cases that it is not.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, what got removed?

Hm. I hadn't considered refresh tokens. Let me think about this and see what to do about it. Thanks for chiming in.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It used to store the access/refresh tokens in KV and would automatically refresh the session but that was removed some time back. I remember fixing some bugs in that area but it was removed soon after

@iuioiua iuioiua marked this pull request as draft March 6, 2024 06:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants