Clarification Added: req-fresh.md5 #1349
Conversation
More details added a better understanding, of how it works.
Co-authored-by: Nirav <61644078+srkds@users.noreply.github.com>
|
Is anyone checking this PR? |
|
Hi, sorry about that! I have two main comments so far:
|
crandmck
added
the
needs tech review
There was a problem hiding this comment.
I think that this doc can be improved, but the proposed changes don't make the subject clearer.
Cache management when it comes to 304s and Etags is handled by default by express. It's a nontrivial job which is abstracted away in the default case, and users still have the tools to manage it themselves if they so choose. req.fresh is one of those tools.
An improvement would be to better document what "fresh" means here, and how express determines it. We don't really want to encourage folks to try to set etags themselves via examples unless they know what they're doing.
| When a client sends the `Cache-Control: no-cache` request header to indicate an end-to-end reload request, this module will return `false` to make handling these requests transparent. | ||
|
|
||
| Further details for how cache validation works can be found in the | ||
| [HTTP/1.1 Caching Specification](https://tools.ietf.org/html/rfc7234). | ||
|
|
||
| ```js | ||
|
|
||
| // Get value for ETag where you saved it initially, In general you |
There was a problem hiding this comment.
This comment isn't very clear. The added example is setting an etag not reading a value.
users do not need to manage or store timestamps to get etags working, so I think this will confuse people
| @@ -2,12 +2,19 @@ | |||
|
|
|||
| When the response is still "fresh" in the client's cache `true` is returned, otherwise `false` is returned to indicate that the client cache is now stale and the full response should be sent. | |||
|
|
|||
| In order to check whether the response is still "fresh" in the client's cache or not, two tags will be used ETag and If-None-Match. ETag will be fetched from response (saved internally from application) and other tag will be received from client's request. | |||
There was a problem hiding this comment.
The headers which go into determing fresh are seen here in jshttp/fresh
- If-None-Match
- Etag
- Cache-Control: no-cache
- If-Modified-Since
- Last-Modified
| // Get value for ETag where you saved it initially, In general you | ||
| // may use a file to store timestamps with API request name | ||
|
|
||
| res.set('ETag', "foo") |
There was a problem hiding this comment.
The example that existed wasn't very clear to begin with. Setting an etag here doesn't mean that req.fresh will be true, so the example gets less clear with the addition.
It's easier perhaps to demonstrate that setting a new, highly random etag on every request can guarantee that req.fresh is false. That way we create an example which will always have the demonstrated output.
| @@ -2,12 +2,19 @@ | |||
|
|
|||
| When the response is still "fresh" in the client's cache `true` is returned, otherwise `false` is returned to indicate that the client cache is now stale and the full response should be sent. | |||
|
|
|||
| In order to check whether the response is still "fresh" in the client's cache or not, two tags will be used ETag and If-None-Match. ETag will be fetched from response (saved internally from application) and other tag will be received from client's request. | |||
There was a problem hiding this comment.
Hey @gulgulia17,
@crandmck said I could leave any feedback for your PR. I'm not a maintainer here or anything, so you don't have to listen to me :]. It's all just punctuation related stuff.
- Add a colon to this:
In order to check whether the response is still "fresh" in the client's cache or not, two tags will be used: ETag and If-None-Match. - Add a comma in the middle + missing 'the':
ETag will be fetched from response (saved internally from application), and the other tag will be received from client's request.
More details added a better understanding, of how it works.