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

v2.6.0 Add cache control string to the router #101

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,7 @@ const routerInstance = router(server, {
appEnv: process.ENV.APP_ENV, // optional, used to specify the env in Sentry, defaults to "unknown"
logger: logger, // pass in the luxuryescapes logger and the error handler will use this, resulting in single line log messages in new relic with stack traces.
sanitizeKeys: [/_token$/, /password/i, "someHideousKey", "path.with.dot"], // array of keys or paths to sanitize from the request body, query and params on error
cacheControlString: 'public, max-age=600', // you can pass cache directives in a ',' separated string string. Reference https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Cache-Control#cache_directives
Copy link
Contributor

@henryshen-le henryshen-le Oct 27, 2024

Choose a reason for hiding this comment

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

Can you add a test for this? I think theres a test file with some examples. Or I can add one for you.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will add the test later todat

})

// define routes
Expand Down
6 changes: 6 additions & 0 deletions index.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -119,6 +119,12 @@ declare module "@luxuryescapes/router" {
* @defaultValue `{}`
*/
jsonOptions?: { [option: string]: string | number | boolean | null | undefined };

/**
* Cache-Control header value
* @see https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Cache-Control
*/
cacheControlString?: string;
Copy link
Contributor

@henryshen-le henryshen-le Oct 27, 2024

Choose a reason for hiding this comment

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

Would it be better to abstract away the syntax and make it easier to use, e.g.

Suggested change
cacheControlString?: string;
cacheControl?: {
type: 'public', // More options can be added when required
TTLInSeconds: number,
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think we should, this would limit it too much, the spec is very wide and has many options that can be combined in different ways.

https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Cache-Control

}

interface SchemaRouteOptions {
Expand Down
10 changes: 10 additions & 0 deletions lib/middleware/cache-control.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
module.exports = exports = ({ cacheControlString }) => {
return function responseCacheControl (req, res, next) {
const originalFunc = res.json
res.json = (responseBody) => {
res.set('Cache-Control', cacheControlString)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I wonder if we should do something to check the res.status?
e.g. only set it if it's 2xx

return originalFunc.call(res, responseBody)
}
next()
}
}
7 changes: 6 additions & 1 deletion lib/router.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,14 +7,15 @@ const createErrorHandler = require('./middleware/error-handler')
const networkLogger = require('./middleware/network-logger')
const requestValidator = require('./middleware/request-validator')
const responseValidator = require('./middleware/response-validator')
const responseCacheControl = require('./middleware/cache-control')
const sentry = require('./sentry')

const handle = (
app,
method,
routeDefinitions,
{ validateResponses, logRequests, logResponses, correlationIdExtractor, logger, appEnv },
{ url, operationId, preHandlers, handlers, schema, isPublic, tags, summary, description, deprecated, warnOnRequestValidationError, jsonOptions }
{ url, operationId, preHandlers, handlers, schema, isPublic, tags, summary, description, deprecated, warnOnRequestValidationError, jsonOptions, cacheControlString }
) => {
let _handlers = [express.json(jsonOptions || {})]

Expand Down Expand Up @@ -43,6 +44,10 @@ const handle = (
_handlers.push(responseValidator({ schema: schema.responses }))
}

if (cacheControlString) {
_handlers.push(responseCacheControl({ cacheControlString }))
}

_handlers = [..._handlers, ...handlers].map(handler => {
const wrappedHandler = asyncMiddleware(handler)
// this ensures that the original middleware name is preserved and not rewritten to 'asyncMiddleware'
Expand Down
2 changes: 1 addition & 1 deletion package.json
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
{
"name": "@luxuryescapes/router",
"version": "2.5.32",
"version": "2.6.0",
"description": "Wrapper around express router that provides validation and documentation out of the box",
"main": "index.js",
"types": "index.d.ts",
Expand Down