diff --git a/.changeset/gold-papayas-grin.md b/.changeset/gold-papayas-grin.md new file mode 100644 index 0000000000..1506c3908b --- /dev/null +++ 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 index 4e2e5b5349..7efcfa9c76 100644 --- a/packages/hydrogen/src/foundation/useQuery/hooks.ts +++ b/packages/hydrogen/src/foundation/useQuery/hooks.ts @@ -50,7 +50,7 @@ export function useQuery( /** A string or array to uniquely identify the current query. */ key: QueryKey, /** An asynchronous query function like `fetch` which returns data. */ - queryFn: () => Promise, + queryFn: (request: HydrogenRequest) => Promise, /** The options to manage the cache behavior of the sub-request. */ queryOptions?: HydrogenUseQueryOptions ) { @@ -98,7 +98,7 @@ export function shouldPreloadQuery( function cachedQueryFnBuilder( key: QueryKey, - generateNewOutput: () => Promise, + generateNewOutput: (request: HydrogenRequest) => Promise, queryOptions?: HydrogenUseQueryOptions ) { const resolvedQueryOptions = { @@ -144,7 +144,7 @@ function cachedQueryFnBuilder( ); 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( 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 index 48f5cccea4..741e842368 100644 --- a/packages/hydrogen/src/hooks/useShopQuery/hooks.ts +++ b/packages/hydrogen/src/hooks/useShopQuery/hooks.ts @@ -1,3 +1,4 @@ +/* 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'; @@ -5,10 +6,11 @@ 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 { /** The data returned by the query. */ @@ -18,14 +20,8 @@ export interface UseShopQueryResponse { // 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({ ); } - 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 | 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({ } } - /** - * 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({ 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 = { @@ -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 index e064472c79..7afbf38b1a 100644 --- 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 index 6ff2c441fe..5d3f7ce3c4 100644 --- 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', () => {