-
Notifications
You must be signed in to change notification settings - Fork 264
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
Subrequest cache #133
Subrequest cache #133
Conversation
@maxshirshin Do you know if 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
This behavior can be disabled by providing Any idea? cc @jplhomer |
export type StorefrontQueryOptions = StorefromCommonOptions & { | ||
query: string; | ||
mutation?: never; | ||
cache?: CachingStrategy; | ||
}; | ||
|
||
export type StorefrontMutationOptions = StorefromCommonOptions & { | ||
query?: never; | ||
mutation: string; | ||
cache?: never; | ||
}; |
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.
@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.
fetch: ( | ||
url: string, | ||
requestInit: RequestInit, | ||
fetchOptions: Omit<FetchCacheOptions, 'cacheInstance' | 'waitUntil'>, | ||
) => | ||
fetchWithServerCache(url, requestInit, { | ||
waitUntil, | ||
cacheKey: [url, requestInit], | ||
cacheInstance: cache, | ||
...fetchOptions, | ||
}), |
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.
@jplhomer Thoughts on this? Should we have this only as a standalone import instead?
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 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?
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.
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()})
?
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.
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?
@jplhomer @frehner I think most of the code we have now in Then, |
@jplhomer Alright so I've removed the Should we merge this for now and make further adjustments in new PRs? |
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.
Let's ship it!
Related: #67
Docs PR #131
Most of this is extracted from Hydrogen 1. Summary of changes:
storefront.query({ cache })
option for sub-request cache.storefront.query({ mutation })
option, which ignores cache.storefront
object:storefront.CacheLong(...)
,storefront.CacheShort(...)
, etc.context.fetch(...)
method to call 3p APIs and cache responses.The same function can be used standalone asimport {fetchWithServerCache} from '@hydrogen/remix'
.Add in-memory cache for local development until mini-oxygen is fixed.Use a new version of mini-oxygen that fixes the cache problems.