Skip to content
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

Clarification Added: req-fresh.md5 #1349

Open
wants to merge 2 commits into
base: gh-pages
Choose a base branch
from

Conversation

gulgulia17
Copy link

More details added a better understanding, of how it works.

More details added a better understanding, of how it works.
@gulgulia17 gulgulia17 changed the title Clarification Added Clarification Added: req-fresh.md5 Jul 6, 2022
@gulgulia17
Copy link
Author

Is anyone checking this PR?

@dougwilson
Copy link
Contributor

Hi, sorry about that!

I have two main comments so far:

  1. We should edit the 5x docs with the same updates as here, as they are the same currently.
  2. These seem incomplete, as noteably it leaves out the timestaml checking. Also as a nit the if-none-match is actually a list of tags, not just a single tag.

@crandmck crandmck added the needs tech review A doc edit that requires technical review before merging label Mar 6, 2024
Copy link

@waleedtariq109 waleedtariq109 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Everything looks good to me; feel free to merge. @crandmck

Copy link
Member

@jonchurch jonchurch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.
Copy link
Member

@jonchurch jonchurch Mar 8, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.
Copy link
Contributor

@chrisdel101 chrisdel101 Apr 9, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

  1. 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.
  2. 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs tech review A doc edit that requires technical review before merging pr
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants