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

Conversation

marhalpert
Copy link
Contributor

@marhalpert marhalpert commented Oct 24, 2024

This pull request introduces a new feature to support cache control headers in responses and updates.

Cache Control Feature:

  • README.md: Added cacheControlString option to the router configuration to allow passing cache directives.
  • lib/middleware/cache-control.js: Implemented new middleware to set the Cache-Control header in responses.
  • lib/router.js:
    • Imported the new responseCacheControl middleware.
    • Added cacheControlString to the handler configuration and included the middleware in the handler stack if cacheControlString is provided.

Version Update:

  • package.json: Updated the package version from 2.5.32 to 2.6.0.

Usage

const TTL = 60 * 10 // 10 minutes
const mount = (router: RouterAbstraction): void => {
  router.get({
    url: `${basePath}/program-configuration`,
    operationId: 'programConfigurationRead',
    isPublic: true,
    schema: programConfigurationReadSchema,
    preHandlers: [],
    handlers: [programConfigurationReadHandler],
    summary: 'Read programConfiguration',
    description: 'Get programConfiguration',
    cacheControlString: `public, max-age=${TTL}`,
  })
}

before

Screenshot 2024-10-24 at 5 28 17 pm

after

Screenshot 2024-10-24 at 5 28 40 pm

@marhalpert marhalpert requested review from henryshen-le and nguyenchr and removed request for nguyenchr October 24, 2024 06:24
@marhalpert marhalpert changed the title v2.6.00 Add cache control string to the router v2.6.0 Add cache control string to the router Oct 24, 2024
@@ -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

* 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

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants