Subrequest cache

[frandiox]

Related: #67
Docs PR #131

Most of this is extracted from Hydrogen 1. Summary of changes:

• Add storefront.query({ cache }) option for sub-request cache.
• Add storefront.query({ mutation }) option, which ignores cache.
• Add strategies to storefront object: storefront.CacheLong(...), storefront.CacheShort(...), etc.
• Add context.fetch(...) method to call 3p APIs and cache responses. The same function can be used standalone as import {fetchWithServerCache} from '@hydrogen/remix'.
• Add Request Group ID and Buyer ID to SFAPI requests.
• Add in-memory cache for local development until mini-oxygen is fixed. Use a new version of mini-oxygen that fixes the cache problems.
• Minify queries sent over the network (remove comments, spaces, etc)

[frandiox]

@maxshirshin Do you know if mini-oxygen should work with cache? I think we are using it in Hydrogen 1 preview but I can't seem to make it work here. With a simple code like this:

export default {
  async fetch(
    request,
    env,
    context,
  ) {
      const cache = await caches.open('test');
      console.log(await cache.match(new Request(request.url)));

      return new Response('ok');
   }
 }

The line that runs cache.match throws the following:

Error: Some functionality, such as asynchronous I/O (fetch, Cache API, KV), timeouts (setTimeout, setInterval), and generating random values (crypto.getRandomValues, crypto.subtle.generateKey), can only be performed while handling a request.

This behavior can be disabled by providing globalAsyncIO: true to Miniflare here, but then a new error comes up. This time, the request comes to this line in Undici and fails the instanceof check... I guess because it's a Request from two different realms.

Any idea?

cc @jplhomer

[frandiox]

@frehner Can this be improved? TS complains when using query and mutation at the same time but the error is rather cryptic.

Thinking again that perhaps we should switch to storefront.query('...', {}) and storefront.mutate('...', {}) instead.

[frandiox]

@jplhomer Thoughts on this? Should we have this only as a standalone import instead?

[jplhomer]

Yeah storefront.fetch is a confusing pattern, because you're not fetching from a storefront 😄 we probably wanna inject a separate utility.

I'm also thinking more about how Next.js and Cloudflare recommend interacting with cache, which is to set a cache header on the outgoing request:

• Cloudflare allows you to define cache directives on the cf property: https://developers.cloudflare.com/workers/examples/cache-using-fetch/
• Next.js allows you to define cache directives on the next property: https://beta.nextjs.org/docs/data-fetching/caching

What if we followed this pattern? @Shopify/hydrogen thoughts?

[frandiox]

Right now it looks like fetchWithServerCache('...', {headers}, {cache: CacheLong()}), where the cache options are passed in the third parameter. Are you suggesting moving it to the second parameter instead? fetchWithServerCache('...', {headers, cache: CacheLong()}) ?

[benjaminsehl]

I was going to post the same thing @jplhomer.

If we want to have a single interface for things, that's cool — but then maybe it's worth calling it hydrogen instead?

[frandiox]

@jplhomer @frehner I think most of the code we have now in @hydrogen/remix is actually quite independent from Remix. Perhaps the whole storefront object here, its cache and utilities could live directly in @shopify/hydrogen or @shopify/hydrogen-xyz? That might be useful in Next.js or Node.

Then, @hydrogen/remix (or @shopify/hydrogen-remix) would really just contain createRequestHandler and re-export createStorefrontClient 🤔

[frandiox]

@jplhomer Alright so I've removed the fetchWithServerCache export, moved context.storefront.fetch to context.fetch, and changed its signature to context.fetch('...', {hydrogen:{cache}}).

Should we merge this for now and make further adjustments in new PRs?

[jplhomer]

Let's ship it!