fix: use provided cartId in cartGetDefault before falling back to getCartId#3664
fix: use provided cartId in cartGetDefault before falling back to getCartId#3664Vitalini wants to merge 3 commits intoShopify:mainfrom
Conversation
…CartId The cartGetDefault function ignored the cartId passed via cartInput and always used getCartId(). This made it impossible to query a specific cart by its ID. Use nullish coalescing to prefer cartInput.cartId when provided, falling back to getCartId() otherwise. Fixes Shopify#3639
|
I have signed the CLA! |
fredericoo
left a comment
There was a problem hiding this comment.
nice fix - the bug is real and the one-liner is the right approach. A couple of things needed before this is merge-ready.
blocking: changes to packages/*/src/** require a changeset. This is a bug fix to @shopify/hydrogen, so we need a patch bump changeset for hydrogen.
Something like:
---
"@shopify/hydrogen": patch
---
Fix `cart.get()` to use provided `cartId` before falling back to `getCartId()`Run npx changeset add or create the file manually.
| }: CartGetOptions): CartGetFunction { | ||
| return async (cartInput?: CartGetProps) => { | ||
| const cartId = getCartId(); | ||
| const cartId = cartInput?.cartId ?? getCartId(); |
There was a problem hiding this comment.
blocking: Re: cartGetDefault.test.ts:45-54 - the existing test "should return a cartId passed in" passes both before and after this fix. It doesn't catch the bug.
Here's why: the test sets getCartId: () => CART_ID (which returns a valid ID), then calls cartGet({cartId: 'gid://shopify/Cart/c1-456'}). Before the fix, const cartId = getCartId() resolves to CART_ID, but the query call uses {cartId, ...cartInput} where cartInput is spread after cartId. Since later properties in a spread override earlier ones, cartInput.cartId wins in the variables object regardless. So the query always got the right ID anyway.
The actual bug is: when getCartId() returns undefined but cartInput.cartId is provided, the early return if (!cartId) return null exits before the query ever runs.
Let's add a test that covers the real scenario:
it('should use provided cartId even when getCartId returns undefined', async () => {
const cartGet = cartGetDefault({
storefront: mockCreateStorefrontClient(),
getCartId: () => undefined,
});
const result = await cartGet({cartId: 'gid://shopify/Cart/c1-456'});
expect(result).not.toBeNull();
expect(result).toHaveProperty('id', 'gid://shopify/Cart/c1-456');
});This test would fail without the fix (early return on undefined) and pass with it.
| }: CartGetOptions): CartGetFunction { | ||
| return async (cartInput?: CartGetProps) => { | ||
| const cartId = getCartId(); | ||
| const cartId = cartInput?.cartId ?? getCartId(); |
There was a problem hiding this comment.
non-blocking: the three other cart functions that accept an optional cartId override use || instead of ?? (cartAttributesUpdateDefault, cartMetafieldsSetDefault, cartMetafieldDeleteDefault).
Most other cart mutation functions don't accept a cartId override at all. For a cart ID (always a non-empty string or undefined), the behaviour is identical. ?? is semantically more precise since it only falls through on null/undefined, not on all falsy values - I'd keep ?? here and consider aligning the other three in a follow-up if the team agrees.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Problem
The
cartGetDefaultfunction ignores thecartIdpassed viacartInputand always callsgetCartId(). This makes it impossible to query a specific cart by its ID when usingcart.get().hydrogen/packages/hydrogen/src/cart/queries/cartGetDefault.ts
Line 66 in fcfbcaa
Solution
Use nullish coalescing (
??) to prefercartInput.cartIdwhen provided, falling back togetCartId()otherwise.This aligns with the JSDoc on
CartGetProps.cartIdwhich states:...implying
getCartId()should be the fallback, not the only source.Fixes #3639