⚠️ No Changeset found

Latest commit: 69e26ee

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

✔️ Deploy Preview for kit-demo canceled.

🔨 Explore the source changes: 69e26ee

🔍 Inspect the deploy log:

It looks like there's another PR to fix this issue as well: #2911. I haven't looked at either in any detail yet. Can you comment on the differences between the two approaches?

It looks like there's another PR to fix this issue as well: #2911. I haven't looked at either in any detail yet. Can you comment on the differences between the two approaches?

the param of function load_node have changed after commit ee906a3b

* @param {{
 *   request: import('types/hooks').ServerRequest;
 *   event: import('types/hooks').RequestEvent;

The code below is unable to achieve goals. 228f0e660

const headers = /** @type {import('types/helper').RequestHeaders} */ ({
    ...request.headers,
    ...opts.headers
});

Headers can't be merged.

I try to modify it 6f5308e7:

opts.headers = new Headers({
	...Object.fromEntries(event.request.headers),
	...opts.headers
});

It's working, but it can't pass tests.
I didn't look at the test. I guess opts.headers is a Headers object in the test.

This also relates to my #3503 where I'm grasping around for ways to get at the incoming request headers in order to fulfill a server-side rendered response (and one that makes data fetch calls).

I suggested either making "event" available in externalFetch, or for a narrower scoped change, making "fetch" have the cookies the way it would if you did a credentials: "include" fetch on the client. Haven't heard if either way is preferred, or neither.

Question though -- should this behavior be gated by the presence of "credentials: include"? At the very least it seems like it should respect "credentials: omit" the same as they do here right?

https://github.com/sveltejs/kit/blob/35461704dcc2e3859abb9a77e36163cc722c8708/packages/kit/src/runtime/server/page/load_node.js#L179

Thanks @aolose. @johnnysprinkles yes, we do need to account for credentials — cookie and authorization need special treatment. Will update the PR

Realised there's at least a couple of headers that should get special casing — if-none-match should be removed (it's harmless, but can't possibly match), and referer should be set to the page URL.

Adding some tests alongside this behaviour

This fix appears to be causing problems when I'm using the fetch command in a route's load function within the module context.

I'm getting the following error "Unexpected token < in JSON at position 2", which seems to mean that I'm getting an HTML error page in place of the JSON file that I'm trying to request (it worked perfectly in 1.0.0-next.252 but breaks in 1.0.0-next.252).

I'm guessing this issue can be solved by explicitly adding certain headers to the request, eg. allowing CORS? It would be great if requests could be sent with suitable default headers to prevent this issue!

Additional info: It seems this issue may only affect requests that have an "Authorization" header.

I'm getting an error too. I think we need more doc to say when to use fetch from global and fetch from loadInput.

@bothness comments on merged PRs will end up getting lost. If you have encountered a bug, please file a new issue

Thanks for spotting this @benmccann. I'll file a fresh issue.