[DO NOT MERGE] Single Fetch Experiment

[michenly]

WHY are these changes introduced?

This PR aims to try out Remix's Single Fetch feature ahead of v3

Close https://github.com/Shopify/hydrogen-internal/issues/108

WHAT is this pull request doing?

Must do

• Turn on single fetch feature flag
• import single fetch type
• add nonce to <RemixServer />
• turn on cd's compatibility flag for http_headers_getsetcookie => need to make this configurable

remove the optional changes for v2 into branch mc-single-fetch-v3-template

Template Update (optional for Remix v2 user with future flag on) * use ResponeStub instead of Response Object everywhere * Any response body for error route can now throw new Error * replace `redirect` with ResponeStub (throw right away) and always use headers.set for locations * Watch out for redirect that occur in parent and child route. Parent will likely take precedent * remove `defer` everywhere * remove `json` everywhere, unless you specifically want to serialized the response * Type improvements: UIMatch_SingleFetch, MetaArgs_SingleFetch * Use response!.status syntax in loader/action even thou the type of response can be undefined

HOW to test your changes?

Post-merge steps

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
• I've added or updated the documentation

[github-actions]

We detected some changes in packages/*/package.json or packages/*/src, and there are no updates in the .changeset.
If the changes are user-facing and should cause a version bump, run npm run changeset add to track your changes and include them in the next release CHANGELOG.
If you are making simple updates to examples or documentation, you do not need to add a changeset.

[shopify]

Oxygen deployed a preview of your mc-single-fetch branch. Details:

Storefront	Status	Preview link	Deployment details	Last update (UTC)
optimistic-cart-ui	✅ Successful (Logs)	Preview deployment	Inspect deployment	May 28, 2024 8:17 PM
subscriptions	✅ Successful (Logs)	Preview deployment	Inspect deployment	May 28, 2024 8:17 PM
third-party-queries-caching	✅ Successful (Logs)	Preview deployment	Inspect deployment	May 28, 2024 8:17 PM
vite	✅ Successful (Logs)	Preview deployment	Inspect deployment	May 28, 2024 8:17 PM
custom-cart-method	✅ Successful (Logs)	Preview deployment	Inspect deployment	May 28, 2024 8:00 PM
Skeleton (skeleton.hydrogen.shop)	✅ Successful (Logs)	Preview deployment	Inspect deployment	May 28, 2024 8:17 PM

Learn more about Hydrogen's GitHub integration.

[blittle]

Unless we rethink this API with Remix middleware, I can think of two solutions for fixing the Customer Account API:

1. Like we force the user to commit the session, continue to require them to mutate the response directly:

export async function loader({context, response}) {
  const {data, errors} = await context.customerAccount.query(QUERY);

- return json(data, {
-   headers: {
-     'Set-Cookie': await context.session.commit(),
-   }
- });
+ response.headers.append('Set-Cookie', await context.session.commit());
+ return data;
})

2. Because the response is directly mutated, it would be nice for the customerAccount abstraction to handle that for you. It's easy to forget to commit the session, which introduces a non-obvious bug. The challenge is, how do we get the response object into our abstraction? It's unavailable in server.ts. Without a better middleware/context solution, the only way to do it at the moment is to directly pass it to the customerAccount methods:

export async function loader({context, response}) {
- const {data, errors} = await context.customerAccount.query(QUERY);

- return json(data, {
-   headers: {
-     'Set-Cookie': await context.session.commit(),
-   }
- });
+ const {data, errors} = await context.customerAccount.query(response, QUERY);
+ return data;
})

One major pro of this approach is it's easy to toggle how the customerAccount abstraction works for single-fetch based on the arguments passed. But at the same time, this feels awkward. Ideally a solution would be just:

export async function loader({context, response}) {
  const {data, errors} = await context.customerAccount.query(QUERY);
  return data;
})

I don't think this is possible without a way to inject the Remix response into the customer account abstraction, which should be possible with middleware. Until then, do we pick one of the solutions above? Or do we hold off adopting single-fetch until middleware also ships (which should be soon as it's apart of Remix 3)?

[frandiox]

