-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
Dynamic cache length overriding #2755
Conversation
For the Endpoint badge: #2473. `request-handler.js` is such a bear. I’m looking forward to being able to rewrite it when the service refactor is done.
|
@@ -140,9 +147,13 @@ function handleRequest(cacheHeaderConfig, handlerOptions) { | |||
return | |||
} | |||
if (requestCache.has(cacheIndex)) { | |||
const cached = requestCache.get(cacheIndex).data |
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.
Updated for consistency with same-named variable above.
OK, so having spent some time reading over this, I think I can see what's going on: You've got ..yeah? And it also looks correct. I can see how we've ended up at this point (one edge-case at a time) and I get that its difficult to have a single place in the codebase that encodes all of that logic because we want to introduce each of those params at different layers of the application for different reasons (also I accept that I've contributed some of this complexity). This set of rules is going to be non-obvious to a contributor who isn't already deeply familiar with the codebase and "how long will this badge be cached for" may be a hard question to answer or reason about. I think as time goes on and we get rid of the rest of the legacy services, we can ditch 2 of these layers, but until that time this set of rules seems like it could really use some docs to explain that chain of overrides. Does that sound reasonable? |
Yes, that is a great summary of what is happening. Maybe we can just paste it into The more I understand the code in |
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.
seems fair enough
Co-Authored-By: paulmelnikow <github@paulmelnikow.com>
Thanks for reviewing! |
For the Endpoint badge: #2473.
request-handler.js
is such a bear. I’m looking forward to being able to rewrite it when the service refactor is done.