From d86c9e3cb9436ba18c66931151a130afd9bfe6bf Mon Sep 17 00:00:00 2001 From: Guilherme Carvalho Date: Tue, 14 May 2024 14:49:52 -0300 Subject: [PATCH 1/6] Fetch multiple CMS template pages instead of just the first Currently, when trying to load contentTypes from the Headless CMS we just fetch the first page and ignore the hasNextPage response from the cmsClient. This PR changes that to load all publishedContentTypes before returning. This might be troublesome if a given account has thousands of content types of the same type (PLP, for instance). We can either increase the page size to make fewer requests or investigate if we can move the filtering to the CMS Client. --- packages/core/src/server/cms/index.ts | 30 ++++++++-- packages/core/test/server/cms/index.test.ts | 65 +++++++++++++++++++++ 2 files changed, 91 insertions(+), 4 deletions(-) create mode 100644 packages/core/test/server/cms/index.test.ts diff --git a/packages/core/src/server/cms/index.ts b/packages/core/src/server/cms/index.ts index 56caa70640..ebea814442 100644 --- a/packages/core/src/server/cms/index.ts +++ b/packages/core/src/server/cms/index.ts @@ -45,10 +45,32 @@ export const clientCMS = new ClientCMS({ tenant: config.api.storeId, }) -export const getCMSPage = async (options: Options) => { - return await (isLocator(options) - ? clientCMS.getCMSPage(options).then((page) => ({ data: [page] })) - : clientCMS.getCMSPagesByContentType(options.contentType, options.filters)) +export const getCMSPage = async ( + options: Options, + cmsClient: ClientCMS = clientCMS +) => { + if (isLocator(options)) { + return await cmsClient + .getCMSPage(options) + .then((page) => ({ data: [page] })) + } + + const pages = [] + let page = 1 + let hasNextPage = true + + while (hasNextPage) { + const response = await cmsClient.getCMSPagesByContentType( + options.contentType, + { ...options.filters, page: page } + ) + + page = page + 1 + hasNextPage = response.hasNextPage + pages.push(...response.data) + } + + return { data: pages } } export const getPage = async (options: Options) => { diff --git a/packages/core/test/server/cms/index.test.ts b/packages/core/test/server/cms/index.test.ts new file mode 100644 index 0000000000..f247c360a5 --- /dev/null +++ b/packages/core/test/server/cms/index.test.ts @@ -0,0 +1,65 @@ +import { clientCMS, getCMSPage } from '../../../src/server/cms' +import { jest } from '@jest/globals' +import { ContentData } from '@vtex/client-cms' + +describe('CMS Integration', () => { + const mockData = (count = 1) => { + const data: ContentData[] = [] + for (let i = 0; i < count; i = i + 1) { + data.push({ + id: `data-id-${i}`, + name: `data-name-${i}`, + status: `data-status-${i}`, + type: `data-type-${i}`, + sections: [], + releaseId: `release-${i}`, + }) + } + return data + } + + describe('getCMSPage', () => { + it('returns the first page if there is only one page', async () => { + const mockFunction = jest.fn(() => { + return Promise.resolve({ + data: mockData(3), + hasNextPage: false, + totalItems: 3, + }) + }) + clientCMS.getCMSPagesByContentType = mockFunction + + const result = await getCMSPage({ contentType: 'plp' }, clientCMS) + + expect(mockFunction.mock.calls.length).toBe(1) + expect(result.data.length).toBe(3) + }) + + it('loads multiple pages', async () => { + const mockFunction: jest.Mock = + jest.fn() + + mockFunction.mockImplementationOnce(() => { + return Promise.resolve({ + data: mockData(10), + hasNextPage: true, + totalItems: 15, + }) + }) + mockFunction.mockImplementationOnce(() => { + return Promise.resolve({ + data: mockData(5), + hasNextPage: false, + totalItems: 15, + }) + }) + + clientCMS.getCMSPagesByContentType = mockFunction + + const result = await getCMSPage({ contentType: 'plp' }, clientCMS) + + expect(mockFunction.mock.calls.length).toBe(2) + expect(result.data.length).toBe(15) + }) + }) +}) From faef2e110b17023ec2b4d1aad060f004f1fd1eed Mon Sep 17 00:00:00 2001 From: Guilherme Carvalho Date: Wed, 15 May 2024 08:45:44 -0300 Subject: [PATCH 2/6] Bonus content: upgrade declared version of graphql to 15.6.0 --- packages/core/package.json | 2 +- yarn.lock | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/core/package.json b/packages/core/package.json index c9d87597c0..c2aa63de3e 100644 --- a/packages/core/package.json +++ b/packages/core/package.json @@ -61,7 +61,7 @@ "css-loader": "^6.7.1", "deepmerge": "^4.3.1", "draftjs-to-html": "^0.9.1", - "graphql": "^15.0.0", + "graphql": "^15.6.0", "include-media": "^1.4.10", "next": "^13.5.6", "next-seo": "^6.4.0", diff --git a/yarn.lock b/yarn.lock index 413a6dfe5a..9b022df6a8 100644 --- a/yarn.lock +++ b/yarn.lock @@ -9146,7 +9146,7 @@ graphql-ws@5.12.1: resolved "https://registry.yarnpkg.com/graphql-ws/-/graphql-ws-5.12.1.tgz#c62d5ac54dbd409cc6520b0b39de374b3d59d0dd" integrity sha512-umt4f5NnMK46ChM2coO36PTFhHouBrK9stWWBczERguwYrGnPNxJ9dimU6IyOBfOkC6Izhkg4H8+F51W/8CYDg== -graphql@^15.0.0, graphql@^15.6.0: +graphql@^15.6.0: version "15.8.0" resolved "https://registry.yarnpkg.com/graphql/-/graphql-15.8.0.tgz#33410e96b012fa3bdb1091cc99a94769db212b38" integrity sha512-5gghUc24tP9HRznNpV2+FIoq3xKkj5dTQqf4v0CpdPbFVwFkWoxOM+o+2OC9ZSvjEMTjfmG9QT+gcvggTwW1zw== From 395cbdd1ad7abfbf4d2be912d95af709ff7b062b Mon Sep 17 00:00:00 2001 From: Guilherme Carvalho Date: Wed, 15 May 2024 19:24:52 -0300 Subject: [PATCH 3/6] Fetch pages concurrently --- packages/core/src/server/cms/index.ts | 35 ++++++++++++++++++++------- 1 file changed, 26 insertions(+), 9 deletions(-) diff --git a/packages/core/src/server/cms/index.ts b/packages/core/src/server/cms/index.ts index ebea814442..4cdf774313 100644 --- a/packages/core/src/server/cms/index.ts +++ b/packages/core/src/server/cms/index.ts @@ -57,17 +57,34 @@ export const getCMSPage = async ( const pages = [] let page = 1 - let hasNextPage = true - - while (hasNextPage) { - const response = await cmsClient.getCMSPagesByContentType( - options.contentType, - { ...options.filters, page: page } + const perPage = 10 + const response = await cmsClient.getCMSPagesByContentType( + options.contentType, + { ...options.filters, page: page, perPage } + ) + + pages.push(...response.data) + + const totalPagesToFetch = Math.ceil(response.totalItems / perPage) // How many pages have content + const pagesToFetch = Array.from( + { length: totalPagesToFetch - 1 }, // We want all those pages minus the first one that we fetched + (_, i) => i + 2 // + 1 because indices are 0 based, and + 1 because we already fetched the first + ) + + if (response.totalItems > pages.length) { + const restOfPages = await Promise.all( + pagesToFetch.map((i) => + cmsClient.getCMSPagesByContentType(options.contentType, { + ...options.filters, + page: i, + perPage, + }) + ) ) - page = page + 1 - hasNextPage = response.hasNextPage - pages.push(...response.data) + restOfPages.forEach((response) => { + pages.push(...response.data) + }) } return { data: pages } From d7af4ac2fdcb1d81ab70a12755bddb59d8197fe2 Mon Sep 17 00:00:00 2001 From: Guilherme Carvalho Date: Wed, 15 May 2024 19:47:59 -0300 Subject: [PATCH 4/6] Add in-memory caching --- packages/core/src/server/cms/index.ts | 74 ++++++++++++++++----------- 1 file changed, 45 insertions(+), 29 deletions(-) diff --git a/packages/core/src/server/cms/index.ts b/packages/core/src/server/cms/index.ts index 4cdf774313..59cb5d42d5 100644 --- a/packages/core/src/server/cms/index.ts +++ b/packages/core/src/server/cms/index.ts @@ -45,6 +45,19 @@ export const clientCMS = new ClientCMS({ tenant: config.api.storeId, }) +/* + * This in memory cache exists because for each page (think category or department) + * we are fetching all the pages of the same content type from the headless CMS to + * find the one that matches the slug. + * + * So instead of making multiple request for the Headless CMS API for each page we make + * one for each content-type and reuse the results for the next page. + * + * Since we rebuild on a CMS publication the server will go away and will "invalidate" + * the cache + */ +const getCMSPageCache = {} + export const getCMSPage = async ( options: Options, cmsClient: ClientCMS = clientCMS @@ -55,39 +68,42 @@ export const getCMSPage = async ( .then((page) => ({ data: [page] })) } - const pages = [] - let page = 1 - const perPage = 10 - const response = await cmsClient.getCMSPagesByContentType( - options.contentType, - { ...options.filters, page: page, perPage } - ) - - pages.push(...response.data) - - const totalPagesToFetch = Math.ceil(response.totalItems / perPage) // How many pages have content - const pagesToFetch = Array.from( - { length: totalPagesToFetch - 1 }, // We want all those pages minus the first one that we fetched - (_, i) => i + 2 // + 1 because indices are 0 based, and + 1 because we already fetched the first - ) - - if (response.totalItems > pages.length) { - const restOfPages = await Promise.all( - pagesToFetch.map((i) => - cmsClient.getCMSPagesByContentType(options.contentType, { - ...options.filters, - page: i, - perPage, - }) - ) + if (!getCMSPageCache[options.contentType]) { + const pages = [] + let page = 1 + const perPage = 10 + const response = await cmsClient.getCMSPagesByContentType( + options.contentType, + { ...options.filters, page: page, perPage } + ) + + pages.push(...response.data) + + const totalPagesToFetch = Math.ceil(response.totalItems / perPage) // How many pages have content + const pagesToFetch = Array.from( + { length: totalPagesToFetch - 1 }, // We want all those pages minus the first one that we fetched + (_, i) => i + 2 // + 1 because indices are 0 based, and + 1 because we already fetched the first ) - restOfPages.forEach((response) => { - pages.push(...response.data) - }) + if (response.totalItems > pages.length) { + const restOfPages = await Promise.all( + pagesToFetch.map((i) => + cmsClient.getCMSPagesByContentType(options.contentType, { + ...options.filters, + page: i, + perPage, + }) + ) + ) + + restOfPages.forEach((response) => { + pages.push(...response.data) + }) + } + getCMSPageCache[options.contentType] = { data: pages } } - return { data: pages } + return getCMSPageCache[options.contentType] } export const getPage = async (options: Options) => { From c10deeda0b8792eca5a6343f137ffa360e449c73 Mon Sep 17 00:00:00 2001 From: Guilherme Carvalho Date: Thu, 16 May 2024 14:30:49 -0300 Subject: [PATCH 5/6] Add tests to test caching behavior as well --- packages/core/src/server/cms/index.ts | 19 +++++++++--- packages/core/test/server/cms/index.test.ts | 34 +++++++++++++++++++-- 2 files changed, 47 insertions(+), 6 deletions(-) diff --git a/packages/core/src/server/cms/index.ts b/packages/core/src/server/cms/index.ts index 59cb5d42d5..3d5033fb82 100644 --- a/packages/core/src/server/cms/index.ts +++ b/packages/core/src/server/cms/index.ts @@ -5,6 +5,14 @@ import MissingContentError from 'src/sdk/error/MissingContentError' import MultipleContentError from 'src/sdk/error/MultipleContentError' import config from '../../../faststore.config' +type Cache = { + [key: string]: { data: Array } +} +type ExtraOptions = { + cmsClient?: ClientCMS + cache?: Cache +} + export type Options = | Locator | { @@ -60,15 +68,18 @@ const getCMSPageCache = {} export const getCMSPage = async ( options: Options, - cmsClient: ClientCMS = clientCMS + extraOptions?: ExtraOptions ) => { + const cmsClient = extraOptions?.cmsClient ?? clientCMS + const cache = extraOptions.cache ?? getCMSPageCache + if (isLocator(options)) { return await cmsClient .getCMSPage(options) .then((page) => ({ data: [page] })) } - if (!getCMSPageCache[options.contentType]) { + if (!cache[options.contentType]) { const pages = [] let page = 1 const perPage = 10 @@ -100,10 +111,10 @@ export const getCMSPage = async ( pages.push(...response.data) }) } - getCMSPageCache[options.contentType] = { data: pages } + cache[options.contentType] = { data: pages } } - return getCMSPageCache[options.contentType] + return cache[options.contentType] } export const getPage = async (options: Options) => { diff --git a/packages/core/test/server/cms/index.test.ts b/packages/core/test/server/cms/index.test.ts index f247c360a5..d1a890e7ea 100644 --- a/packages/core/test/server/cms/index.test.ts +++ b/packages/core/test/server/cms/index.test.ts @@ -29,7 +29,10 @@ describe('CMS Integration', () => { }) clientCMS.getCMSPagesByContentType = mockFunction - const result = await getCMSPage({ contentType: 'plp' }, clientCMS) + const result = await getCMSPage( + { contentType: 'plp' }, + { cmsClient: clientCMS } + ) expect(mockFunction.mock.calls.length).toBe(1) expect(result.data.length).toBe(3) @@ -56,10 +59,37 @@ describe('CMS Integration', () => { clientCMS.getCMSPagesByContentType = mockFunction - const result = await getCMSPage({ contentType: 'plp' }, clientCMS) + const result = await getCMSPage( + { contentType: 'plp' }, + { cmsClient: clientCMS, cache: {} } + ) expect(mockFunction.mock.calls.length).toBe(2) expect(result.data.length).toBe(15) }) + + it('it makes no request if the cache is filled', async () => { + const mockFunction: jest.Mock = + jest.fn() + + mockFunction.mockImplementationOnce(() => { + return Promise.resolve({ + data: mockData(10), + hasNextPage: true, + totalItems: 15, + }) + }) + + clientCMS.getCMSPagesByContentType = mockFunction + + const cache = { plp: { data: [] } } + const result = await getCMSPage( + { contentType: 'plp' }, + { cmsClient: clientCMS, cache: cache } + ) + + expect(mockFunction.mock.calls.length).toBe(0) + expect(result.data.length).toBe(0) + }) }) }) From 45e82da5bb5c25bbd39fffbb7d76f04f702e97e2 Mon Sep 17 00:00:00 2001 From: Guilherme Carvalho Date: Thu, 16 May 2024 14:35:56 -0300 Subject: [PATCH 6/6] Add missing `?` --- packages/core/src/server/cms/index.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/core/src/server/cms/index.ts b/packages/core/src/server/cms/index.ts index 3d5033fb82..1a52f62bc5 100644 --- a/packages/core/src/server/cms/index.ts +++ b/packages/core/src/server/cms/index.ts @@ -71,7 +71,7 @@ export const getCMSPage = async ( extraOptions?: ExtraOptions ) => { const cmsClient = extraOptions?.cmsClient ?? clientCMS - const cache = extraOptions.cache ?? getCMSPageCache + const cache = extraOptions?.cache ?? getCMSPageCache if (isLocator(options)) { return await cmsClient