feat: Remove caching on fetching product inventory data #2801
Conversation
🦋 Changeset detectedLatest commit: b329bc6 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
1 Skipped Deployment
|
| "@bigcommerce/catalyst-core": minor | ||
| --- | ||
|
|
||
| Fetch product inventory data with a separate GQL query with no caching |
There was a problem hiding this comment.
Is the rationale for marking this as a minor the impact it could have on performance and/or costs for the PDP?
This is the primary question that I have when introducing queries that aren't cached. Are we good with every single PDP load invoking the API? Are we certain merchants won't run into rate limits?
| variables, | ||
| customerAccessToken, | ||
| fetchOptions: customerAccessToken ? { cache: 'no-store' } : { next: { revalidate } }, | ||
| fetchOptions: { cache: 'no-store' }, |
There was a problem hiding this comment.
This seems to be removing caching from the existing query for product variants. Was this intentional? If the query is specifically for variant inventory quantities, can we update the query name to be explicit about that.
| variables, | ||
| customerAccessToken, | ||
| fetchOptions: customerAccessToken ? { cache: 'no-store' } : { next: { revalidate } }, | ||
| fetchOptions: { cache: 'no-store' }, |
There was a problem hiding this comment.
Is no-store the appropriate value here or would a very small revalidation period be desirable? There's is always a chance for there to be stale inventory data, even if it's always fetched without caching since there could be race conditions when loading the page. This could be addressed with polling, but I question whether that's a good approach. Instead, a warning when stock is very low might be good enough to let the user know that the item product might be out of stock soon.
| const streamableProductVariant = Streamable.from(async () => { | ||
| const product = await streamableProduct; | ||
| const product = await streamableProductInventory; |
There was a problem hiding this comment.
Shouldn't the variable name change if this is specifically tracking product inventory?
| if (!product.inventory.hasVariantInventory) { | ||
| return undefined; | ||
| } | ||
|
|
||
| const variables = { | ||
| productId, | ||
| sku: product.sku, | ||
| }; | ||
|
|
||
| const variants = await getStreamableProductVariant(variables, customerAccessToken); |
There was a problem hiding this comment.
If the result here is coupled to both queries and we don't use getStreamableProductVariant elsewhere then we should combine this all into a single query so that we have all the data we need in a single query.
What/Why?
Changes in a product inventory does not reflect right away on product details page due GQL caching in Catalyst. To fix this issue, we will fetch the product/variant inventory data separately with no caching.
Testing
Screen record
https://github.com/user-attachments/assets/e060094d-c076-49c9-86e6-cc0797c1aac1
Migration
Rebase