feat(cache-browser-local-storage): Implemented TTL support to cached items #1457
Conversation
… search if the TTL feature is disabled
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit 51f2510:
|
| if (!timeToLive) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
could return right after line 27 to save some more impact
There was a problem hiding this comment.
@Haroenv It has been updated 👍
There was a problem hiding this comment.
Sorry, I meant right after creating the timeToLive variable, so after line 27, we wouldn't need to do the Object.entries etc.
There was a problem hiding this comment.
Ah that's what you mean. This was done in order to deal with existing cache items that were made before this PR is merged.
Line 30 - 34 searches for old cache items and removes them, as they are no longer in a format that is supported.
| /** | ||
| * The time to live for each cached item in seconds. | ||
| */ | ||
| readonly ttl?: number; |
There was a problem hiding this comment.
I guess we can go full length here and use timeToLive to make it easier for everyone, wdyt?
There was a problem hiding this comment.
Took a look at some of the other Options.ts files, and I do agree with this suggestion.
Not only does it make it a bit easier to read, it also makes it more consistent with other configurations.
There was a problem hiding this comment.
It has been updated 👍
…and updated existing tests
|
I've added 2 new tests for this feature, Since this question might come up, I'll answer it in advance. |
|
@shortcuts Are there any updates yet regarding this PR? Or is it still being processed internally? |
oh sorry I forgot, checking! |
There was a problem hiding this comment.
looks good! thanks for the contribution :D I'll port it to the next major as stated here algolia/api-clients-automation#1489
|
@shortcuts Just making sure, I just have to update the |
exactly! |
Head branch was pushed to by a user without write access
|
@shortcuts It has been updated 👍 |
|
Hmm, seems like the bundle size is a bit bigger then when I locally ran the test. |
|
After a fresh compile the size was indeed bigger than before, this has been fixed now |
Feature summary
This introduces the ability to set a TTL for cached items, in order to reduce the size of this cache and prevents the cache from permanently holding onto the items.
This feature is optional, and if the
timeToLiveoption is not configured, it will behave like how it currently does.Why?
We ran into storage quotas, and saw that this cache can grow to a couple MB after a while. We made our own patched version, but it would be nice if this feature was implemented upstream.
Providing this fix in the form of a pull request was discussed via the support.algolia.com/hc/requests/551781 support ticket.