-
Notifications
You must be signed in to change notification settings - Fork 8
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
feat: add caching to service worker #92
Conversation
We should probably use the verifiedFetchUrl you have in the other PR |
you're right.. I forgot service worker cache requires inputs to be a request object |
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.
looks mostly good, we need to figure out ipfs/helia-verified-fetch#17 tho
Yeah. That can be done in parallel, i.e. no need to block this PR, since the Cache API takes precedence over the browser HTTP cache.. |
also make sure that we actually store reponse in cache when fetching it in the background
Pretty obvious but worth noting here that depending on whether the origin has For example, when opening On the other hand, all resources |
src/sw.ts
Outdated
function setExpiresHeader (response: Response, ttlSeconds: number = 3600): void { | ||
const expirationTime = new Date(Date.now() + ttlSeconds * 1000) | ||
|
||
response.headers.set('Expires', expirationTime.toUTCString()) |
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.
It may be sensible to make this cache header explicitly SW specific (originally suggested in https://phyks.me/2019/01/manage-expiration-of-cached-assets-with-service-worker-caching.html)
response.headers.set('Expires', expirationTime.toUTCString()) | |
response.headers.set('sw-cache-expires', expirationTime.toUTCString()) |
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.
that makes sense
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.
Long term, if we can avoid inventing things specific to service worker, and reuse HTTP semantics, that is better for verified-fetch being useful in more contexts, and easier to test (if these are set upstream, places like https://github.com/ipfs/helia-http-gateway get these headers for free, and we also increase surface of things that can be tested there).
My initial thought here was that if we reuse semantically meaningful Expires
and set it only if not set yet, in the future it can be set inside verified-fetch
, and not only we don't need to invent special header here, but also improve the upstream library and the code here will be smaller.
But fine to go with custom header here for now.
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.
@lidel Because the SW Cache API always takes precedence over browser HTTP cache and and verified-fetch
will be returning the Cache-Control
header, the Expires
header would be: no-op.
Even if we were to use the Expires
header, by respecting it when there's a Cache-Control
header, we'd be breaking from HTTP semantics. This is why I think it makes sense to make it clear that this header is purely for SW purposes.
Yea, it would be nice if we could map the ipns name to a CID and re-route requests to ipfs://cid so that they're cached. |
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.
leaving some comments.. im going to check the code out locally and play with it
src/sw.ts
Outdated
* Note that this ignores the Cache-Control header since the expires header is set by us | ||
*/ | ||
function hasExpired (response: Response): boolean { | ||
const expiresHeader = response.headers.get('Expires') |
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.
const expiresHeader = response.headers.get('Expires') | |
const expiresHeader = response.headers.get('sw-cache-expires') |
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.
resolved in #109
src/sw.ts
Outdated
function setExpiresHeader (response: Response, ttlSeconds: number = 3600): void { | ||
const expirationTime = new Date(Date.now() + ttlSeconds * 1000) | ||
|
||
response.headers.set('Expires', expirationTime.toUTCString()) |
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.
that makes sense
src/sw.ts
Outdated
trace('helia-ws: setting expires header on response key %s before storing in cache', cacheKey) | ||
// 👇 Set expires header to an hour from now for mutable (ipns://) resources | ||
// Note that this technically breaks HTTP semantics, whereby the cache-control max-age takes precendence | ||
// Seting this header is only used by the service worker asd a mechanism similar to stale-while-revalidate |
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.
// Seting this header is only used by the service worker asd a mechanism similar to stale-while-revalidate | |
// Setting this header is only used by the service worker using a mechanism similar to stale-while-revalidate |
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.
resolved in #109
src/sw.ts
Outdated
} | ||
|
||
log('helia-ws: storing cache key %s in cache', cacheKey) | ||
void cache.put(cacheKey, respToCache) |
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.
void cache.put(cacheKey, respToCache) | |
await cache.put(cacheKey, respToCache) |
control flow should be done in higher level fetchHandler or getResponseFromCacheOrFetch
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.
resolved in #109
src/sw.ts
Outdated
// If there is an entry in the cache for event.request, | ||
// then response will be defined and we can just return it. |
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.
spacing here seems off
// If there is an entry in the cache for event.request, | |
// then response will be defined and we can just return it. | |
// If there is an entry in the cache for event.request, | |
// then response will be defined and we can just return it. |
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.
resolved in #109
src/sw.ts
Outdated
fetchHandler({ path: url.pathname, request: event.request }) | ||
.then(async response => storeReponseInCache({ response, isMutable, cache, cacheKey })) | ||
.catch(err => { | ||
err('helia-ws: failed updating response in cache for %s in the background', cacheKey, err) | ||
}) | ||
|
||
return cachedResponse | ||
} | ||
|
||
// 👇 fetch because no cached response was found | ||
const response = await fetchHandler({ path: url.pathname, request: event.request }) | ||
|
||
void storeReponseInCache({ response, isMutable, cache, cacheKey }).catch(err => { | ||
err('helia-ws: failed storing response in cache for %s', cacheKey, err) | ||
}) | ||
|
||
return response |
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 feel like we could combine these so we don't have two potential places where cache storing happens and could fail
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.
looking at the code more locally, we don't need to fetch from the network at all on cache-hit. Since we're currently fighting rate limits from the trustless gateway, we could just not do network requests. The likelihood of something updating on IPFS is very low, whereas stale-while-revalidate is good for frequently updated content that you want a quick response for.
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.
resolved in #109
src/sw.ts
Outdated
fetchHandler({ path: url.pathname, request: event.request }) | ||
.then(async response => storeReponseInCache({ response, isMutable, cache, cacheKey })) | ||
.catch(err => { | ||
err('helia-ws: failed updating response in cache for %s in the background', cacheKey, err) | ||
}) |
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.
do we need to call fetchHandler
and update the cache if we already have a cached response?
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.
resolved in #109. stale-while-revalidate only for ipns cache HIT
src/sw.ts
Outdated
fetchHandler({ path: url.pathname, request: event.request }) | ||
.then(async response => storeReponseInCache({ response, isMutable, cache, cacheKey })) | ||
.catch(err => { | ||
err('helia-ws: failed updating response in cache for %s in the background', cacheKey, err) |
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.
err('helia-ws: failed updating response in cache for %s in the background', cacheKey, err) | |
error('helia-ws: failed updating response in cache for %s in the background', cacheKey, err) |
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.
resolved in #109
Description
This PR adds caching for responses using the Cache API.
Fixes #73
Notes & open questions
isRootRequestForContent
case?: https://github.com/ipfs-shipyard/helia-service-worker-gateway/blob/3cc5b9882dbbf347a8c5fd51df311c8705dc6fc8/src/sw.ts#L94-L96event.request
. This seems reasonable but maybe it's not optimal?TODO
Change checklist