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: add optional methods to the cache store #3747

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

Conversation

ivan-tymoshenko
Copy link
Contributor

The idea of this PR is to add some optional methods to the cache store that might not be used by the interceptor, but are useful for managing the cache. Right now it's hard to invalidate the cache from the outside of the package, because there is no way to know what values have been cached.

Any ideas about the intercase for these methods are welcome. I'm opening it's as a draft because it's missing a docs at least.

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

Looks good to me,

Please add types too.

lib/cache/memory-cache-store.js Show resolved Hide resolved
lib/cache/memory-cache-store.js Show resolved Hide resolved
@ivan-tymoshenko ivan-tymoshenko marked this pull request as ready for review October 22, 2024 09:38
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

lgtm

Copy link
Member

@flakey5 flakey5 left a comment

Choose a reason for hiding this comment

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

can you add type checks for the new methods in assertCacheStore please

function assertCacheStore (store, name = 'CacheStore') {

@@ -88,6 +92,10 @@ declare namespace CacheHandler {

get isFull (): boolean

getOrigins (): Promise<string[]>

getRoutesByOrigin (origin: string): Promise<{ method: string, path: string }[]>
Copy link
Member

Choose a reason for hiding this comment

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

nit but would prefer if the { method: string, path: string } type was defined outside of the fn declaration so it can be referenced elsewhere if needed (incl user space)

i.e.

export interface RouteByOrigin {
  method: string
  path: string
}

export interface CacheStore {
  getRoutesByOrigin(origin: string): Promise<RouteByOrigin[]>
  // ...
}

@ivan-tymoshenko
Copy link
Contributor Author

can you add type checks for the new methods in assertCacheStore please

function assertCacheStore (store, name = 'CacheStore') {

I'm not sure about that as these methods are optional, because they are not used by the undici package.

@mcollina
Copy link
Member

Ci does not look green

@ivan-tymoshenko
Copy link
Contributor Author

Ci does not look green

It fails in the main with the same error.

@ivan-tymoshenko ivan-tymoshenko marked this pull request as draft October 24, 2024 13:36
@ivan-tymoshenko
Copy link
Contributor Author

I make it a draft for now. Not sure about the interface.

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