feat: cache etag support #3758
feat: cache etag support #3758
Conversation
Closes nodejs#3718 Signed-off-by: flakey5 <[email protected]>
|
|
||
| // Check for the response's etag header, ignore if there's multiple | ||
| if (typeof headers.etag === 'string') { | ||
| value.etag = headers.etag |
There was a problem hiding this comment.
Shouldn't etag be validated here? https://httpwg.org/specs/rfc9110.html#field.etag
There was a problem hiding this comment.
and also account for weak etags
|
I think this is a bit too light on tests, and I'm not sure if this implementation is correct. Can you add some tests where the response is a 304 thanks to if-none-match? |
Also, we could test how undici responds to different server implementations. I myself have run into situations when locally stored strong ETag matches the server-sent one yet the server is returning the same ETag with 200 instead of 304. |
Re http caching, adds support for etags for when we need to revalidate a cached response.
No breaking changes to the public api. the
etagproperty is nullable so a cache store could just not implement it and it will be fine. However, I did add it to the tests as if it was required just so we can ensure the builtin cache stores fully support the behavior.This only checks if the etag exists in the response's headers and ignores it if it's in the trailers for simplicity sakes (as allowed by the spec in 8.8.3)
Closes #3718