Fix preload cache ballooning (#2316)

* Add console debugger to check on ballooning size of preload queries * Don't shard our preload cache keys by buyer IP * Remove unused import * Add some more logging * Run `useQuery` with the current `request` context * Remove extra console log * Add changeset
Shopify · Nov 15, 2022 · 20e5b79 · 20e5b79
1 parent 03ecf64
commit 20e5b79
Show file tree

Hide file tree

Showing 5 changed files with 107 additions and 93 deletions.
diff --git a/.changeset/gold-papayas-grin.md b/.changeset/gold-papayas-grin.md
@@ -0,0 +1,5 @@
+---
+'@shopify/hydrogen': patch
+---
+
+Fix issue with preload cache which would cause problems when scaled to large amounts of traffic in production.
diff --git a/packages/hydrogen/src/foundation/useQuery/hooks.ts b/packages/hydrogen/src/foundation/useQuery/hooks.ts
@@ -50,7 +50,7 @@ export function useQuery<T>(
   /** A string or array to uniquely identify the current query. */
   key: QueryKey,
   /** An asynchronous query function like `fetch` which returns data. */
-  queryFn: () => Promise<T>,
+  queryFn: (request: HydrogenRequest) => Promise<T>,
   /** The options to manage the cache behavior of the sub-request. */
   queryOptions?: HydrogenUseQueryOptions
 ) {
@@ -98,7 +98,7 @@ export function shouldPreloadQuery(
 
 function cachedQueryFnBuilder<T>(
   key: QueryKey,
-  generateNewOutput: () => Promise<T>,
+  generateNewOutput: (request: HydrogenRequest) => Promise<T>,
   queryOptions?: HydrogenUseQueryOptions
 ) {
   const resolvedQueryOptions = {
@@ -144,7 +144,7 @@ function cachedQueryFnBuilder<T>(
             );
 
             try {
-              const output = await generateNewOutput();
+              const output = await generateNewOutput(request);
 
               if (shouldCacheResponse(output)) {
                 await setItemInCache(key, output, resolvedQueryOptions?.cache);
@@ -164,7 +164,7 @@ function cachedQueryFnBuilder<T>(
       return output;
     }
 
-    const newOutput = await generateNewOutput();
+    const newOutput = await generateNewOutput(request);
 
     /**
      * Important: Do this async

diff --git a/packages/hydrogen/src/hooks/useShopQuery/hooks.ts b/packages/hydrogen/src/hooks/useShopQuery/hooks.ts
@@ -1,14 +1,16 @@
+/* eslint-disable react-hooks/rules-of-hooks */
 import {useShop} from '../../foundation/useShop/index.js';
 import {getLoggerWithContext} from '../../utilities/log/index.js';
 import type {CachingStrategy, PreloadOptions} from '../../types.js';
 import {graphqlRequestBody} from '../../utilities/index.js';
 import {useServerRequest} from '../../foundation/ServerRequestProvider/index.js';
 import {injectGraphQLTracker} from '../../utilities/graphql-tracker.js';
 import {sendMessageToClient} from '../../utilities/devtools.js';
-import {fetchSync} from '../../foundation/fetchSync/server/fetchSync.js';
 import {META_ENV_SSR} from '../../foundation/ssr-interop.js';
 import {getStorefrontApiRequestHeaders} from '../../utilities/storefrontApi.js';
 import {parseJSON} from '../../utilities/parse.js';
+import {useQuery} from '../../foundation/useQuery/hooks.js';
+import {HydrogenRequest} from '../../foundation/HydrogenRequest/HydrogenRequest.server.js';
 
 export interface UseShopQueryResponse<T> {
   /** The data returned by the query. */
@@ -18,14 +20,8 @@ export interface UseShopQueryResponse<T> {
 
 // Check if the response body has GraphQL errors
 // https://spec.graphql.org/June2018/#sec-Response-Format
-const shouldCacheResponse = ([body]: [any, Response]) => {
-  try {
-    return !parseJSON(body)?.errors;
-  } catch {
-    // If we can't parse the response, then assume
-    // an error and don't cache the response
-    return false;
-  }
+const shouldCacheResponse = (body: any) => {
+  return !body?.errors;
 };
 
 /**
@@ -68,61 +64,91 @@ export function useShopQuery<T>({
     );
   }
 
-  const serverRequest = useServerRequest(); // eslint-disable-line react-hooks/rules-of-hooks
+  const serverRequest = useServerRequest();
   const log = getLoggerWithContext(serverRequest);
 
+  const {
+    storeDomain,
+    storefrontApiVersion,
+    storefrontToken,
+    storefrontId,
+    privateStorefrontToken,
+  } = useShop();
+
   const body = query ? graphqlRequestBody(query, variables) : '';
-  const {url, requestInit} = useCreateShopRequest(body); // eslint-disable-line react-hooks/rules-of-hooks
 
-  let text: string;
-  let data: any;
-  let useQueryError: any;
-  let response: ReturnType<typeof fetchSync> | null = null;
+  const {data, error} = useQuery(
+    [storeDomain, storefrontApiVersion, body],
+    async (request) => {
+      const {url, requestInit} = useCreateShopRequest({
+        body,
+        request,
+        storeDomain,
+        storefrontToken,
+        storefrontApiVersion,
+        storefrontId,
+        privateStorefrontToken,
+      });
+      const response = await fetch(url, requestInit);
+      const text = await response.text();
 
-  try {
-    response = fetchSync(url, {
-      ...requestInit,
-      cache,
-      preload,
-      shouldCacheResponse,
-    });
+      try {
+        const data = parseJSON(text);
 
-    text = response.text();
+        /**
+         * GraphQL errors get printed to the console but ultimately
+         * get returned to the consumer.
+         */
+        if (data?.errors) {
+          const errors = Array.isArray(data.errors)
+            ? data.errors
+            : [data.errors];
+          const requestId = response?.headers?.get('x-request-id') ?? '';
+          for (const error of errors) {
+            if (__HYDROGEN_DEV__ && !__HYDROGEN_TEST__) {
+              throw new Error(
+                `Storefront API GraphQL Error: ${error.message}.\nRequest id: ${requestId}`
+              );
+            } else {
+              log.error(
+                'Storefront API GraphQL Error',
+                error,
+                'Storefront API GraphQL request id',
+                requestId
+              );
+            }
+          }
+          log.error(`Storefront API GraphQL error count: ${errors.length}`);
+        }
 
-    try {
-      data = response.json();
-    } catch (error: any) {
-      if (response.headers.get('content-length')) {
-        useQueryError = new Error(
-          `Unable to parse response (x-request-id: ${response.headers.get(
-            'x-request-id'
-          )}):\n${text}`
-        );
-      } else {
-        useQueryError = new Error(
-          `${response.status} ${
-            response.statusText
-          } (x-request-id: ${response.headers.get('x-request-id')})`
-        );
+        return data;
+      } catch (error: any) {
+        if (response.headers.get('content-length')) {
+          throw new Error(
+            `Unable to parse response (x-request-id: ${response.headers.get(
+              'x-request-id'
+            )}):\n${text}`
+          );
+        } else {
+          throw new Error(
+            `${response.status} ${
+              response.statusText
+            } (x-request-id: ${response.headers.get('x-request-id')})`
+          );
+        }
       }
-    }
-  } catch (error: any) {
-    // Pass-through thrown promise for Suspense functionality
-    if (error?.then) {
-      throw error;
-    }
-
-    useQueryError = error;
-  }
+    },
+    {cache, preload, shouldCacheResponse}
+  );
 
   /**
    * The fetch request itself failed, so we handle that differently than a GraphQL error
    */
-  if (useQueryError) {
-    const errorMessage = createErrorMessage(useQueryError);
+  if (error) {
+    const errorMessage = createErrorMessage(error);
 
     log.error(errorMessage);
-    log.error(useQueryError);
+    log.error(error);
 
     if (__HYDROGEN_DEV__ && !__HYDROGEN_TEST__) {
       throw new Error(errorMessage);
@@ -134,30 +160,6 @@ export function useShopQuery<T>({
     }
   }
 
-  /**
-   * GraphQL errors get printed to the console but ultimately
-   * get returned to the consumer.
-   */
-  if (data?.errors) {
-    const errors = Array.isArray(data.errors) ? data.errors : [data.errors];
-    const requestId = response?.headers?.get('x-request-id') ?? '';
-    for (const error of errors) {
-      if (__HYDROGEN_DEV__ && !__HYDROGEN_TEST__) {
-        throw new Error(
-          `Storefront API GraphQL Error: ${error.message}.\nRequest id: ${requestId}`
-        );
-      } else {
-        log.error(
-          'Storefront API GraphQL Error',
-          error,
-          'Storefront API GraphQL request id',
-          requestId
-        );
-      }
-    }
-    log.error(`Storefront API GraphQL error count: ${errors.length}`);
-  }
-
   if (
     __HYDROGEN_DEV__ &&
     (log.options().showUnusedQueryProperties ||
@@ -212,16 +214,23 @@ export function useShopQuery<T>({
   return data!;
 }
 
-function useCreateShopRequest(body: string) {
-  const {
-    storeDomain,
-    storefrontToken,
-    storefrontApiVersion,
-    storefrontId,
-    privateStorefrontToken,
-  } = useShop();
-
-  const request = useServerRequest();
+function useCreateShopRequest({
+  body,
+  request,
+  storeDomain,
+  storefrontToken,
+  storefrontApiVersion,
+  storefrontId,
+  privateStorefrontToken,
+}: {
+  body: string;
+  request: HydrogenRequest;
+  storeDomain: string;
+  storefrontToken: string;
+  storefrontApiVersion: string;
+  storefrontId?: string;
+  privateStorefrontToken?: string;
+}) {
   const buyerIp = request.getBuyerIp();
 
   let headers: Record<string, string> = {
@@ -266,3 +275,5 @@ function createErrorMessage(fetchError: Response | Error) {
     return `Failed to connect to the Storefront API: ${fetchError.message}`;
   }
 }
+
+/* eslint-enable react-hooks/rules-of-hooks */
diff --git a/packages/hydrogen/src/utilities/hash.ts b/packages/hydrogen/src/utilities/hash.ts
@@ -1,4 +1,3 @@
-import {STOREFRONT_API_BUYER_IP_HEADER} from '../constants.js';
 import type {QueryKey} from '../types.js';
 
 export function hashKey(queryKey: QueryKey): string {
@@ -16,9 +15,6 @@ export function hashKey(queryKey: QueryKey): string {
         // fallback to a safer (but slower) stringify.
         if (!!key.body && typeof key.body === 'string') {
           hash += key.body;
-          if (!!key.headers && key.headers[STOREFRONT_API_BUYER_IP_HEADER]) {
-            hash += key.headers[STOREFRONT_API_BUYER_IP_HEADER];
-          }
         } else {
           hash += JSON.stringify(key);
         }

diff --git a/packages/hydrogen/src/utilities/tests/hash.vitest.ts b/packages/hydrogen/src/utilities/tests/hash.vitest.ts
@@ -18,14 +18,16 @@ describe('Hash key for subrequests', () => {
     expect(
       hashKey(['hello', 'world', {body: 'longquery', headers: {}}])
     ).toEqual('helloworldlongquery');
+  });
 
+  it('does not include any buyer-specific information in hashes', () => {
     expect(
       hashKey([
         'hello',
         'world',
         {body: 'longquery', headers: {[STOREFRONT_API_BUYER_IP_HEADER]: '42'}},
       ])
-    ).toEqual('helloworldlongquery42');
+    ).toEqual('helloworldlongquery');
   });
 
   it('does not choke on other types', () => {