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

[http-cache]: handle cache invalidation of location and content-location headers #3735

Draft
wants to merge 12 commits into
base: main
Choose a base branch
from

Conversation

Uzlopak
Copy link
Contributor

@Uzlopak Uzlopak commented Oct 15, 2024

this is a follow up to the follow up.

const result = this.#store.deleteByOriginAndPath(this.#requestOptions.origin, targetURL.pathname)
// Fail silently if it is a promise/async function
result && result.catch && result.catch(noop)
} catch (err) { /* Fail silently */ }
Copy link
Member

Choose a reason for hiding this comment

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

I think this should be awaited if it's a promise.

Base automatically changed from followup-http-cache to main October 15, 2024 21:55
@Uzlopak Uzlopak marked this pull request as draft October 15, 2024 21:57
@ivan-tymoshenko
Copy link
Contributor

@Uzlopak What do you think about making this method more generic for calling it outside of the undici package. I have two suggestions:

  • pass an array of paths instead of one path. This might be useful for another cache store types if user wants to delete mutiple paths. These storages might perform a batch delete operation much faster than deleting it one by one.
  • pass also an http method and not only the path. For most cases users would want to cache only GET/HEAD requests, but still someone might want to cache other methods, so it would be nice to have an ability to delete routes from the cache independenty

const cachedPaths = this.#data.get(origin)
for (const key of cachedPaths.keys()) {
// not good to use startsWith here, but it's fine for now
if (key.startsWith(path)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I would split the key here by the : because the MemoryCacheStore indeed stores keys in a format path:method, but this might be not true for other types of stores. So if someone will rely on this it might be a breaking change in the future.

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.

3 participants