-
Notifications
You must be signed in to change notification settings - Fork 377
🍪 Hydrogen Cookie Migration for New Shopify Cookie Architecture #3309
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
2311f45 to
1684f1e
Compare
|
Oxygen deployed a preview of your
Learn more about Hydrogen's GitHub integration. |
| * using Hydrogen's built-in analytics. | ||
| * @default true | ||
| */ | ||
| tracking?: boolean; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| tracking?: boolean; | |
| shouldCollectTrackingInformation?: boolean; |
or something like shouldCollectShopifyTrackingInformation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given the existing patterns (e.g. poweredByHeader: boolean), I think we can use collectTrackingInformation: boolean better?
| if ( | ||
| isDocumentResponse && | ||
| collectedSubrequestHeaders && | ||
| collectedSubrequestHeaders.setCookie.length > 1 && |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why .length > 1? to confirm, this is just to confirm that we did indeed make a request to the server (rather than having this be returned from the cache), right? or is there something else I'm missing here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems _shopify_essential might be returned alone from SFAPI, and we can't be sure this means we got everything we needed (at least during the transition period). With this we just check at at least essential + another cookie comes back. Therefore, if we only have 1 cookie, we'll fire a request from the browser to get everything.
Will leave a comment around there 👍
| onVisitorConsentCollected: (consent) => { | ||
| try { | ||
| // Store consent to refresh local cookies after it changes | ||
| setCollectedConsent(JSON.stringify(consent)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are we bothering to JSON.stringify the consent? We don't seem to actually be reading it anywhere
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was one of the bug fixes added in the PR to existing Hydrogen behavior. It wasn't calling canTrack again after consent changed.
Basically, every time consent changes (the user clicks the banner, or shows the preferences banner and clicks) we should recalculate hasUserConsent, since writing or deleting cookies depend on that.
We use it below as a hook dependency, which means that whenever it changes that other hook will run (and will call canTrack()). This callback is fired multiple times from privacy-banner, and sometimes it doesn't really mean "consent collected" (a bug in my opinion but it will take longer to fix). With this, we can check if consent has changed across different calls because the stringified version would be different only if consent changes.
I was going to add something else here but there are already two comments where this is set and read:
// Store consent to refresh local cookies after it changes
setCollectedConsent(JSON.stringify(consent));
// Make this value depend on collectedConsent to re-run `canTrack()` when consent changes
[privacyReady, canTrack, collectedConsent],
| doesMerchantSupportGranularConsent | ||
| firstPartyMarketingAllowed | ||
| getCCPAConsent | ||
| getRegulation |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for all of these i'm assuming you double checked the customer privacy API to confirm that all of these are outdated/no longer exist?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, they were deprecated some time ago, according to their changelog and code
| type TrackingValues = { | ||
| uniqueToken: string; | ||
| visitToken: string; | ||
| consent: string; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we pick a more descriptive name here (or at least add a comment)? i imagine this is referring to the _cmp value?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can add comments but these names seem to be the convention in other areas 👍 (e.g. _cmp => "consent management platform", current cookie is called _tracking_consent -- which should go away eventually).
| @@ -0,0 +1,9 @@ | |||
| --- | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(threading)
Bug: Cross-subdomain matching fails for sibling domains
Location: packages/hydrogen-react/src/tracking-utils.ts:75-79
The Problem
The current matching logic only handles:
- Same origin (
hydrogen.shop↔hydrogen.shop) - Checkout as subdomain of storefront (
checkout.hydrogen.shop→hydrogen.shop)
But it fails when storefront and checkout are sibling subdomains under a shared parent:
Storefront: oldnavy.gapcanada.ca
Checkout: secure-oldnavy.gapcanada.ca
// Current check:
'secure-oldnavy.gapcanada.ca'.endsWith('.oldnavy.gapcanada.ca') // FALSE ❌
This means SFAPI responses from checkout are ignored, and we fall back to stale navigation entry values.
The Fix
Add a third condition that checks if both hosts share a common ancestor domain:
const isMatch =
matchedHost === currentHost ||
(sfapiPath && matchedHost?.endsWith(`.${currentHost}`)) ||
(sfapiPath && hasCommonAncestorDomain(matchedHost, currentHost)); // ← Add thisWhere hasCommonAncestorDomain finds the common suffix parts (e.g., gapcanada.ca) and requires at least 2 matching parts (or 3 for multi-part TLDs like .co.uk) to prevent matching unrelated domains.
Other affected merchants
This would also affect setups like:
www.store.com+checkout.store.comshop.brand.co.uk+secure.brand.co.uk
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For this we decided we're okay leaving this as is, since this only affects merchants with sibling domains AND who aren't using the proxy. We will explicitly document this limitation instead of supporting it here
kdaviduik
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🚀!!!!!
Co-authored-by: Kara Daviduik <[email protected]>
Updates both @Shopify/hydrogen and @shopify/remix-oxygen createRequestHandler to match the implementation in PR #3309: - Add collectTrackingInformation option (default: true) - Add warning when storefront instance is missing from context - Use appendServerTimingHeader utility instead of manual header - Move poweredByHeader append to after tracking header processing - Add TODO comments for future major version changes - Improve JSDoc documentation for all options
Re-export getTrackingValues from @shopify/hydrogen-react to align with PR #3309 cookie migration implementation.
Aligns the parameter name with PR #3309 for consistency.
… Architecture (#3332) * Add tracking cookies migration infrastructure This commit adds the infrastructure for migrating tracking cookies from legacy _shopify_y/_shopify_s cookies to consolidated encrypted cookies using Server-Timing headers. Key changes: - Add tracking-utils.ts with getTrackingValues() to read tracking values from Server-Timing headers via Performance API - Add server-timing.ts with utilities for detecting SFAPI proxy mode and server-returned tracking values - Update storefront client to forward tracking values via headers - Add sameDomainForStorefrontApi option for proxy detection - Update customer-privacy component to support new cookie migration - Add buyerIpSig to StorefrontHeaders type - Update useShopifyCookies with fetchTrackingValues option * Add E2E tests for tracking cookies migration This commit adds comprehensive E2E tests for the tracking cookies migration feature, including both new and legacy cookie scenarios. Test structure: - new-cookies/: Tests for the new consolidated cookie system - consent-tracking-accept/decline.spec.ts - privacy-banner-accept/decline/consent-change.spec.ts - privacy-banner-migration.spec.ts (migration from legacy cookies) - old-cookies/: Tests for legacy _shopify_y/_shopify_s cookies - consent-tracking-accept/decline.spec.ts - privacy-banner-accept/decline.spec.ts - smoke/: Basic smoke tests for cart and home page Also adds: - Unit tests for tracking-utils.ts and server-timing.ts - E2E fixtures for server setup and storefront mocking - Environment configurations for different consent scenarios - Updated playwright configuration * Fix: Delay PerfKit loading until consent is collected This fixes a bug where PerfKit would load before consent was collected, causing it to use stale tracking values. The fix delays PerfKit loading until after the consent collection callback is fired. Changes: - Add consentCollected state to AnalyticsProvider - Only render PerfKit after consentCollected is true - Update onVisitorConsentCollected callback to track consent state - Add sameDomainForStorefrontApi to Consent type * Add SFAPI proxy support to createRequestHandler - Update @shopify/remix-oxygen createRequestHandler with SFAPI proxy - Add new createRequestHandler export to @Shopify/hydrogen - Proxy automatically routes /api/.../graphql.json to Storefront API - Forward Set-Cookie and server-timing headers from subrequests - Set _sfapi_proxy server-timing header for document requests - New proxyStorefrontApiRequests option (default: true) * Update package-lock.json for version bumps * Align createRequestHandler with PR #3309 Updates both @Shopify/hydrogen and @shopify/remix-oxygen createRequestHandler to match the implementation in PR #3309: - Add collectTrackingInformation option (default: true) - Add warning when storefront instance is missing from context - Use appendServerTimingHeader utility instead of manual header - Move poweredByHeader append to after tracking header processing - Add TODO comments for future major version changes - Improve JSDoc documentation for all options * Add missing getTrackingValues export from hydrogen package Re-export getTrackingValues from @shopify/hydrogen-react to align with PR #3309 cookie migration implementation. * Add buyerIpSig to remix-oxygen StorefrontHeaders Add the buyerIpSig field to both the StorefrontHeaders type and getStorefrontHeaders function in remix-oxygen to align with the hydrogen package's StorefrontHeaders type. This fixes type compatibility when passing getStorefrontHeaders() result to createStorefrontClient(). * Add sec-purpose header check to remix-oxygen getStorefrontHeaders Align with hydrogen's getStorefrontHeaders to check sec-purpose first, then fall back to purpose. This ensures browser-initiated prefetches (via Speculation Rules API or <link rel="prefetch">) are properly detected. * Rename proxyStorefrontApiRequests to proxyStandardRoutes Aligns the parameter name with PR #3309 for consistency. * Update Playwright to 1.57.0 Fixes Chromium crashes (SIGTRAP/SEGV) on macOS 15 Sequoia when running e2e tests. The older version 1.40.1 had compatibility issues with the newer macOS version. * Fix e2e test bundle interception pattern for Vite pre-bundled deps Update the route interception pattern in setWithPrivacyBanner to match Vite's pre-bundled dependency path format. The previous pattern (**/@fs/**/hydrogen/dist/**/*.js) didn't match the actual paths served by Vite dev server (**/node_modules/.vite/deps/@shopify_hydrogen.js*). * Fix * fix * Fix create-hydrogen snapshot for TokenlessApi route Update the integration test snapshot to include the TokenlessApi route that was added in #2948 but the snapshot was never updated. Also includes skeleton template changes for the cookie migration backport: - Use createRequestHandler from @Shopify/hydrogen with proxyStandardRoutes - Minor formatting cleanup in root.tsx * Disable proxyStandardRoutes in classic-remix example The classic-remix example uses @shopify/remix-oxygen's createRequestHandler which now defaults to proxyStandardRoutes: true. This requires a storefront instance with specific methods that the classic-remix example doesn't provide. Setting proxyStandardRoutes: false restores the previous behavior and fixes the dev test. * Also disable collectTrackingInformation in classic-remix example The classic-remix example doesn't use the full Hydrogen stack, so disable both proxyStandardRoutes and collectTrackingInformation to avoid issues with the new request handler options. * Guard getSetCookie call for environments without Headers support Some environments like the classic Remix compiler may not have the getSetCookie method on their Headers implementation. * Remove redundant in skeleton template; update changesets to be more consistent with 2025-07 PR
🍪 Hydrogen Cookie Migration for New Shopify Cookie Architecture
Overview
This PR implements support for Shopify's new consolidated cookie architecture. The legacy
_shopify_yand_shopify_scookies are being sunset in 2026, with their functionality absorbed into the newhttp-onlycookies:_shopify_analytics,_shopify_marketing, and_shopify_essential.Main Changes
1. Storefront API Proxy
createRequestHandlerhttp-onlycookies to be properly set2. New Tracking Utilities
getTrackingValues(): Reads tracking values (uniqueToken/visitToken) fromserver-timingheaders via the Performance API_shopify_y/_shopify_scookies during transitiongetShopifyCookiesis deprecated and now works internally by callinggetTrackingValues3. Updated
useShopifyCookiesHookfetchTrackingValuesoption to fetch cookies from browser4. Privacy/Consent Integration
sameDomainForStorefrontApioption for proxy-aware consent librariesShopifyProviderauto-detects SFAPI proxy availability viaserver-timingheaders5. Backward Compatibility
Migration Impact
server.tsto use newcreateRequestHandlerhydrogen-reactin custom frameworksE2E Testing