Repository: sveltejs/kit
Pull request #3521 (2022-01-24 02:40:31)

[fix] reading from same response body twice during prerender (#3473)
### Please don't delete this checklist! Before submitting the PR, please make sure you do the following:
- [X] 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
- [X] This message body should clearly illustrate what problems it solves.
- [X] Ideally, include a test that fails without this PR but passes with it.

This PR fixes the behavior described in #3473, where building the default template with adapter static fails as during the pre-rendering the same response body was read twice. As @bluwy pointed out in the original issue, this is related to fetch (and accordingly node-fetch) only allowing a single read of a response body and can be circumvented by using the clone() API that is provided for this purpose.
I also wrote a test that fails without this PR, and I think that would be generally useful to have.

### Tests
- [X] Run the tests with `pnpm test` and lint the project with `pnpm lint` and `pnpm check`

``test/test.js:953:2 › Load › handles large responses`` fails due to problems discussed in #591

### Changesets
- [X] 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


netlify[bot]: 👷 Deploy Preview for *kit-demo* processing.


🔨 Explore the source changes: 915fb2e013fd64e191e8d732e748f89822dce3f3

🔍 Inspect the deploy log: [https://app.netlify.com/sites/kit-demo/deploys/61ee11b8dff60c0008c67902](https://app.netlify.com/sites/kit-demo/deploys/61ee11b8dff60c0008c67902)

changeset-bot[bot]: ###  🦋  Changeset detected

Latest commit: 915fb2e013fd64e191e8d732e748f89822dce3f3

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

<details><summary>This PR includes changesets to release 1 package</summary>

  | Name          | Type  |
| ------------- | ----- |
| @sveltejs/kit | Patch |

</details>

Not sure what this means? [Click here  to learn what changesets are](https://github.com/changesets/changesets/blob/master/docs/adding-a-changeset.md).

[Click here if you're a maintainer who wants to add another changeset to this PR](https://github.com/Theo-Steiner/kit/new/3473/body-already-used?filename=.changeset/silent-panthers-wink.md&value=---%0A%22%40sveltejs%2Fkit%22%3A%20patch%0A---%0A%0A%5Bfix%5D%20reading%20from%20same%20response%20body%20twice%20during%20prerender%20(%233473)%0A)


Theo-Steiner: 
> Looks like cloning it has an impact on large responses (probably expected) and caused the tests to fail 🤔

 it looks like we bumped into #591 here...
Rich-Harris: Thank you.

I pushed a commit that changes the behaviour so that the prerenderer gets access to the same buffered body as the code that inlines responses into the HTML (i.e. without cloning the response), though as it was on its way to GitHub I realised that this would fail in the case where the data wasn't buffered during `load`.

I guess we'll need a hybrid approach in which we use the buffered body if it's available, otherwise buffer it at prerender time. Working on it.

Patch:
diff --git a/.changeset/nine-dots-occur.md b/.changeset/nine-dots-occur.md
new file mode 100644
index 000000000..31acf61d1
--- /dev/null
+++ b/.changeset/nine-dots-occur.md
@@ -0,0 +1,5 @@
+---
+'@sveltejs/kit': patch
+---
+
+[fix] reading from same response body twice during prerender (#3473)
diff --git a/packages/kit/src/core/adapt/prerender/prerender.js b/packages/kit/src/core/adapt/prerender/prerender.js
index d3c8f7005..acbf9d434 100644
--- a/packages/kit/src/core/adapt/prerender/prerender.js
+++ b/packages/kit/src/core/adapt/prerender/prerender.js
@@ -130,8 +130,9 @@ export async function prerender({ cwd, out, log, config, build_data, fallback, a
 	 * @param {string?} referrer
 	 */
 	async function visit(path, decoded_path, referrer) {
-		/** @type {Map<string, Response>} */
+		/** @type {Map<string, import('types/internal').PrerenderDependency>} */
 		const dependencies = new Map();
+
 		const render_path = config.kit.paths?.base
 			? `http://sveltekit-prerender${config.kit.paths.base}${path === '/' ? '' : path}`
 			: `http://sveltekit-prerender${path}`;
@@ -192,9 +193,11 @@ export async function prerender({ cwd, out, log, config, build_data, fallback, a
 			}
 
 			for (const [dependency_path, result] of dependencies) {
-				const response_type = Math.floor(result.status / 100);
+				const { status, headers } = result.response;
+
+				const response_type = Math.floor(status / 100);
 
-				const is_html = result.headers.get('content-type') === 'text/html';
+				const is_html = headers.get('content-type') === 'text/html';
 
 				const parts = dependency_path.split('/');
 				if (is_html && parts[parts.length - 1] !== 'index.html') {
@@ -204,16 +207,17 @@ export async function prerender({ cwd, out, log, config, build_data, fallback, a
 				const file = `${out}${parts.join('/')}`;
 				mkdirp(dirname(file));
 
-				if (result.body) {
-					writeFileSync(file, await result.text());
-					paths.push(dependency_path);
-				}
+				writeFileSync(
+					file,
+					result.body === null ? new Uint8Array(await result.response.arrayBuffer()) : result.body
+				);
+				paths.push(dependency_path);
 
 				if (response_type === OK) {
-					log.info(`${result.status} ${dependency_path}`);
+					log.info(`${status} ${dependency_path}`);
 				} else {
 					error({
-						status: result.status,
+						status,
 						path: dependency_path,
 						referrer: path,
 						referenceType: 'fetched'
diff --git a/packages/kit/src/runtime/server/page/load_node.js b/packages/kit/src/runtime/server/page/load_node.js
index f270b1211..66a61a9ff 100644
--- a/packages/kit/src/runtime/server/page/load_node.js
+++ b/packages/kit/src/runtime/server/page/load_node.js
@@ -99,6 +99,9 @@ export async function load_node({
 				/** @type {Response} */
 				let response;
 
+				/** @type {import('types/internal').PrerenderDependency} */
+				let dependency;
+
 				// handle fetch requests for static assets. e.g. prebaked data, etc.
 				// we need to support everything the browser's fetch supports
 				const prefix = options.paths.assets || options.paths.base;
@@ -125,8 +128,6 @@ export async function load_node({
 						response = await fetch(`${url.origin}/${file}`, /** @type {RequestInit} */ (opts));
 					}
 				} else if (is_root_relative(resolved)) {
-					const relative = resolved;
-
 					if (opts.credentials !== 'omit') {
 						uses_credentials = true;
 
@@ -150,20 +151,15 @@ export async function load_node({
 						throw new Error('Request body must be a string');
 					}
 
-					const rendered = await respond(
-						new Request(new URL(requested, event.url).href, opts),
-						options,
-						{
-							fetched: requested,
-							initiator: route
-						}
-					);
+					response = await respond(new Request(new URL(requested, event.url).href, opts), options, {
+						fetched: requested,
+						initiator: route
+					});
 
 					if (state.prerender) {
-						state.prerender.dependencies.set(relative, rendered);
+						dependency = { response, body: null };
+						state.prerender.dependencies.set(resolved, dependency);
 					}
-
-					response = rendered;
 				} else {
 					// external
 					if (resolved.startsWith('//')) {
@@ -219,9 +215,28 @@ export async function load_node({
 									});
 							}
 
+							if (dependency) {
+								dependency.body = body;
+							}
+
 							return body;
 						}
 
+						if (key === 'arrayBuffer') {
+							return async () => {
+								const buffer = await response.arrayBuffer();
+
+								if (dependency) {
+									dependency.body = new Uint8Array(buffer);
+								}
+
+								// TODO should buffer be inlined into the page (albeit base64'd)?
+								// any conditions in which it shouldn't be?
+
+								return buffer;
+							};
+						}
+
 						if (key === 'text') {
 							return text;
 						}
diff --git a/packages/kit/types/internal.d.ts b/packages/kit/types/internal.d.ts
index 5c2d85e4a..3b340c005 100644
--- a/packages/kit/types/internal.d.ts
+++ b/packages/kit/types/internal.d.ts
@@ -5,10 +5,15 @@ import { Either } from './helper';
 import { ExternalFetch, GetSession, Handle, HandleError, RequestEvent } from './hooks';
 import { Load } from './page';
 
+export interface PrerenderDependency {
+	response: Response;
+	body: null | string | Uint8Array;
+}
+
 export interface PrerenderOptions {
 	fallback?: string;
 	all: boolean;
-	dependencies: Map<string, Response>;
+	dependencies: Map<string, PrerenderDependency>;
 }
 
 export interface AppModule {


Test patch:
diff --git a/packages/kit/test/prerendering/basics/src/routes/fetch-endpoint/buffered.json.js b/packages/kit/test/prerendering/basics/src/routes/fetch-endpoint/buffered.json.js
new file mode 100644
index 000000000..ceb936631
--- /dev/null
+++ b/packages/kit/test/prerendering/basics/src/routes/fetch-endpoint/buffered.json.js
@@ -0,0 +1,5 @@
+export async function get() {
+	return {
+		body: { answer: 42 }
+	};
+}
diff --git a/packages/kit/test/prerendering/basics/src/routes/fetch-endpoint/buffered.svelte b/packages/kit/test/prerendering/basics/src/routes/fetch-endpoint/buffered.svelte
new file mode 100644
index 000000000..ea6c07029
--- /dev/null
+++ b/packages/kit/test/prerendering/basics/src/routes/fetch-endpoint/buffered.svelte
@@ -0,0 +1,18 @@
+<script context="module">
+	/** @type {import('@sveltejs/kit').Load} */
+	export async function load({ fetch }) {
+		const url = '/fetch-endpoint/buffered.json';
+		const res = await fetch(url);
+
+		return {
+			props: await res.json()
+		};
+	}
+</script>
+
+<script>
+	/** @type {number} */
+	export let answer;
+</script>
+
+<h1>the answer is {answer}</h1>
diff --git a/packages/kit/test/prerendering/basics/src/routes/fetch-endpoint/not-buffered.json.js b/packages/kit/test/prerendering/basics/src/routes/fetch-endpoint/not-buffered.json.js
new file mode 100644
index 000000000..ceb936631
--- /dev/null
+++ b/packages/kit/test/prerendering/basics/src/routes/fetch-endpoint/not-buffered.json.js
@@ -0,0 +1,5 @@
+export async function get() {
+	return {
+		body: { answer: 42 }
+	};
+}
diff --git a/packages/kit/test/prerendering/basics/src/routes/fetch-endpoint/not-buffered.svelte b/packages/kit/test/prerendering/basics/src/routes/fetch-endpoint/not-buffered.svelte
new file mode 100644
index 000000000..949a0b4e4
--- /dev/null
+++ b/packages/kit/test/prerendering/basics/src/routes/fetch-endpoint/not-buffered.svelte
@@ -0,0 +1,20 @@
+<script context="module">
+	/** @type {import('@sveltejs/kit').Load} */
+	export async function load({ fetch }) {
+		const url = '/fetch-endpoint/not-buffered.json';
+		const res = await fetch(url);
+
+		return {
+			props: {
+				headers: res.headers
+			}
+		};
+	}
+</script>
+
+<script>
+	/** @type {Headers} */
+	export let headers;
+</script>
+
+<h1>content-type: {headers.get('content-type')}</h1>
diff --git a/packages/kit/test/prerendering/basics/test/test.js b/packages/kit/test/prerendering/basics/test/test.js
index f58833560..16972c334 100644
--- a/packages/kit/test/prerendering/basics/test/test.js
+++ b/packages/kit/test/prerendering/basics/test/test.js
@@ -42,4 +42,20 @@ test('inserts http-equiv tag for cache-control headers', () => {
 	assert.ok(content.includes('<meta http-equiv="cache-control" content="max-age=300">'));
 });
 
+test('renders page with data from endpoint', () => {
+	const content = read('fetch-endpoint/buffered/index.html');
+	assert.ok(content.includes('<h1>the answer is 42</h1>'));
+
+	const json = read('fetch-endpoint/buffered.json');
+	assert.equal(json, JSON.stringify({ answer: 42 }));
+});
+
+test('renders page with unbuffered data from endpoint', () => {
+	const content = read('fetch-endpoint/not-buffered/index.html');
+	assert.ok(content.includes('<h1>content-type: application/json; charset=utf-8</h1>'), content);
+
+	const json = read('fetch-endpoint/not-buffered.json');
+	assert.equal(json, JSON.stringify({ answer: 42 }));
+});
+
 test.run();