Pass request to createStorefrontClient
#1444
Conversation
|
We detected some changes in |
| [STOREFRONT_ACCESS_TOKEN_HEADER]: | ||
| defaultHeaders[STOREFRONT_ACCESS_TOKEN_HEADER], |
There was a problem hiding this comment.
@wizardlyhel This one is only the public token. The private one lives in a different variable and is not being added to the cache key. Is this correct?
There was a problem hiding this comment.
No. The access token could be the private token if it exists
hydrogen/packages/hydrogen/src/storefront.ts
Lines 258 to 260 in 1b45311
There was a problem hiding this comment.
wait nvm .. those 2 function generates different headers
getPrivateTokenHeaders returns contentType, buyerIp, privateStorefrontToken
getPublicTokenHeaders returns contentType, publicStorefrontToken
There was a problem hiding this comment.
Yeah it's different header. Should I fix that here and also add the private one?
| export function getClientIp(request: CrossRuntimeRequest) { | ||
| return ( | ||
| request.headers?.get?.('oxygen-buyer-ip') ?? | ||
| request.headers?.get?.('cf-connecting-ip') ?? | ||
| request.headers?.['x-forwarded-for']?.split(',')[0] ?? | ||
| request.socket?.remoteAddress ?? | ||
| null | ||
| ); | ||
| } |
There was a problem hiding this comment.
Is it OK to check for Node here and CFW here? We are leaving Deno and Bun out, but currently those don't provide the IP in the request object.
Should we just care about Oxygen instead? In that case I think we should add an optional parameter to overwrite this behavior for non-Oxygen environments, right (like the current storefrontHeaders.buyerIp).
There was a problem hiding this comment.
I think it's okay to just care about what Hydrogen cares about but developers must be able to override this ip value since it is the only way to avoid being blocked by Shopify ABS if they were to use other hosting options. Example: AWS hosting
| if (!requestId) { | ||
| // Store it in the headers object for later access | ||
| request.headers['request-id'] = requestId = generateUUID(); | ||
| } |
There was a problem hiding this comment.
This piece here is going to generate an ID and store it in the request headers object so that it can be used in the next call (e.g., when calling createWithCache and later createStorefrontClient, they should access the same request-id if the environment doesn't provide it).
| @@ -26,7 +25,7 @@ export default { | |||
| throw new Error('SESSION_SECRET environment variable is not set'); | |||
| } | |||
|
|
|||
| const waitUntil = (p: Promise<any>) => executionContext.waitUntil(p); | |||
| const waitUntil = executionContext.waitUntil.bind(executionContext); | |||
There was a problem hiding this comment.
FYI I'm changing these lines to use bind because that's what we have in our skeleton template. It's not as easy to understand as arrow functions but it keeps the type correct in JS projects, which I think is useful?
|
Hmm .. passing the However, |
Yeah I also have the same feeling. Just wanted to show how this looks like but not really sure about it. Let's see what the rest of the team thinks. @blittle @juanpprieto |
|
@frandiox @wizardlyhel I agree, as long as we are unopinionated about the deployment platform, then I don't think it's worth making this change. |
|
The tests are extracted in #1449 |
Let's discuss if this is a good API or not (original public discussion). The problem is the "cross runtime request" issue.
I've added tests here for a the storefront client as well (the tests we had before were in an e2e that we removed long ago...), so we should merge at least that (will revert other things if we don't want it).