Fix add-to-cart session event in Live View

[wizardlyhel]

WHY are these changes introduced?

Add-to-cart session in Live View is not working

Docs - https://github.com/Shopify/shopify-dev/pull/31897

WHAT is this pull request doing?

• add the missing headers

HOW to test your changes?

1. Open Shopify admin -> Analytics -> Live View dashboard
2. View oxygen preview link (won't work on localhost unless your public access token is generated by oxygen)
3. Add an item to cart

Result: You should see the Active Cart counter increase in the Customer Behaviour card.

Note: You won't see the Visitors right now counter increase if you are running in preview. This will only increase when running in production.

Checklist

• I've read the Contributing Guidelines
• I've considered possible cross-platform impacts (Mac, Linux, Windows)
• I've added a changeset if this PR contains user-facing or noteworthy changes
• I've added tests to cover my changes

[frandiox]

Thanks for the fixes! I've added some comments below.

In order to decouple the request object and its headers from Hydrogen, would it make sense to have a utility imported from Oxygen that returns all the necessary headers for Hydrogen? Something like:

import {getHeadersForHydrogen} from '@shopify/remix-oxygen';

//...

createStorefrontClient({
   cache,
   waitUntil,
   i18n: getLocaleFromRequest(request),
   publicStorefrontToken: '...',
   privateStorefrontToken: '...,
   storeDomain: '...',
   storefrontApiVersion: '...',
   // This populates buyerIp, requestGroupId, cookies, etc.
   ...getHeadersForHydrogen(request),
})

This way it's the platform adapter the one in charge to access the platform request. When deploying to Node/CFW/etc. devs can just get the headers manually and pass them as parameters like they do now. This would solve the comment below regarding request type.

The name of the utility is just an example of course.

Note: the call to getShopifyCookies can stay in Hydrogen, just the cookie header comes from getHeadersForHydrogen.

Alternatively, we could add a headers: getHeadersForHydrogen(request) property if we dislike the spread operator here, and just ignore the current buyerIp and requestGroupId if provided. I think I'd slightly prefer the spread operator, though.

[wizardlyhel]

I like this approach. This guarantees the request is a worker request from Oxygen and deals with the Oxygen specific keys.
Also agrees that cookie is just a straight proxy from worker request.

import {getHeadersForHydrogen} from '@shopify/remix-oxygen';

//...

createStorefrontClient({
   cache,
   waitUntil,
   i18n: getLocaleFromRequest(request),
   publicStorefrontToken: '...',
   privateStorefrontToken: '...,
   storeDomain: '...',
   storefrontApiVersion: '...',
   // This populates buyerIp, requestGroupId, cookies, etc.
   ...getHeadersForHydrogen(request),
})

As for the parameter layout, I don't like spread because it make typescript angry. So I am leaning towards the header props.

createStorefrontClient({
   cache,
   waitUntil,
   i18n: getLocaleFromRequest(request),
   publicStorefrontToken: '...',
   privateStorefrontToken: '...,
   storeDomain: '...',
   storefrontApiVersion: '...',

   // This populates buyerIp, requestGroupId, cookies, and developer can further extend to add user defined headers
   header: getHeadersForHydrogen(request),

   buyerIp, // deprecation warning for next calver release
   requestGroupId, // deprecation warning for next calver release
})

[frandiox]

As for the parameter layout, I don't like spread because it make typescript angry. So I am leaning towards the header props.

Sure, I don't have a big opinion on this. I'm not sure if headers and getHeadersForHydrogen are the best fit though, perhaps we can brainstorm some names.

@Shopify/hydrogen @gfscott tl;dr Team, can we think of a better name for this prop and function?

import {getHeadersForHydrogen} from '@shopify/remix-oxygen';

createStorefrontClient({
  // ...
  headers: getHeadersForHydrogen(request),
})

The function is extracting information from the request. For now they are just headers (and will likely be always headers... right?). Examples are 'cookies', 'oxygen-buyer-ip' and 'request-id' headers, for now. The reason to pass this instead of the request directly is explained here.

Other ideas:

• headers: getStorefrontHeaders(request)
• requestMeta: extractRequestInformation(request) (full random)

[gfscott]

I'm not sure if headers and getHeadersForHydrogen are the best fit though, perhaps we can brainstorm some names

Couple questions just to make sure I'm getting the full picture here:

• headers as prop name: does this risk any confusion with the native Request.headers object?
  • Perhaps for clarity we namespace it as hydrogenHeaders or something?
• getHeadersForHydrogen() function name: can we simplify to getHydrogenHeaders()?

[wizardlyhel]

I like storefrontHeaders: getStorefrontHeaders(request)

These are specific headers for making a SFAPI call

[wizardlyhel]

Decided to go with this:

+ import {getStorefrontHeaders} from '@shopify/remix-oxygen';
import {createStorefrontClient, storefrontRedirect} from '@shopify/hydrogen';

export default {
  async fetch(
    request: Request,
    env: Env,
    executionContext: ExecutionContext,
  ): Promise<Response> {

    const {storefront} = createStorefrontClient({
      cache,
      waitUntil,
-     buyerIp: getBuyerIp(request),
      i18n: {language: 'EN', country: 'US'},
      publicStorefrontToken: env.PUBLIC_STOREFRONT_API_TOKEN,
      privateStorefrontToken: env.PRIVATE_STOREFRONT_API_TOKEN,
      storeDomain: `https://${env.PUBLIC_STORE_DOMAIN}`,
      storefrontApiVersion: env.PUBLIC_STOREFRONT_API_VERSION || '2023-01',
      storefrontId: env.PUBLIC_STOREFRONT_ID,
-     requestGroupId: request.headers.get('request-id'),
+     storefrontHeaders: getStorefrontHeaders(request),
    });

buyerIp and requestGroupId are soft deprecated. Deprecation message will be triggered on the server side:

• If getBuyerIp is in use
• If storefrontHeaders is not defined

To-do

• The naming of storefrontHeaders: getStorefrontHeaders(request) can be up for debate
• How to share the StorefrontHeaders type - it is defined in remix-oxygen and hydrogen
• Better deprecation message

[frandiox]

Added a few minor suggestions 👍
We'll need to update the docs as well when releasing this.

[frandiox]

Since I guess storefrontHeaders will still be optional, perhaps better to warn only when the other properties are used?

Suggested change

	if (!storefrontHeaders) {
	if (!storefrontHeaders && (buyerIp || requestGroupId)) {

[wizardlyhel]

I initially was gonna detect by buyerIp and requestGroupId.. but both of these are null in dev mode

and .. don't we need to have one or the other?