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

feat(cache): use vary headers to compare cached response with request headers #3

Closed
wants to merge 15 commits into from

Conversation

IsakT
Copy link

@IsakT IsakT commented Jul 5, 2024

In this first commit, the cache handler handles multiple cached entries from the same cache key. It then tries to find which of the entries match the request, if any.

What I can't get my head around is how we are supposed to set the cache. We need both the response and the request at the same time. We need the vary headers and data from the response - the vary headers so that we can compare, the data to cache the response, and the request headers to have something to compare against.

My interpretation of the current functionality is that after we fetch an existing entry from the cache, the onComplete method caches that object again. What is the purpose of caching the same entry again?

lib/interceptor/cache.js Outdated Show resolved Hide resolved
lib/interceptor/cache.js Outdated Show resolved Hide resolved
lib/interceptor/cache.js Outdated Show resolved Hide resolved
lib/interceptor/cache.js Outdated Show resolved Hide resolved
lib/interceptor/cache.js Outdated Show resolved Hide resolved
lib/interceptor/cache.js Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
lib/interceptor/cache.js Outdated Show resolved Hide resolved
lib/interceptor/cache.js Outdated Show resolved Hide resolved
lib/interceptor/cache.js Outdated Show resolved Hide resolved
lib/interceptor/cache.js Outdated Show resolved Hide resolved
lib/interceptor/cache.js Outdated Show resolved Hide resolved
lib/interceptor/cache.js Outdated Show resolved Hide resolved
lib/interceptor/cache.js Outdated Show resolved Hide resolved
lib/interceptor/cache.js Outdated Show resolved Hide resolved
lib/interceptor/cache.js Outdated Show resolved Hide resolved
lib/interceptor/cache.js Outdated Show resolved Hide resolved
lib/interceptor/cache.js Outdated Show resolved Hide resolved
@IsakT
Copy link
Author

IsakT commented Aug 13, 2024

I implemented a CacheStore, with a simple interface so that it could be exchanged for a different cache if needed. Can we move the current implementation of CacheStore out of the cache.js file - maybe to util.js or to its own file? The reason would be so that we can export it and so that it can be tested and/or used elsewhere.

lib/interceptor/cache.js Outdated Show resolved Hide resolved
@IsakT
Copy link
Author

IsakT commented Aug 15, 2024

I tried to make the CacheStore class accessible from the outside using Symbol(), but couldn't get it to work. I exported it explicitly with export which works for now.

lib/interceptor/cache.js Outdated Show resolved Hide resolved
Copy link
Member

@ronag ronag left a comment

Choose a reason for hiding this comment

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

What about range queries? Are those implicit vary?

@ronag
Copy link
Member

ronag commented Aug 15, 2024

Is like to have max entry size and max entry ttl options

@ronag
Copy link
Member

ronag commented Aug 15, 2024

Also more tests wouldn't hurt

@IsakT
Copy link
Author

IsakT commented Aug 15, 2024

Is like to have max entry size and max entry ttl options

What should happen when the cache gets full? Simply ignore more entries to be stored, or delete an old one when there is a new entry?

Also, what do you mean with 'max entry ttl options'? The maximum amount of ttl a single entry can have?

@IsakT
Copy link
Author

IsakT commented Aug 15, 2024

What about range queries? Are those implicit vary?

No, technically Range headers are not implicit Vary, because a server could choose to ignore Range headers and send the same response no matter what the Range header is. But if the server deems them as valid of course, you could say they are practically implicit Vary. But then the server also needs to respond with 206 Partial Content and include a Content-Range header.

I will implement the case where the server responds with a 307 Redirect where it's also dependent on the Range header.

…ug where data structure weren't correctly parsed back from JSON
lib/interceptor/cache.js Outdated Show resolved Hide resolved
lib/interceptor/cache.js Outdated Show resolved Hide resolved
@IsakT IsakT marked this pull request as ready for review August 22, 2024 07:31
lib/interceptor/cache.js Outdated Show resolved Hide resolved
@ronag
Copy link
Member

ronag commented Sep 11, 2024

Continued in nodejs/undici#3562

@ronag ronag closed this Sep 11, 2024
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.

2 participants