Reading the docs on SingleFetch, it sounds like the ResponseStub is unique to each loader/action. So we wouldn't be able to simply pass 1 single response stub to our constructors or anything like that.

Perhaps this is some feedback we can give to the Remix team on middleware: it would be nice to have access to these response stubs somehow for each loader/action. I'm not sure if this is possible but I could imagine some kind of AsyncLocalStorage-like utility where we can call getResponseStub() inside our customer/cart functions and get the right one depending on the loader/action that is running.

However, I think for now we probably need to go the explicit way and assume we are not going to have this functionality in middleware 🤔

So going back to what's possible... I think would prefer signatures like

context.customerAccount.query(QUERY, {response})

which is similar to the storefront client and lets us add more options or required params.

[michenly]

ResponseStub is unique to each loader/action

I think this is mostly true from a type perspective where the data returned would be different.

However, the part of the utilities we want to access in our utilities are the shared portion that will always be the same.
Similar to the typing of our HydrogenSession where there will always be get, set, unset and commit methods that act the same. And the data in the generics.

I do agree for now passing in response is the way to go thou 👍

[blittle]

@frandiox @michenly part of me thinks it's better to not add an argument for now, and instead force the user to just handle it on the outside. Otherwise we are going to introduce a breaking change right now to require a response argument, then a few months later from now we'll change it again once there's a better middleware solution. If we make the user handle it, down the road we'll just say, "you can now delete that code where you manually commit the session". Additionally, we already make them commit the session.

I want to optimize for the least amount of changes required for developers. Or maybe another option is we wait to adopt single-fetch until there's also a middleware solution.

[michenly]

@blittle I will take the breaking change into consideration and think about it some more. However...

I just finished the refactor for adding an response option for both cart & customerAccount. And I am actually catching A LOT of possible errors we made around when the header setting should occur. Moving it into the utilities is helping to ensure the session setting always occur because we can set the response header when we set session.

So I wonder if the breaking change is actually worth it for the quality improvement.

[blittle]

@michenly I agree that it should be inside our abstraction. It was always my concern that it would be forgotten if left to the user on the outside. I'm just saying I want to minimize API changes. If we make this change now, and then in a few weeks middleware comes out, then we want to change it again? I'd rather find a way with middleware to inject the response without requiring everywhere as a parameter. That said, I think you should keep refactoring it to be handled inside the abstraction, I just hope to remove the necessity to pass response to each method by the time we ship this.

[blittle]

One thing to note, resource routes (where only a loader is exported, no default export) it's not recommended to use defineLoader: https://remix.run/docs/en/main/guides/single-fetch#resource-routes

It is not recommended to use defineLoader/defineAction for externally-accessed resource routes that need to return specific Response instances. It's best to just stick with loader/LoaderFunctionArgs for these cases.

[frandiox]

That's confusing...

[michenly]

super confusing indeed...

So the reason why I changed it to defineLoader is because the type for response in loader is ResponseStub | undefined., and I want to ensure response is always defined.

Switching back to loader means I likely need to handle the case where ResponseStub is undefined. Or I can just assume ResponseStub is always defined. Which means this route is always being hit with Remix APIs (It actually is, since it is being redirected). But at this point, I can also assume this is NOT a true Resource Route that would cause type error thou.

[blittle]

In general I feel like loaders are more confusing with single fetch. Resource route loaders still return new Response, other loaders don't :(

[blittle]

It seems like it doesn't matter if we wrap it in defineLoader, so maybe we just keep it consistent even if they say it's not recommended? 🤷🏼

[michenly]

I am going with loader unless the type fail and I specifically needed defineLoader so far. Will see if anything fail thou. I am testing CA API right now and finding some interesting failure. Fixing it while testing to see slowly what the pattern is.

[frandiox]

@michenly @blittle Hey here's another random idea:

We already have both session and customerAccount instances in our Remix-Oxygen's handleRequest. Maybe we can add set-cookie header to the response there (i.e. commit the session) if we detect it's necessary depending on the methods that have been called in the customerAccount instance?

Or maybe we don't do this automatically but we at least warn the users here if they didn't call session.commit() themselves? This is if we can actually detect a session commit is needed given the methods called in customerAccount.