fix: make visitorConsent conditional in cart operations#3720
fix: make visitorConsent conditional in cart operations#3720itsjustriley merged 4 commits intomainfrom
Conversation
|
Oxygen deployed a preview of your
Learn more about Hydrogen's GitHub integration. |
| @@ -1,17 +1,41 @@ | |||
| export const CartLineAdd = (cartFragment: string): string => /* GraphQL */ ` | |||
| type CartQueryOptions = { | |||
| includeVisitorConsent?: boolean; | |||
There was a problem hiding this comment.
what was the previous behaviour before this param existed? we should default to whatever was before, right?
is there ever a world where we would want people not to include visitor consent? and should we even allow that?
these are genuine questions, im wondering if we could just hardcode it as true and include it every time
There was a problem hiding this comment.
The default behaviour was that it wasn't included. We added the feature to include it unconditionally based on an API change (#3408). That PR states: "Most Hydrogen storefronts do NOT need this feature. If you're using Hydrogen's analytics provider, or Shopify's Customer Privacy API, then consent is already handled automatically." So we're defaulting to the original behaviour in this PR.
There was a problem hiding this comment.
excellent! thanks for the comprehensive reply
There was a problem hiding this comment.
thank you for prompting me to double check!
|
is it worth adding an e2e test for metafields? this could have been caught by it |
definitely! https://github.com/Shopify/developer-tools-team/issues/1199 |
fredericoo
left a comment
fredericoo
left a comment
There was a problem hiding this comment.
just some small comments that are not necessarily needed to be addressed
also if you rebase or merge main in all the tests should pass 🥳
Fixes #3708 Cart operations were unconditionally declaring `$visitorConsent: VisitorConsent` in GraphQL mutations, causing schema validation failures on stores without the VisitorConsent type (older API versions or certain store configurations). The visitorConsent parameter is now only included when explicitly provided via `optionalParams.visitorConsent`. This restores compatibility while preserving the feature for users who need it. Changes: - Add cart-query-helpers.ts with getInContextVariables/getInContextDirective - Update 16 cart mutation files to use conditional inclusion - Add tests verifying conditional behavior Co-Authored-By: Claude Opus 4.5 (1M context) <noreply@anthropic.com>
Same root cause as the hydrogen fix - cart queries were unconditionally
declaring `$visitorConsent: VisitorConsent`, causing schema validation
failures on stores without that type.
Cart query functions now accept an optional second parameter:
`{ includeVisitorConsent?: boolean }`. The visitorConsent variable and
directive argument are only included when explicitly enabled.
Co-Authored-By: Claude Opus 4.5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.5 (1M context) <noreply@anthropic.com>
… into helpers Address review feedback on PR #3720. The visitorConsent helpers and type were duplicated across 17 mutation/query files with 3 inconsistent names (CartMutationBuilderOptions, CartMutationOptions, CartQueryBuilderOptions). Callers also repeated the derivation logic and forced ?? false at every call site. - Export CartBuilderOptions from cart-query-helpers.ts as the single source of truth - Add shouldIncludeVisitorConsent() to centralize the visitorConsent detection logic - Make getInContextVariables/getInContextDirective params optional (default false) so callers no longer write options.includeVisitorConsent ?? false at 34 call sites Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
itsjustriley
force-pushed
the
fix/visitor-consent-conditional
branch
from
aeac91d to
45c3550
Compare
2 participants
Summary
Fixes #3708
$visitorConsent: VisitorConsentin GraphQL operationsVisitorConsenttypeChanges
@shopify/hydrogen (commit 1):
cart-query-helpers.tswithgetInContextVariables()andgetInContextDirective()helpers@shopify/hydrogen-react (commit 2):
{ includeVisitorConsent?: boolean }parameterTest plan
🤖 Generated with Claude Code