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: use storage and invalidation #7

Merged
merged 22 commits into from
Dec 23, 2021

Conversation

simone-sanfratello
Copy link
Collaborator

@simone-sanfratello simone-sanfratello commented Nov 5, 2021

proposal for use of an external storage, with optional invalidation


TODO

  • test on storage/s
  • 100% coverage (and significant cases)
  • use an empty logger on storage
  • fix CI, add a redis instance
  • restore expiration logic on memory storage / do not use Date.now
  • expose explicit "invalidation" function
  • improve redis storage (pipeline etc)
  • storage gc function
  • memory storage with sync deletes
  • redis storage with sync deletes
  • optional invalidation
  • benchmarks
  • redis references TTL
  • redis gc
  • "stale-on-error" > next
  • solve TODOs
  • check redis pipeline
  • memory references: use a btree? a set? arrays are good enough
  • example
    • basic
    • mjs
  • update documentation

index.js Outdated Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved
@simone-sanfratello simone-sanfratello marked this pull request as draft November 7, 2021 07:14
@simone-sanfratello
Copy link
Collaborator Author

still have to write test on storage and reach 100% coverage

storage/memory.js Outdated Show resolved Hide resolved
storage/memory.js Outdated Show resolved Hide resolved
storage/memory.js Outdated Show resolved Hide resolved
storage/memory.js Outdated Show resolved Hide resolved
storage/memory.js Outdated Show resolved Hide resolved
storage/memory.js Outdated Show resolved Hide resolved
storage/redis.js Outdated Show resolved Hide resolved
storage/redis.js Outdated Show resolved Hide resolved
@mcollina
Copy link
Owner

mcollina commented Nov 7, 2021

great work!

@simone-sanfratello
Copy link
Collaborator Author

simone-sanfratello commented Nov 7, 2021

I'm collecting on top all the points
still have to work a lot on redis :)
and on gc

storage/redis-worker.js Outdated Show resolved Hide resolved
storage/memory.js Outdated Show resolved Hide resolved
storage/memory.js Outdated Show resolved Hide resolved
storage/redis.js Outdated Show resolved Hide resolved
storage/redis.js Outdated Show resolved Hide resolved
storage/redis.js Outdated Show resolved Hide resolved
storage/redis.js Outdated Show resolved Hide resolved
storage/redis.js Outdated Show resolved Hide resolved
storage/redis.js Outdated Show resolved Hide resolved
@simone-sanfratello simone-sanfratello changed the title draft: use storage draft: use storage and invalidation Nov 28, 2021
@simone-sanfratello
Copy link
Collaborator Author

@mcollina wait to review, still a wip

@mcollina
Copy link
Owner

looks good!

@simone-sanfratello simone-sanfratello changed the title draft: use storage and invalidation feat: use storage and invalidation Dec 1, 2021
Copy link
Owner

@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.

Good work!

storage/redis.js Outdated Show resolved Hide resolved
storage/memory.js Outdated Show resolved Hide resolved
examples/redis.js Outdated Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved
storage/memory.js Outdated Show resolved Hide resolved
storage/memory.js Outdated Show resolved Hide resolved
storage/memory.js Outdated Show resolved Hide resolved
storage/memory.js Outdated Show resolved Hide resolved
storage/memory.js Outdated Show resolved Hide resolved
Copy link
Owner

@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.

This needs lots of docs, specifically how to use references

@simone-sanfratello
Copy link
Collaborator Author

This needs lots of docs, specifically how to use references

I'm thinking of doing am extended use case in mercurius/cache and link there for documentation, for 2 reason: it's hard to explain references talking with abstractions, and the result of documentation here would be very close to the mercurius/cache one

wdyt?

@mcollina
Copy link
Owner

Just do one and copy it over/adapt here. I think this deserves some docs anyway.

@simone-sanfratello
Copy link
Collaborator Author

simone-sanfratello commented Dec 16, 2021

@mcollina apart of documentation, the code is done
there are some points in the code marked with TODO, we can probably check them later
I'm writing doc for mercurius-cache for references and invalidation, then I'll port here

Copy link
Owner

@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.

Could you create a "Maintainers" Section on the README and add both yourself and myself there? This module is as much yours as it is mine now.

Also add yourself to the package.json too.

index.js Outdated Show resolved Hide resolved
@simone-sanfratello
Copy link
Collaborator Author

@mcollina done!

Copy link
Owner

@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

@mcollina mcollina merged commit e1b8507 into mcollina:main Dec 23, 2021
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.

None yet

2 participants