Skip to content

fix(hydrogen): respect cartId arg in cartGetDefault#3718

Open
andromia3 wants to merge 2 commits intoShopify:mainfrom
andromia3:fix/cart-get-respects-cartid
Open

fix(hydrogen): respect cartId arg in cartGetDefault#3718
andromia3 wants to merge 2 commits intoShopify:mainfrom
andromia3:fix/cart-get-respects-cartid

Conversation

@andromia3
Copy link
Copy Markdown

Closes #3639.

Problem

`cartGetDefault` returns a function whose public type declares a `cartInput?: CartGetProps` parameter, with `cartInput.cartId` documented as "The cart ID" and `@default cart.getCartId()`. But the implementation ignored `cartInput.cartId` entirely:

```ts
return async (cartInput?: CartGetProps) => {
const cartId = getCartId(); // cartInput.cartId ignored
if (!cartId) return null; // <-- bails even when caller provided one
// ...
variables: {cartId, ...cartInput}, // spread masks it in the variables, but only if we get here
};
```

If `getCartId()` returns `undefined` (e.g. no cart cookie) and the caller passes `cartGet({cartId: 'gid://shopify/Cart/...'})`, the early return fires and the function yields `null` instead of fetching the cart the caller asked for.

Fix

One-line change: prefer the caller's `cartId` before falling back to `getCartId()`.

```diff

  • const cartId = getCartId();
  • const cartId = cartInput?.cartId ?? getCartId();
    ```

Why the existing test didn't catch it

There's already a test ("should return a cartId passed in") that passes `cartGet({cartId: '...'})`. It passes on the broken code because:

  1. `getCartId: () => CART_ID` in the test returns a value, so `if (!cartId) return null` doesn't fire.
  2. The spread `...cartInput` in the GraphQL `variables` object then overrides `cartId` with the caller's value, so the mock storefront happens to echo back the "correct" id.

The bug only manifests when `getCartId()` returns falsy — exactly the scenario in #3639.

New regression test

```ts
it('should use the cartId passed in when getCartId returns undefined', async () => {
const cartGet = cartGetDefault({
storefront: mockCreateStorefrontClient(),
getCartId: () => undefined,
});

const result = await cartGet({cartId: 'gid://shopify/Cart/c1-456'});

expect(result).toHaveProperty('id', 'gid://shopify/Cart/c1-456');
});
```

This fails on the old code (`result` is `null`) and passes after the fix.

Scope note

Other cart operations (`cartLinesAddDefault`, etc.) accept `optionalParams.cartId` via `CartOptionalInput` and use the same `{cartId: options.getCartId(), ..., ...optionalParams}` pattern, where the spread happens to override cleanly. They don't share cartGet's early-return problem, so they're left untouched in this PR.

Changeset

Patch bump for `@shopify/hydrogen` included.

The returned function declares a `cartInput?: CartGetProps` parameter
whose `cartId` field is documented as "The cart ID" with
"@default cart.getCartId()". But the implementation was calling
`getCartId()` unconditionally and then checking `if (!cartId) return null`,
so when `getCartId()` returned undefined the function bailed out with
null even though the caller had passed a valid cart id.

The existing test "should return a cartId passed in" only covered the
case where `getCartId()` returns a value. The spread of `...cartInput`
into the GraphQL variables masked the bug in that test: the early
return never fired.

Added a regression test that sets `getCartId: () => undefined` and
passes a `cartId` via the input. This test fails on the old code
(returns null) and passes after the fix.
@andromia3 andromia3 requested a review from a team as a code owner April 14, 2026 15:00
@andromia3
Copy link
Copy Markdown
Author

Follow-up commit pushed after a review pass through the full dependency path.

What I traced

  1. All usages of cartGetDefault / CartGetFunction / CartGetProps — only `createCartHandler.ts:271` consumes it (exposed as `cart.get`), plus the existing tests and the re-export from `index.ts`. No internal caller depends on the old behaviour.
  2. Full path through `createCartHandler` — `createCartHandler` wraps `getCartId` so it can return `undefined` legitimately (no cart cookie yet, no `cartCreate` called yet). This is the exact real-world scenario in ignored cartId in cartGetDefault #3639.
  3. Real-world `getCartId` — `cartGetIdDefault` in `packages/hydrogen/src/cart/cartGetIdDefault.ts` reads the `cart` cookie and returns `undefined` when absent, confirming the null-return path is reachable in normal use.
  4. Every existing test against `cart.get` — including `createCartHandler.test.ts:171` `"function get can provide overridable parameter"`, which passes the same `{cartId: '…'}` override. On the old code that test happened to work because `getCartId` was truthy (`c1-123`), so the early return didn't fire and the spread of `...cartInput` overrode `cartId` in the GraphQL variables. Traced it — still passes with both changes.

What I found during the trace

The one-line fix (`cartInput?.cartId ?? getCartId()`) doesn't cover one edge case: a caller that computes the cart id dynamically and passes `{cartId: maybeUndefined}`. In that case the local `cartId` is correctly resolved via `??`, but the `variables: {cartId, ...cartInput}` spread then overwrites `cartId` with `undefined` from the input, and the query goes out with `cartId: undefined`.

Added in the follow-up commit

  • Swap the GraphQL `variables` spread order: `{cartId, ...cartInput}` → `{...cartInput, cartId}`. The resolved local `cartId` now always wins, while `country` / `language` / `numCartLines` / `visitorConsent` still come through from `cartInput`.
  • New regression test (`"should fall back to getCartId when cartInput.cartId is explicitly undefined"`) that calls `cartGet({cartId: undefined})` and asserts the cookie cart id is returned. Fails on the old spread order, passes after the swap.
  • Changeset updated to cover both fixes.

Scope note (unchanged)

Sibling cart operations (`cartLinesAddDefault`, `cartLinesRemoveDefault`, etc.) use a similar `{cartId: options.getCartId(), ..., ...optionalParams}` pattern, but none of them have `cartGetDefault`'s `if (!cartId) return null` early return, so they don't share this bug. Left out of scope here.

Follow-up to the previous commit on this branch.

On review, I traced the full path of the fix and found a second case
the one-line change didn't cover: if a caller computes the cart id
dynamically and passes it through as `{cartId: maybeUndefined}`, the
old spread order `variables: {cartId, ...cartInput}` would overwrite
the already-resolved local `cartId` with `undefined` and the query
would go out with `cartId: undefined`.

Swapped the spread order to `{...cartInput, cartId}` so the resolved
local `cartId` always wins over whatever was passed in as the spread,
while still picking up `country` / `language` / `numCartLines` /
`visitorConsent` from cartInput as before. Added a regression test
that pins this by calling `cartGet({cartId: undefined})` and asserting
the cookie cart id is returned.

Verified all four existing tests still pass by tracing execution
against the mock storefront client, and checked that no caller in
the repo (including `createCartHandler.test.ts:171`) relies on the
old spread-order behaviour.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

ignored cartId in cartGetDefault

1 participant