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

[Draft] Memoize limit #281

Open
wants to merge 14 commits into
base: master
Choose a base branch
from
Open

Conversation

EvandroLG
Copy link
Contributor

My intention is to add an option to limit the maximum number of results being cached, this way we can guarantee that the in-memory object will never be storing more results than we want.

It's just a draft of the solution using Map. In the final solution, I think we should support environments that don't implement Map. I would like to hear your thoughts before jumping to the final solution @angus-c :)

@angus-c
Copy link
Owner

angus-c commented Aug 8, 2021

Thanks for sharing your proposal!

So this is really two enhancements? 1) limiting cache size 2) being able to cache by object key instead of string key.

On (1)
I like this. Maybe move options argument to last so this does not need to be a breaking change? Also could just call it max for transparency?
On (2)
How will we implement for environments that don't implement Map? I'm a little concerned the code could end up very long.

@EvandroLG
Copy link
Contributor Author

The improvement I had initially thought of only covered limiting cache size, but you are right that using Map we could use any kind of type as cache key.

  1. I would keep it as options since we can further implement more options, e.g cache lifetime. I agree that we should move options to last.
  2. I just pushed a tiny solution for environments that don't support Map. In case we want to store any kind of key as the original Map works, I have to update the code slightly. For simplicity, I would prefer to go with this solution. Let me know what you think about this implementation :)

@angus-c
Copy link
Owner

angus-c commented Aug 30, 2021

Sorry for the delay! To be clear I think it's probably to restrict this PR to cache size limit. I like to keep just functions simple and with limited options :)

@EvandroLG
Copy link
Contributor Author

@angus-c sorry for the delay!
In that case, would we only support browsers that support Map or would you have another solution for that problem?

@angus-c
Copy link
Owner

angus-c commented Oct 30, 2021

Hey @EvandroLG! I would prefer just to not use map at all (or a polyfill). The problem is I want to support pre-map environments while also keeping the code short and simple. So can we just limit the change to the cache size limit?

@EvandroLG
Copy link
Contributor Author

Yes, we can change the implementation, but in the end we would use a solution using Object and Array, just like this fallback class, right? Or would you have another approach for this problem?

@angus-c
Copy link
Owner

angus-c commented Nov 3, 2021

Maybe hold onto this plan for a short while. I'm thinking of allowing ES code soon, once I move to all ESM

@angus-c angus-c added the maybe label Mar 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants