Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
feat: add caching to service worker #92
Changes from 10 commits
6756630
3cc5b98
1e9a30a
34a396b
7a08d10
9ff7314
f56d25e
0095181
555fbcc
30dd440
6a0d416
69f4d4c
fdbeb97
a0c8dcb
4714ab1
0bc73ed
3e003ae
511c64f
c08c467
e8a41ea
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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)
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 insideverified-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 theCache-Control
header, theExpires
header would be: no-op.Even if we were to use the
Expires
header, by respecting it when there's aCache-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.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.
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
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
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
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.
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
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
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
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.
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
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.
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