Closed codercms closed 7 months ago
@sindresorhus I think the test environment should be fixed because there is something wrong with xo or eslint
I think the test environment should be fixed because there is something wrong with xo or eslint
Fixed in main
branch. Just rebase from main
.
Can you explain your changes? It still does cloning, so I'm not sure how it fixes the issue for SvelteKit? And are you sure this won't have any adverse effects on Ky usage? There is probably a reason we always do cloning, even though I cannot remember why.
@sindresorhus let me explain a little bit more, SvelteKit has a smart caching system. When you run some requests during SSR they will not run again during CSR (if you met all requirements). This caching system relies heavily on the "fetch" function that SvelteKit provides for data loading functions (special functions to load data for app routes).
In vanilla JS it looks like this:
/** @type {import('./$types').PageLoad} */
export async function load({ depends, fetch }) {
const resp = await fetch('...some resource');
const json = await resp.json();
return {
someData: json
};
}
So, when response is cloned this logic gets broken, because SvelteKit hooks fetch on itself, from https://github.com/sveltejs/kit/blob/5ef7d0b387a8e0ff2f8d631d83585d479595d73c/packages/kit/src/runtime/server/page/load_data.js#L246:
As you can see from the source code, SvelteKit hooks the fetch response through the Proxy object. And there is also a special logic with the hooked "text" method - calling fetched.push
(the "json" method just calls the "text" method and parses the text into JSON).
This is fundamental for SvelteKit to remember which requests were made during SSR.
So, let's look now at ky's source code:
for (const [type, mimeType] of Object.entries(responseTypes) as ObjectEntries<typeof responseTypes>) {
result[type] = async () => {
// eslint-disable-next-line @typescript-eslint/prefer-nullish-coalescing
ky.request.headers.set('accept', ky.request.headers.get('accept') || mimeType);
// here is SvelteKit hooked response via Proxy
const awaitedResult = await result;
// and here is "un-hooked" response, so SvelteKit will never know that the "text" method was actually called
const response = awaitedResult.clone();
if (type === 'json') {
if (response.status === 204) {
return '';
}
const arrayBuffer = await response.clone().arrayBuffer();
const responseSize = arrayBuffer.byteLength;
if (responseSize === 0) {
return '';
}
// but this could be easy fixed by returning "not cloned " response, this will keep SvelteKit's hook on returned response
if (options.parseJson) {
// calling .text() on hooked response
return options.parseJson(await awaitedResult.text());
}
}
return awaitedResult[type]();
// instead of this
// return response[type]();
};
}
return result;
@sindresorhus
And are you sure this won't have any adverse effects on Ky usage? There is probably a reason we always do cloning, even though I cannot remember why.
The only reason I know for cloning a response is you want to access the response body more than once, so there is already one copy of the response and I think it's safe enough to return body from original response.
I’d love to get rid of clone() where possible, it’s given us quite a few problems. The main thing that needs to be checked for regressions is hooks that use the response. Each hook needs to receive a cloned response so that they are not dependent upon each other.
Closing as this is not moving forward. We are still happy to improve SvelteKit compatibility, but the submitter must do extensive testing to ensure it doesn't break anything like hooks.
response.clone() breaks compatibility with libraries which hooks fetch on itself, e.g. SvelteKit - https://github.com/sveltejs/kit/blob/5ef7d0b387a8e0ff2f8d631d83585d479595d73c/packages/kit/src/runtime/server/page/load_data.js#L246
Response cloning leading to lost context for SvelteKit and request would be resent during CSR