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

Fix getServerSession parameter type #411

Closed
wants to merge 1 commit into from

Conversation

epavanello
Copy link

"getServerSession" is mainly used inside a "LayoutServerLoad" function (it expose an event typed "ServerLoadEvent"), but the event object is only propagated to "getRequestSupabaseClient" that accept a generic "RequestEvent". It doesn't work if I need to get the server session from an api (see https://kit.svelte.dev/docs/routing#server) that by "RequestHandler" offer only a generic "RequestEvent" insead of an "ServerLoadEvent".

What kind of change does this PR introduce?

Bug fix

What is the current behavior?

getServerSession has incompatible event type if used by apis (see https://kit.svelte.dev/docs/routing#server)

What is the new behavior?

Only a replacement for a more generic type

Additional context

"getServerSession" is mainly used inside a "LayoutServerLoad" function (it expose an event typed "ServerLoadEvent"), but the event object is only propagated to "getRequestSupabaseClient" that accept a generic "RequestEvent".
It doesn't work if I need to get the server session from an api (see https://kit.svelte.dev/docs/routing#server) that by "RequestHandler" offer only a generic "RequestEvent" insead of an "ServerLoadEvent".
@david-plugge
Copy link
Collaborator

Thanks for pointing this out.

ServerLoadEvent is the correct type here as we only use getServerSession to pass the session to universal load functions.
It´s a little complicated to explain why we need this helper.

The flow looks like this:

  1. handle in hooks.server.js
  2. server only load functions
  3. shared load functions

Inside the server load functions we can use getSupabase to get the supabase client instance. This instance is able to override the session cookie because we have access to cookies inside ServerLoadEvent. Same thing on +server request handlers.

But when a universal load function runs on the server we don´t have a way to override the session cookie (refresh the session when it´s expired or close to beeing expired).
getServerSession returns the session if it is currently valid for more than a set number of seconds and otherwise refreshes the clients session. That way we can make sure the client will never try to refresh the session in a universal load running on the server.

I hope this explains the usage of getServerSession.

So to sum it up, this PR should be closed and the docs should make clear why this helper function is needed.

But i didn´t had a lot of time to read up what changed in the supabase client package so we maybe don´t even need to include this helper anymore.

@epavanello
Copy link
Author

I'm sorry, I believe I poorly explained myself.
The issue is that I am unable to use getServerSession inside a sveltekit API (e.g. src/routes/my-api/+server.js) because the event parameter is of type RequestEvent, not ServerLoadEvent. (See the type here: https://kit.svelte.dev/docs/routing#server)
Therefore, I need to explicitly cast the event to ServerLoadEvent in order to call getServerSession.

@david-plugge
Copy link
Collaborator

david-plugge commented Jan 4, 2023

This is by design, you are meant to use const { session } = await getSupabase(event) in all load functions/requestHandlers.
getSupabase can be used everywhere in sveltekit as we internally detect which event type was passed in.
getServerSession should only be used inside the root layout load to propagate the session to shared load functions. We should rename getServerSession to something more verbose

@epavanello
Copy link
Author

Okay, I understand now.
It works with getSupabase(event)

@epavanello epavanello closed this Jan 4, 2023
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

2 participants