Skip to content

Commit c0c83b9

Browse files
dario-piotrowiczvicb
dario-piotrowicz
and
vicb
committed
fix: make sure that fetch cache sets are properly awaited
Next.js does not await promises that update the incremental cache for fetch requests, that is needed in our runtime otherwise the cache updates get lost, so this change makes sure that the promise is properly awaited via `waitUntil` Co-authored-by: Victor Berchet <[email protected]>
1 parent c3dc401 commit c0c83b9

File tree

9 files changed

+239
-54
lines changed

9 files changed

+239
-54
lines changed

‎.changeset/few-ducks-listen.md‎

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,9 @@
1+
---
2+
"@opennextjs/cloudflare": patch
3+
---
4+
5+
fix: make sure that fetch cache `set`s are properly awaited
6+
7+
Next.js does not await promises that update the incremental cache for fetch requests,
8+
that is needed in our runtime otherwise the cache updates get lost, so this change
9+
makes sure that the promise is properly awaited via `waitUntil`

‎examples/e2e/app-router/e2e/ssr.test.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -28,7 +28,7 @@ test.skip("Server Side Render and loading.tsx", async ({ page }) => {
2828
}
2929
});
3030

31-
test.skip("Fetch cache properly cached", async ({ page }) => {
31+
test("Fetch cache properly cached", async ({ page }) => {
3232
await page.goto("/ssr");
3333
const originalDate = await page.getByText("Cached fetch:").textContent();
3434
await page.waitForTimeout(2000);

‎packages/cloudflare/src/cli/build/bundle-server.ts‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@ import { patchVercelOgLibrary } from "./patches/ast/patch-vercel-og-library.js";
1010
import { patchWebpackRuntime } from "./patches/ast/webpack-runtime.js";
1111
import * as patches from "./patches/index.js";
1212
import { ContentUpdater } from "./patches/plugins/content-updater.js";
13+
import { patchFetchCacheSetMissingWaitUntil } from "./patches/plugins/fetch-cache-wait-until.js";
1314
import { patchLoadInstrumentation } from "./patches/plugins/load-instrumentation.js";
1415
import { handleOptionalDependencies } from "./patches/plugins/optional-deps.js";
1516
import { fixRequire } from "./patches/plugins/require.js";
@@ -87,6 +88,7 @@ export async function bundleServer(buildOpts: BuildOptions): Promise<void> {
8788
fixRequire(updater),
8889
handleOptionalDependencies(optionalDependencies),
8990
patchLoadInstrumentation(updater),
91+
patchFetchCacheSetMissingWaitUntil(updater),
9092
// Apply updater updaters, must be the last plugin
9193
updater.plugin,
9294
],

‎packages/cloudflare/src/cli/build/patches/plugins/content-updater.ts‎

Lines changed: 17 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -8,11 +8,21 @@ import { readFile } from "node:fs/promises";
88

99
import { type OnLoadArgs, type OnLoadOptions, type Plugin, type PluginBuild } from "esbuild";
1010

11+
/**
12+
* The callbacks returns either an updated content or undefined if the content is unchanged.
13+
*/
1114
export type Callback = (args: {
1215
contents: string;
1316
path: string;
1417
}) => string | undefined | Promise<string | undefined>;
15-
export type Updater = OnLoadOptions & { callback: Callback };
18+
19+
/**
20+
* The callback is called only when `contentFilter` matches the content.
21+
* It can be used as a fast heuristic to prevent an expensive update.
22+
*/
23+
export type OnUpdateOptions = OnLoadOptions & { contentFilter: RegExp };
24+
25+
export type Updater = OnUpdateOptions & { callback: Callback };
1626

1727
export class ContentUpdater {
1828
updaters = new Map<string, Updater>();
@@ -23,11 +33,11 @@ export class ContentUpdater {
2333
* The callbacks are called in order of registration.
2434
*
2535
* @param name The name of the plugin (must be unique).
26-
* @param options Same options as the `onLoad` hook to restrict updates.
36+
* @param options Options.
2737
* @param callback The callback updating the content.
2838
* @returns A noop ESBuild plugin.
2939
*/
30-
updateContent(name: string, options: OnLoadOptions, callback: Callback): Plugin {
40+
updateContent(name: string, options: OnUpdateOptions, callback: Callback): Plugin {
3141
if (this.updaters.has(name)) {
3242
throw new Error(`Plugin "${name}" already registered`);
3343
}
@@ -48,13 +58,16 @@ export class ContentUpdater {
4858
setup: async (build: PluginBuild) => {
4959
build.onLoad({ filter: /\.(js|mjs|cjs|jsx|ts|tsx)$/ }, async (args: OnLoadArgs) => {
5060
let contents = await readFile(args.path, "utf-8");
51-
for (const { filter, namespace, callback } of this.updaters.values()) {
61+
for (const { filter, namespace, contentFilter, callback } of this.updaters.values()) {
5262
if (namespace !== undefined && args.namespace !== namespace) {
5363
continue;
5464
}
5565
if (!filter.test(args.path)) {
5666
continue;
5767
}
68+
if (!contentFilter.test(contents)) {
69+
continue;
70+
}
5871
contents = (await callback({ contents, path: args.path })) ?? contents;
5972
}
6073
return { contents };

‎packages/cloudflare/src/cli/build/patches/plugins/fetch-cache-wait-until.spec.ts‎

Lines changed: 107 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,107 @@
1+
import { describe, expect, test } from "vitest";
2+
3+
import { patchCode } from "../ast/util.js";
4+
import { ruleForMinifiedCode, ruleForNonMinifiedCode } from "./fetch-cache-wait-until.js";
5+
6+
describe("patchFetchCacheSetMissingWaitUntil", () => {
7+
test("on minified code", () => {
8+
const code = `
9+
{
10+
let [o4, a2] = (0, d2.cloneResponse)(e3);
11+
return o4.arrayBuffer().then(async (e4) => {
12+
var a3;
13+
let i4 = Buffer.from(e4), s3 = { headers: Object.fromEntries(o4.headers.entries()), body: i4.toString("base64"), status: o4.status, url: o4.url };
14+
null == $ || null == (a3 = $.serverComponentsHmrCache) || a3.set(n2, s3), F && await H.set(n2, { kind: c2.CachedRouteKind.FETCH, data: s3, revalidate: t5 }, { fetchCache: true, revalidate: r4, fetchUrl: _, fetchIdx: q, tags: A2 });
15+
}).catch((e4) => console.warn("Failed to set fetch cache", u4, e4)).finally(X), a2;
16+
}`;
17+
18+
expect(patchCode(code, ruleForMinifiedCode)).toMatchInlineSnapshot(`
19+
"{
20+
let [o4, a2] = (0, d2.cloneResponse)(e3);
21+
globalThis.__openNextAls.getStore().waitUntil(o4.arrayBuffer().then(async (e4) => {
22+
var a3;
23+
let i4 = Buffer.from(e4), s3 = { headers: Object.fromEntries(o4.headers.entries()), body: i4.toString("base64"), status: o4.status, url: o4.url };
24+
null == $ || null == (a3 = $.serverComponentsHmrCache) || a3.set(n2, s3), F && await H.set(n2, { kind: c2.CachedRouteKind.FETCH, data: s3, revalidate: t5 }, { fetchCache: true, revalidate: r4, fetchUrl: _, fetchIdx: q, tags: A2 });
25+
}).catch((e4) => console.warn("Failed to set fetch cache", u4, e4)).finally(X));
26+
return a2;
27+
28+
}"
29+
`);
30+
});
31+
32+
test("on non-minified code", () => {
33+
const code = `
34+
// We're cloning the response using this utility because there
35+
// exists a bug in the undici library around response cloning.
36+
// See the following pull request for more details:
37+
// https://github.com/vercel/next.js/pull/73274
38+
const [cloned1, cloned2] = (0, _cloneresponse.cloneResponse)(res);
39+
// We are dynamically rendering including dev mode. We want to return
40+
// the response to the caller as soon as possible because it might stream
41+
// over a very long time.
42+
cloned1.arrayBuffer().then(async (arrayBuffer)=>{
43+
var _requestStore_serverComponentsHmrCache;
44+
const bodyBuffer = Buffer.from(arrayBuffer);
45+
const fetchedData = {
46+
headers: Object.fromEntries(cloned1.headers.entries()),
47+
body: bodyBuffer.toString('base64'),
48+
status: cloned1.status,
49+
url: cloned1.url
50+
};
51+
requestStore == null ? void 0 : (_requestStore_serverComponentsHmrCache = requestStore.serverComponentsHmrCache) == null ? void 0 : _requestStore_serverComponentsHmrCache.set(cacheKey, fetchedData);
52+
if (isCacheableRevalidate) {
53+
await incrementalCache.set(cacheKey, {
54+
kind: _responsecache.CachedRouteKind.FETCH,
55+
data: fetchedData,
56+
revalidate: normalizedRevalidate
57+
}, {
58+
fetchCache: true,
59+
revalidate: externalRevalidate,
60+
fetchUrl,
61+
fetchIdx,
62+
tags
63+
});
64+
}
65+
}).catch((error)=>console.warn(\`Failed to set fetch cache\`, input, error)).finally(handleUnlock);
66+
return cloned2;
67+
`;
68+
69+
expect(patchCode(code, ruleForNonMinifiedCode)).toMatchInlineSnapshot(`
70+
"// We're cloning the response using this utility because there
71+
// exists a bug in the undici library around response cloning.
72+
// See the following pull request for more details:
73+
// https://github.com/vercel/next.js/pull/73274
74+
const [cloned1, cloned2] = (0, _cloneresponse.cloneResponse)(res);
75+
// We are dynamically rendering including dev mode. We want to return
76+
// the response to the caller as soon as possible because it might stream
77+
// over a very long time.
78+
globalThis.__openNextAls.getStore().waitUntil(cloned1.arrayBuffer().then(async (arrayBuffer)=>{
79+
var _requestStore_serverComponentsHmrCache;
80+
const bodyBuffer = Buffer.from(arrayBuffer);
81+
const fetchedData = {
82+
headers: Object.fromEntries(cloned1.headers.entries()),
83+
body: bodyBuffer.toString('base64'),
84+
status: cloned1.status,
85+
url: cloned1.url
86+
};
87+
requestStore == null ? void 0 : (_requestStore_serverComponentsHmrCache = requestStore.serverComponentsHmrCache) == null ? void 0 : _requestStore_serverComponentsHmrCache.set(cacheKey, fetchedData);
88+
if (isCacheableRevalidate) {
89+
await incrementalCache.set(cacheKey, {
90+
kind: _responsecache.CachedRouteKind.FETCH,
91+
data: fetchedData,
92+
revalidate: normalizedRevalidate
93+
}, {
94+
fetchCache: true,
95+
revalidate: externalRevalidate,
96+
fetchUrl,
97+
fetchIdx,
98+
tags
99+
});
100+
}
101+
}).catch((error)=>console.warn(\`Failed to set fetch cache\`, input, error)).finally(handleUnlock));
102+
103+
return cloned2;
104+
"
105+
`);
106+
});
107+
});

‎packages/cloudflare/src/cli/build/patches/plugins/fetch-cache-wait-until.ts‎

Lines changed: 57 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,57 @@
1+
import { patchCode } from "../ast/util.js";
2+
import type { ContentUpdater } from "./content-updater.js";
3+
4+
/**
5+
* The following Next.js code sets values in the incremental cache for fetch calls:
6+
* https://github.com/vercel/next.js/blob/e5fc495e3d4/packages/next/src/server/lib/patch-fetch.ts#L690-L728
7+
*
8+
* The issue here is that this promise is never awaited in the Next.js code (since in a standard node.js server
9+
* the promise will eventually simply just run) but we do need to run it inside `waitUntil` (so that the worker
10+
* is not killed before the promise is fully executed), without that this promise gets discarded and values
11+
* don't get saved in the incremental cache.
12+
*
13+
* This function wraps the promise in a `waitUntil` call (retrieved from `globalThis.__openNextAls.getStore()`).
14+
*/
15+
export function patchFetchCacheSetMissingWaitUntil(updater: ContentUpdater) {
16+
return updater.updateContent(
17+
"patch-fetch-cache-set-missing-wait-until",
18+
{ filter: /\.(js|mjs|cjs|jsx|ts|tsx)$/, contentFilter: /Failed to set fetch cache/ },
19+
({ contents }) => {
20+
contents = patchCode(contents, ruleForMinifiedCode);
21+
return patchCode(contents, ruleForNonMinifiedCode);
22+
}
23+
);
24+
}
25+
26+
export const ruleForMinifiedCode = `
27+
rule:
28+
kind: return_statement
29+
pattern: return $PROMISE, $CLONED2
30+
regex: Failed to set fetch cache
31+
follows:
32+
kind: lexical_declaration
33+
pattern: let [$CLONED1, $CLONED2]
34+
35+
fix: |
36+
globalThis.__openNextAls?.getStore()?.waitUntil?.($PROMISE);
37+
return $CLONED2;
38+
`;
39+
40+
export const ruleForNonMinifiedCode = `
41+
rule:
42+
kind: expression_statement
43+
regex: Failed to set fetch cache
44+
pattern: $PROMISE;
45+
follows:
46+
kind: comment
47+
follows:
48+
kind: comment
49+
follows:
50+
kind: comment
51+
follows:
52+
kind: lexical_declaration
53+
pattern: const [cloned1, cloned2]
54+
55+
fix: |
56+
globalThis.__openNextAls?.getStore()?.waitUntil?.($PROMISE);
57+
`;

‎packages/cloudflare/src/cli/build/patches/plugins/load-instrumentation.ts‎

Lines changed: 2 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -18,11 +18,7 @@ fix: async loadInstrumentationModule() { }
1818
export function patchLoadInstrumentation(updater: ContentUpdater) {
1919
return updater.updateContent(
2020
"patch-load-instrumentation",
21-
{ filter: /\.(js|mjs|cjs|jsx|ts|tsx)$/ },
22-
({ contents }) => {
23-
if (/async loadInstrumentationModule\(/.test(contents)) {
24-
return patchCode(contents, instrumentationRule);
25-
}
26-
}
21+
{ filter: /\.(js|mjs|cjs|jsx|ts|tsx)$/, contentFilter: /async loadInstrumentationModule\(/ },
22+
({ contents }) => patchCode(contents, instrumentationRule)
2723
);
2824
}

‎packages/cloudflare/src/cli/build/patches/plugins/require-page.ts‎

Lines changed: 2 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -12,12 +12,9 @@ export function inlineRequirePagePlugin(updater: ContentUpdater, buildOpts: Buil
1212
"inline-require-page",
1313
{
1414
filter: getCrossPlatformPathRegex(String.raw`/next/dist/server/require\.js$`, { escape: false }),
15+
contentFilter: /function requirePage\(/,
1516
},
16-
async ({ contents }) => {
17-
if (/function requirePage\(/.test(contents)) {
18-
return patchCode(contents, await getRule(buildOpts));
19-
}
20-
}
17+
async ({ contents }) => patchCode(contents, await getRule(buildOpts))
2118
);
2219
}
2320

‎packages/cloudflare/src/cli/build/patches/plugins/require.ts‎

Lines changed: 42 additions & 38 deletions
Original file line numberDiff line numberDiff line change
@@ -1,44 +1,48 @@
11
import type { ContentUpdater } from "./content-updater";
22

33
export function fixRequire(updater: ContentUpdater) {
4-
return updater.updateContent("fix-require", { filter: /\.(js|mjs|cjs|jsx|ts|tsx)$/ }, ({ contents }) => {
5-
// `eval(...)` is not supported by workerd.
6-
contents = contents.replaceAll(`eval("require")`, "require");
4+
return updater.updateContent(
5+
"fix-require",
6+
{ filter: /\.(js|mjs|cjs|jsx|ts|tsx)$/, contentFilter: /.*/ },
7+
({ contents }) => {
8+
// `eval(...)` is not supported by workerd.
9+
contents = contents.replaceAll(`eval("require")`, "require");
710

8-
// `@opentelemetry` has a few issues.
9-
//
10-
// Next.js has the following code in `next/dist/server/lib/trace/tracer.js`:
11-
//
12-
// try {
13-
// api = require('@opentelemetry/api');
14-
// } catch (err) {
15-
// api = require('next/dist/compiled/@opentelemetry/api');
16-
// }
17-
//
18-
// The intent is to allow users to install their own version of `@opentelemetry/api`.
19-
//
20-
// The problem is that even when users do not explicitely install `@opentelemetry/api`,
21-
// `require('@opentelemetry/api')` resolves to the package which is a dependency
22-
// of Next.
23-
//
24-
// The second problem is that when Next traces files, it would not copy the `api/build/esm`
25-
// folder (used by the `module` conditions in package.json) it would only copy `api/build/src`.
26-
// This could be solved by updating the next config:
27-
//
28-
// const nextConfig: NextConfig = {
29-
// // ...
30-
// outputFileTracingIncludes: {
31-
// "*": ["./node_modules/@opentelemetry/api/build/**/*"],
32-
// },
33-
// };
34-
//
35-
// We can consider doing that when we want to enable users to install their own version
36-
// of `@opentelemetry/api`. For now we simply use the pre-compiled version.
37-
contents = contents.replace(
38-
/require\(.@opentelemetry\/api.\)/g,
39-
`require("next/dist/compiled/@opentelemetry/api")`
40-
);
11+
// `@opentelemetry` has a few issues.
12+
//
13+
// Next.js has the following code in `next/dist/server/lib/trace/tracer.js`:
14+
//
15+
// try {
16+
// api = require('@opentelemetry/api');
17+
// } catch (err) {
18+
// api = require('next/dist/compiled/@opentelemetry/api');
19+
// }
20+
//
21+
// The intent is to allow users to install their own version of `@opentelemetry/api`.
22+
//
23+
// The problem is that even when users do not explicitly install `@opentelemetry/api`,
24+
// `require('@opentelemetry/api')` resolves to the package which is a dependency
25+
// of Next.
26+
//
27+
// The second problem is that when Next traces files, it would not copy the `api/build/esm`
28+
// folder (used by the `module` conditions in package.json) it would only copy `api/build/src`.
29+
// This could be solved by updating the next config:
30+
//
31+
// const nextConfig: NextConfig = {
32+
// // ...
33+
// outputFileTracingIncludes: {
34+
// "*": ["./node_modules/@opentelemetry/api/build/**/*"],
35+
// },
36+
// };
37+
//
38+
// We can consider doing that when we want to enable users to install their own version
39+
// of `@opentelemetry/api`. For now we simply use the pre-compiled version.
40+
contents = contents.replace(
41+
/require\(.@opentelemetry\/api.\)/g,
42+
`require("next/dist/compiled/@opentelemetry/api")`
43+
);
4144

42-
return contents;
43-
});
45+
return contents;
46+
}
47+
);
4448
}

0 commit comments

Comments
 (0)