Fix falback issue (#252)

* fix fallback false

* fix plugin not being applied

* fix get could fail on batchWriteError

* fix don't try to fetch missing key

* updated docs

Author: conico974

README.md

@@ -414,16 +414,19 @@ This cost estimate is based on the `us-east-1` region pricing and does not consi
 
 #### WORKAROUND: Patch fetch behaviour for ISR. Only for [email protected]+
 
-In order to make fetch work as expected with ISR, you need to patch the `fetch` function in your app. Just add this lines to your root layout component:
+If you use ISR and fetch in your app, you may encounter a bug that makes your revalidate values inconsistent. 
+The issue is that it revalidates using the lowest revalidate of all fetch calls in your page, regardless of their individual values. To fix this bug, you need to modify the fetch function in your root layout component with the following code snippet
 
 ```ts
-const asyncStorage = require("next/dist/client/components/static-generation-async-storage.external");
-//@ts-ignore
-const staticStore = (fetch as any).__nextGetStaticStore?.() || asyncStorage.staticGenerationAsyncStorage;
-const store = staticStore.getStore();
-store.isOnDemandRevalidate = store.isOnDemandRevalidate && !(process.env.OPEN_NEXT_ISR === 'true');
+export default function RootLayout() {
+  const asyncStorage = require("next/dist/client/components/static-generation-async-storage.external");
+  //@ts-ignore
+  const staticStore = (fetch as any).__nextGetStaticStore?.() || asyncStorage.staticGenerationAsyncStorage;
+  const store = staticStore.getStore();
+  store.isOnDemandRevalidate = store.isOnDemandRevalidate && !(process.env.OPEN_NEXT_ISR === 'true');
+  return <>...</>;
+}
 ```
-Without this workaround, if you have 2 fetch calls in your page with different revalidate values, both will use the smallest value during ISR revalidation.
 
 #### WORKAROUND: Create one cache behavior per top-level file and folder in `public/` (AWS specific)
 

packages/open-next/src/adapters/cache.ts

@@ -131,15 +131,18 @@ export default class S3Cache {
   public async get(key: string, options?: boolean | { fetchCache?: boolean }) {
     const isFetchCache =
       typeof options === "object" ? options.fetchCache : options;
+    const keys = await this.listS3Object(key);
+    if (keys.length === 0) return null;
+    debug("keys", keys);
     return isFetchCache
-      ? this.getFetchCache(key)
-      : this.getIncrementalCache(key);
+      ? this.getFetchCache(key, keys)
+      : this.getIncrementalCache(key, keys);
   }
 
-  async getFetchCache(key: string) {
+  async getFetchCache(key: string, keys: string[]) {
     debug("get fetch cache", { key });
     try {
-      const { Body, LastModified } = await this.getS3Object(key, "fetch");
+      const { Body, LastModified } = await this.getS3Object(key, "fetch", keys);
       const lastModified = await this.getHasRevalidatedTags(
         key,
         LastModified?.getTime(),
@@ -149,6 +152,8 @@ export default class S3Cache {
         return null;
       }
 
+      if (Body === null) return null;
+
       return {
         lastModified,
         value: JSON.parse((await Body?.transformToString()) ?? "{}"),
@@ -159,17 +164,16 @@ export default class S3Cache {
     }
   }
 
-  async getIncrementalCache(key: string): Promise<CacheHandlerValue | null> {
-    const keys = await this.listS3Object(key);
-    if (keys.length === 0) return null;
-    debug("keys", keys);
-
+  async getIncrementalCache(
+    key: string,
+    keys: string[],
+  ): Promise<CacheHandlerValue | null> {
     if (keys.includes(this.buildS3Key(key, "body"))) {
       debug("get body cache ", { key });
       try {
         const [{ Body, LastModified }, { Body: MetaBody }] = await Promise.all([
-          this.getS3Object(key, "body"),
-          this.getS3Object(key, "meta"),
+          this.getS3Object(key, "body", keys),
+          this.getS3Object(key, "meta", keys),
         ]);
         const body = await Body?.transformToByteArray();
         const meta = JSON.parse((await MetaBody?.transformToString()) ?? "{}");
@@ -198,9 +202,9 @@ export default class S3Cache {
       try {
         const [{ Body, LastModified }, { Body: PageBody }, { Body: MetaBody }] =
           await Promise.all([
-            this.getS3Object(key, "html"),
-            this.getS3Object(key, isJson ? "json" : "rsc"),
-            this.getS3Object(key, "meta"),
+            this.getS3Object(key, "html", keys),
+            this.getS3Object(key, isJson ? "json" : "rsc", keys),
+            this.getS3Object(key, "meta", keys),
           ]);
         const lastModified = await this.getHasRevalidatedTags(
           key,
@@ -234,7 +238,11 @@ export default class S3Cache {
     if (keys.includes(this.buildS3Key(key, "redirect"))) {
       debug("get redirect cache", { key });
       try {
-        const { Body, LastModified } = await this.getS3Object(key, "redirect");
+        const { Body, LastModified } = await this.getS3Object(
+          key,
+          "redirect",
+          keys,
+        );
         return {
           lastModified: LastModified?.getTime(),
           value: JSON.parse((await Body?.transformToString()) ?? "{}"),
@@ -404,23 +412,27 @@ export default class S3Cache {
   }
 
   private async batchWriteDynamoItem(req: { path: string; tag: string }[]) {
-    await Promise.all(
-      this.chunkArray(req, 25).map((Items) => {
-        return this.dynamoClient.send(
-          new BatchWriteItemCommand({
-            RequestItems: {
-              [CACHE_DYNAMO_TABLE ?? ""]: Items.map((Item) => ({
-                PutRequest: {
-                  Item: {
-                    ...this.buildDynamoObject(Item.path, Item.tag),
+    try {
+      await Promise.all(
+        this.chunkArray(req, 25).map((Items) => {
+          return this.dynamoClient.send(
+            new BatchWriteItemCommand({
+              RequestItems: {
+                [CACHE_DYNAMO_TABLE ?? ""]: Items.map((Item) => ({
+                  PutRequest: {
+                    Item: {
+                      ...this.buildDynamoObject(Item.path, Item.tag),
+                    },
                   },
-                },
-              })),
-            },
-          }),
-        );
-      }),
-    );
+                })),
+              },
+            }),
+          );
+        }),
+      );
+    } catch (e) {
+      error("Failed to batch write dynamo item", e);
+    }
   }
 
   private buildDynamoKey(key: string) {
@@ -469,16 +481,24 @@ export default class S3Cache {
         Prefix: `${this.buildS3KeyPrefix(key)}.`,
       }),
     );
-    return (Contents ?? []).map(({ Key }) => Key);
+    return (Contents ?? []).map(({ Key }) => Key) as string[];
   }
 
-  private getS3Object(key: string, extension: Extension) {
-    return this.client.send(
-      new GetObjectCommand({
-        Bucket: CACHE_BUCKET_NAME,
-        Key: this.buildS3Key(key, extension),
-      }),
-    );
+  private async getS3Object(key: string, extension: Extension, keys: string[]) {
+    try {
+      if (!keys.includes(this.buildS3Key(key, extension)))
+        return { Body: null, LastModified: null };
+      const result = await this.client.send(
+        new GetObjectCommand({
+          Bucket: CACHE_BUCKET_NAME,
+          Key: this.buildS3Key(key, extension),
+        }),
+      );
+      return result;
+    } catch (e) {
+      error("This error can usually be ignored : ", e);
+      return { Body: null, LastModified: null };
+    }
   }
 
   private putS3Object(

packages/open-next/src/build.ts

@@ -518,14 +518,17 @@ async function createServerBundle(monorepoRoot: string) {
     compareSemver(options.nextVersion, "13.4.13") >= 0
       ? [
           openNextPlugin({
+            name: "opennext-13.4.13-serverHandler",
             target: /plugins\/serverHandler\.js/g,
             replacements: ["./serverHandler.replacement.js"],
           }),
           openNextPlugin({
+            name: "opennext-13.4.13-util",
             target: /plugins\/util\.js/g,
             replacements: ["./util.replacement.js"],
           }),
           openNextPlugin({
+            name: "opennext-13.4.13-default",
             target: /plugins\/routing\/default\.js/g,
             replacements: ["./default.replacement.js"],
           }),
@@ -535,18 +538,17 @@ async function createServerBundle(monorepoRoot: string) {
   if (compareSemver(options.nextVersion, "13.5.1") >= 0) {
     plugins = [
       openNextPlugin({
+        name: "opennext-13.5-serverHandler",
         target: /plugins\/serverHandler\.js/g,
         replacements: ["./13.5/serverHandler.js"],
       }),
       openNextPlugin({
+        name: "opennext-13.5-util",
         target: /plugins\/util\.js/g,
-        replacements: ["./13.5/util.js"],
-      }),
-      openNextPlugin({
-        target: /plugins\/util\.js/g,
-        replacements: ["./util.replacement.js"],
+        replacements: ["./13.5/util.js", "./util.replacement.js"],
       }),
       openNextPlugin({
+        name: "opennext-13.5-default",
         target: /plugins\/routing\/default\.js/g,
         replacements: ["./default.replacement.js"],
       }),

packages/open-next/src/plugin.ts

@@ -6,6 +6,7 @@ import { Plugin } from "esbuild";
 export interface IPluginSettings {
   target: RegExp;
   replacements: string[];
+  name?: string;
 }
 
 const overridePattern = /\/\/#override (\w+)\n([\s\S]*?)\n\/\/#endOverride/gm;
@@ -46,9 +47,10 @@ const importPattern = /\/\/#import([\s\S]*?)\n\/\/#endImport/gm;
 export default function openNextPlugin({
   target,
   replacements,
+  name,
 }: IPluginSettings): Plugin {
   return {
-    name: "opennext",
+    name: name ?? "opennext",
     setup(build) {
       build.onLoad({ filter: target }, async (args) => {
         let contents = await readFile(args.path, "utf-8");
@@ -70,6 +72,9 @@ export default function openNextPlugin({
               const pattern = new RegExp(
                 `\/\/#override (${id})\n([\\s\\S]*?)\n\/\/#endOverride`,
               );
+              console.log(
+                `Open-next plugin ${name} -- Applying override for ${id} from ${fp}`,
+              );
               contents = contents.replace(pattern, replacement);
             }
           }),

packages/tests-e2e/tests/appRouter/isr.revalidate.test.ts

@@ -4,7 +4,10 @@ test("Test revalidate", async ({ request }) => {
   const result = await request.get("/api/isr");
 
   expect(result.status()).toEqual(200);
-  const body = await result.json();
+  const json = await result.json();
+  const body = json.body;
+
+  expect(json.status).toEqual(200);
   expect(body.result).toEqual(true);
   expect(body.cacheControl).toEqual(
     "private, no-cache, no-store, max-age=0, must-revalidate",