sapper
sapper copied to clipboard
sveltejs
Let `this.fetch` forward cookies received on the server
What does this solve?
Requests performed in preload functions might set cookies in their response headers.
When executed on the server, this data is lost as headers are not forwarded to the client.
Current work-around requires code duplication: one part in a script tag, the other in a middleware to have access to a res object.
How does it solve it?
This PR adds an option to this.fetch to let callers ensure cookies are forwarded in the response if run server-side.
I believe that this method is trying to maintain API compatibility with the browser's fetch method, so this probably isn't how we'd want to implement this: https://developer.mozilla.org/en-US/docs/Web/API/WindowOrWorkerGlobalScope/fetch#Parameters
Got it. What do you think about forwarding received cookies by default then?
The current behaviour leads to different outcomes depending on where preload runs, which as I understand is what this.fetch tries to avoid.
It kind of looks to me like that's already being done in the block above in the if (include_credentials) block. I guess the question would be what's different about your usecase such that include_credentials is false - are you making requests across domains?
The block above forwards request cookies, this PR is about forwarding the ones set by the response.
An example use case is an API for authentication setting a JWT in a Set-Cookie header.
Currently, if the request is performed from preload, the cookie is set on the browser only if preload runs client-side.
I'd probably want a second pair of eyes on it, but that sounds reasonable. I think if it were done that way I'd expect it to happen inside the if (include_credentials) block.
The common authentication usecase would fit in this block yes.
However, including credentials in the request and forwarding response cookies seem like different concerns to me.
APIs might store arbitrary cookies on the client, not just for authentication purposes.
This can include requests to endpoints accessible without credentials: 'include'.
Thanks for the PR! And sorry it took awhile to review. It's probably best to test if this works as hoped in SvelteKit. We wouldn't want to have it implemented only in Sapper for people to lose functionality when migrating
SvelteKit 1.0 is now out and Sapper is deprecated, so I'm going to close this