[fix] set rawBody to null when body is nullish in simulated load fetch

[Conduitry]

Fixes #2294. No tests yet. I'd appreciate suggestions for where/how those should be added, as I haven't spent a ton of time in this codebase.

Before submitting the PR, please make sure you do the following

• It's really useful if your PR references an issue where it is discussed ahead of time. In many cases, features are absent for a reason. For large changes, please create an RFC: https://github.com/sveltejs/rfcs
• This message body should clearly illustrate what problems it solves.
• Ideally, include a test that fails without this PR but passes with it.

Tests

• Run the tests with pnpm test and lint the project with pnpm lint and pnpm check

Changesets

• If your PR makes a change that should be noted in one or more packages' changelogs, generate a changeset by running pnpx changeset and following the prompts. All changesets should be patch until SvelteKit 1.0

[changeset-bot]

🦋 Changeset detected

Latest commit: a140832

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
@sveltejs/kit	Patch

Not sure what this means? Click here to learn what changesets are.

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

[benmccann]

lgtm.

i'm not sure the best place to test this if you want to do an integration test. but a unit test might run faster and be less flaky

the smallest unit test would be to make an encode_body function. that's a pretty small function and barely worth having though. an alternative might be to refactor out a function to make the IncomingRequest:

/**
 * @param {string} url
 * @param {RequestInit} opts
 * @param {import('types/hooks').ServerRequest} request
 * @param {string} relative
 * @returns {import('types/app').IncomingRequest}
 */
export function incoming_request(url, opts, request, relative) {
	const headers = /** @type {import('types/helper').RequestHeaders} */ ({
		...opts.headers
	});

	if (opts.credentials !== 'omit') {
		headers.cookie = request.headers.cookie;

		if (!headers.authorization) {
			headers.authorization = request.headers.authorization;
		}
	}

	const search = url.includes('?') ? url.slice(url.indexOf('?') + 1) : '';

	return {
		host: request.host,
		method: opts.method || 'GET',
		headers,
		path: relative,
		rawBody: opts.body === null ? null : new TextEncoder().encode(/** @type {string} */ (opts.body)),
		query: new URLSearchParams(search)
	};
}

[jthegedus]

Will this have an affect on Adapters? Currently rawBody is required and no nullable at the Adapter entrypoint.

[JeanJPNM]

I don't think this is the root cause

[JeanJPNM]

If opts.body were null the result would be an Uint8Array(4) [110, 117, 108, 108], so it's better to check for falsy values

[JeanJPNM]

I forgot about undefined. This one actually yields an empty Uint8Array.

[JeanJPNM]

Also, the type cast (/** @type {string} */) is not necessary anymore, since the check already asserts that body will be a string

[JeanJPNM]

Suggested change

	rawBody:
	opts.body == null
	? null
	: new TextEncoder().encode(/** @type {string} */ (opts.body)),
	rawBody: opts.body == null ? null : new TextEncoder().encode(opts.body),

[Conduitry]

@JeanJPNM Yup, the cast to string is unnecessary now, good catch.

I'm confused about what you're saying about Uint8Array(4) [110, 117, 108, 108] - if that were being returned before for opts.body === null, that seems like it would have been undesirable behavior, and making rawBody be null in that case seems better.

I'm also confused about why you're saying this isn't the root cause. At

kit/packages/kit/src/runtime/server/parse_body/index.js

Line 8 in 02f7aba

if (!raw) return raw;

in parse_body we're parsing that rawBody of null as a body of null, which node-fetch then doesn't complain about. Is there a different route you think we should be taking to that?

---

@jthegedus In the discussions on #2215, it was decided that rawBody should be missing/null when an incoming request has no body. This is true for real requests coming in, but wasn't for simulated load fetch requests (which is what this issue is for). In light of that PR, it sounds like the types already needed updating, even without this PR. I'll open another issue for updating the types.

[JeanJPNM]

About the root cause: I thought that the problem wasn't on that line of code because the typecast was there as a "promise" that the body would be a string, but because the body was undefined, I thought that the code that created opts.body had the root cause of the bug.

[jthegedus]

@jthegedus In the discussions on #2215, it was decided that rawBody should be missing/null when an incoming request has no body. This is true for real requests coming in, but wasn't for simulated load fetch requests (which is what this issue is for). In light of that PR, it sounds like the types already needed updating, even without this PR.

Great, thanks for the clarification. I didn't read that whole discussion, so when I updated the svelte-firebase-adapter to conform to that new rawBody type I used the current TS types which require Uint8Array and so I actually create a new Uint8Array() when no body exists. Wasn't sure if that was right given what I have seen other adapters do.

I'll open another issue for updating the types.

Thanks, I will await the type fix and update on my end accordingly.