refactored etag logic. fixes 122 #133
Conversation
src/index.ts
Outdated
| headers.set('CF-Cache-Status', 'HIT') | ||
| response = new Response(response.body, { headers }) | ||
| response = new Response(response.body, response) | ||
| response.headers.set('cf-cache-status', 'HIT') |
There was a problem hiding this comment.
It seems that Cloudflare Web Server or Cloudflare Cache will add cf-cache-status header itself.
I have removed the line in my own worker script, but the header still appears in the final response.
I could open a new PR removing those cf-cache-status.
There was a problem hiding this comment.
does that header still appear if you remove that resource from cloudflare's cache?
There was a problem hiding this comment.
To be honest, I’m not sure if the cf-cache-status headers need to be set or not. If they are redundant, I still think they should be retained b/c all the tests will need to be refactored otherwise. Best to discuss this issue on a separate thread IMO
There was a problem hiding this comment.
does that header still appear if you remove that resource from cloudflare's cache?
Yes.
|
@ispivey need your review on this. We should also get someone on the FL team I think |
|
Will the PR being continued? I have already tested it on my website and the 304 works as expected. |
|
@shagamemnon Are you still working on this? There's been a general decrease in repo activity.☹ Waiting for ETag code to be polished up before I play around with it. |
|
Went down quite a search rabbit hole before finding these ETag-related issues, but am hoping this gets resolved soon. |
|
I too have just come across this issue. It's discouraging us from going live with Workers hosting a static site which is quite image heavy. Other than this issue, we are really excited about using this as it seems to address our needs very well. This PR has been sitting here for more than 6 months now. Any chance of it being picked up and reviewed please? |
|
As others have mentioned, it'd be great for this to be merged, as etags don't seem to work at the moment :( |
|
Hi @caass, any idea when 0.1.2 may be released with this? |
|
@EatonZ no estimate yet, unfortunately :( |
|
Hey all! Would like to help get this merged this week - I'm working on getting a new version of kv-asset-handler out by Friday. Has anyone tested this PR recently or would be interested in helping me get it over the finish line? @shagamemnon it looks like there is a few conflicts that need to be resolved, would you be available to do that? |
I tested this a while ago on some of our sites and it worked really well, with |
|
@shagamemnon looks like there's still some rebase conflicts! let me know if I can help in anyway |
|
Sorry, I'm not familiar with the release process here, but when should 0.1.2 make it to npm? |
Hopefully this week. I'll ping some folks tomorrow. 👍 |
|
hey all! working on getting the right permissions on my end to publish to NPM, apologies for the delay! |
The TL;DR on issue #122 : I discovered that when a Worker subrequest ...
caches.default.put()(likeetag: "xyc")etag: W/"xyc"before serving eyeballThen ...