review fix

Author: conico974

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

@@ -3,7 +3,7 @@ import type {
   IncrementalCacheContext,
   IncrementalCacheValue,
 } from "types/cache";
-import { getTagFromValue, hasBeenRevalidated } from "utils/cache";
+import { getTagsFromValue, hasBeenRevalidated } from "utils/cache";
 import { isBinaryContentType } from "../utils/binary";
 import { debug, error, warn } from "./logger";
 
@@ -116,7 +116,7 @@ export default class Cache {
       const cacheData = cachedEntry?.value;
 
       const meta = cacheData?.meta;
-      const tags = getTagFromValue(cacheData);
+      const tags = getTagsFromValue(cacheData);
       const _lastModified = cachedEntry.lastModified ?? Date.now();
       const _hasBeenRevalidated = await hasBeenRevalidated(
         key,
@@ -295,6 +295,12 @@ export default class Cache {
             break;
         }
       }
+      if (
+        globalThis.openNextConfig.dangerous?.disableTagCache ||
+        globalThis.tagCache.mode === "nextMode"
+      ) {
+        return;
+      }
       // Write derivedTags to dynamodb
       // If we use an in house version of getDerivedTags in build we should use it here instead of next's one
       const derivedTags: string[] =

packages/open-next/src/core/routing/cacheInterceptor.ts

@@ -5,7 +5,7 @@ import type { InternalEvent, InternalResult } from "types/open-next";
 import type { CacheValue } from "types/overrides";
 import { emptyReadableStream, toReadableStream } from "utils/stream";
 
-import { getTagFromValue, hasBeenRevalidated } from "utils/cache";
+import { getTagsFromValue, hasBeenRevalidated } from "utils/cache";
 import { debug } from "../../adapters/logger";
 import { localizePath } from "./i18n";
 import { generateMessageGroupId } from "./queue";
@@ -164,7 +164,7 @@ export async function cacheInterceptor(
       }
       // We need to check the tag cache now
       if (cachedData.value?.type === "app") {
-        const tags = getTagFromValue(cachedData.value);
+        const tags = getTagsFromValue(cachedData.value);
         const _hasBeenRevalidated = await hasBeenRevalidated(
           localizedPath,
           tags,

packages/open-next/src/overrides/tagCache/constants.ts

@@ -1,5 +1,4 @@
 export const MAX_DYNAMO_BATCH_WRITE_ITEM_COUNT = 25;
-export const MAX_DYNAMO_BATCH_GET_ITEM_COUNT = 100;
 
 /**
  * Sending to dynamo X commands at a time, using about X * 25 write units per batch to not overwhelm DDB

packages/open-next/src/overrides/tagCache/ddb-nextMode.ts renamed to packages/open-next/src/overrides/tagCache/dynamodb-nextMode.ts

@@ -75,6 +75,11 @@ export default {
     if (globalThis.openNextConfig.dangerous?.disableTagCache) {
       return false;
     }
+    if (tags.length > 100) {
+      throw new RecoverableError(
+        "Cannot query more than 100 tags at once. You should not be using this tagCache implementation for this amount of tags",
+      );
+    }
     const { CACHE_DYNAMO_TABLE } = process.env;
     // It's unlikely that we will have more than 100 items to query
     // If that's the case, you should not use this tagCache implementation

packages/open-next/src/types/open-next.ts

@@ -148,8 +148,8 @@ export type IncludedIncrementalCache =
 export type IncludedTagCache =
   | "dynamodb"
   | "dynamodb-lite"
+  | "dynamodb-nextMode"
   | "fs-dev"
-  | "ddb-nextMode"
   | "dummy";
 
 export type IncludedImageLoader = "s3" | "host" | "fs-dev" | "dummy";

packages/open-next/src/types/overrides.ts

@@ -100,6 +100,27 @@ type BaseTagCache = {
   name: string;
 };
 
+/**
+ * On get :
+We have to check for every tag (after reading the incremental cache) that they have not been revalidated.
+
+In DynamoDB, this would require 1 GetItem per tag (including internal one), more realistically 1 BatchGetItem per get (In terms of pricing, it would be billed as multiple single GetItem)
+
+On set :
+We don't have to do anything here
+
+On revalidateTag for each tag :
+We have to update a single entry for this tag
+
+Pros :
+- No need to prepopulate DDB
+- Very little write
+
+Cons :
+- Might be slower on read
+- One page request (i.e. GET request) could require to check a lot of tags (And some of them multiple time when used with the fetch cache)
+- Almost impossible to do automatic cdn revalidation by itself
+*/
 export type NextModeTagCache = BaseTagCache & {
   mode: "nextMode";
   hasBeenRevalidated(tags: string[], lastModified?: number): Promise<boolean>;
@@ -109,6 +130,25 @@ export type NextModeTagCache = BaseTagCache & {
   getPathsByTags?: (tags: string[]) => Promise<string[]>;
 };
 
+/**
+ * On get :
+We just check for the cache key in the tag cache. If it has been revalidated we just return null, otherwise we continue
+
+On set :
+We have to write both the incremental cache and check the tag cache for non existing tag/key combination. For non existing tag/key combination, we have to add them
+
+On revalidateTag for each tag :
+We have to update every possible combination for the requested tag
+
+Pros :
+- Very fast on read
+- Only one query per get (On DynamoDB it's a lot cheaper)
+- Can allow for automatic cdn invalidation on revalidateTag
+
+Cons :
+- Lots of write on set and revalidateTag
+- Needs to be prepopulated at build time to work properly
+ */
 export type OriginalTagCache = BaseTagCache & {
   mode?: "original";
   getByTag(tag: string): Promise<string[]>;

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

@@ -9,11 +9,7 @@ export async function hasBeenRevalidated(
     return false;
   }
   if (globalThis.tagCache.mode === "nextMode") {
-    const hasBeenRevalidated = await globalThis.tagCache.hasBeenRevalidated(
-      tags,
-      lastModified,
-    );
-    return hasBeenRevalidated;
+    return await globalThis.tagCache.hasBeenRevalidated(tags, lastModified);
   }
   // TODO: refactor this, we should introduce a new method in the tagCache interface so that both implementations use hasBeenRevalidated
   const _lastModified = await globalThis.tagCache.getLastModified(
@@ -23,7 +19,7 @@ export async function hasBeenRevalidated(
   return _lastModified === -1;
 }
 
-export function getTagFromValue(value?: CacheValue<false>) {
+export function getTagsFromValue(value?: CacheValue<false>) {
   if (!value) {
     return [];
   }