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 timestampinfo to requests #319
Conversation
rebasing... |
a2e23f1
to
92c05e9
Compare
@@ -18,6 +18,8 @@ app.use(require("errorhandler")(errorhandlerOptions)); | |||
// bibrefs | |||
app.get('/bibrefs', function (req, res, next) { | |||
var refs = req.query["refs"]; | |||
res.setHeader("Expires", new Date(Date.now() + 86400000).toUTCString()); |
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.
Why 24h? Fwiw, the update script runs every hour.
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.
24 hours seems like a fairly sane default per spec, no?
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.
Yeah, It's rare that we get manual update requests nowadays, so I guess the use-case where someone just fixed the db and wants to see the update right away in their spec is rare enough.
@@ -9,7 +9,7 @@ if (process.env.NODE_ENV == "dev" || process.env.NODE_ENV == "development") { | |||
errorhandlerOptions.dumpExceptions = true; | |||
errorhandlerOptions.showStack = true; | |||
} | |||
|
|||
app.enable("etag"); |
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.
Given the app is restarted every time there is a change in the DB, we're not going to get anything out of this unless the etags are stored somewhere. If so, where?
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.
Oh, wait. They're content hashes, no? So that should work.
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.
Yeah, I think they are content hashes:
https://github.com/expressjs/express/blob/3ffceff3ed356a711fce5fee6ed00a03f43affd8/lib/utils.js#L21
Alright, this LGTM. Let's give it a spin. Mind messaging spec-prod to let them know we've shipped this? |
Will do! Thanks! Will this be deployed straight away? I'm cooking up some new goodness based on this :) |
YAY! I can see the 304 in https://specref.herokuapp.com/bibrefs?refs=dom 💃 |
Yup, and the proper cache headers are there too. Awesome! |
No description provided.