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

preflight - cache GET and OPTIONS requests separately #136

Closed
navtoj opened this issue Mar 1, 2023 · 5 comments · Fixed by #138
Closed

preflight - cache GET and OPTIONS requests separately #136

navtoj opened this issue Mar 1, 2023 · 5 comments · Fixed by #138

Comments

@navtoj
Copy link
Contributor

navtoj commented Mar 1, 2023

The cacher() middleware uses the same headers for both the preflight (OPTIONS) and main (GET) requests when the route is the same.

I think adding ctx.request.method to the key would fix the issue.

  1. Is the current behaviour intentional?
  2. Am I missing something?

https://github.com/sebringrose/peko/blob/836e830e1c2daa738b8d71a12bec999bd4c5ea6b/middleware/cacher.ts#L11

@navtoj
Copy link
Contributor Author

navtoj commented Mar 1, 2023

Or.. maybe it'd be better to let the user create a suffix for the cache key by exposing the ctx on the cacher()?

Current

https://github.com/sebringrose/peko/blob/836e830e1c2daa738b8d71a12bec999bd4c5ea6b/middleware/cacher.ts#L9-L11

Additions

function parameter key suffix
id: (ctx: RequestContext) => string -${id(ctx)}

Future

export const cacher = (cache: ResponseCache, id: (ctx: RequestContext) => string): Middleware => async (ctx, next) => { 
   const reqURL = new URL(ctx.request.url) 
   const key = `${reqURL.pathname}${reqURL.search}-${JSON.stringify(ctx.state)}-${id(ctx)}`

@sejori
Copy link
Owner

sejori commented Mar 1, 2023

Excellent catch and thanks for the detailed issue.

The request method should definitely be in the CacheItem key, I think it used to be but has maybe been removed. Either that or this has been a long-standing oversight.

I also like the idea of the optional key generator argument.

Suggested fix:

export const cacher = (cache: ResponseCache, keyGen?: (ctx: RequestContext) => string): Middleware => async (ctx, next) => { 
   const key = keyGen 
       ? keyGen(ctx) 
       : `${ctx.request.method}-${ctx.request.url.pathname}${ctx.request.url.search}-${JSON.stringify(ctx.state)}`

Would you be interested in submitting a PR for this? Would be great to have you as a contributer :)

@navtoj
Copy link
Contributor Author

navtoj commented Mar 1, 2023

Would you be interested in submitting a PR for this?

Yea, sure.

@sebringrose Are there any specific formatting guidelines for this repo?

@sejori
Copy link
Owner

sejori commented Mar 2, 2023

Basic formatting guidelines: (not enforced by linter, just habits I picked up from Deno docs)

  • only include ; semicolon where necessary
  • always use " double quotes instead of ' single quotes

You'll see requested changes on the PR. I realised we can define the default keygen function outside of the cacher for a minor performance boost.

@sejori
Copy link
Owner

sejori commented Mar 2, 2023

https://github.com/sebringrose/peko/releases/tag/1.7.3 🎉

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 a pull request may close this issue.

2 participants