-
Notifications
You must be signed in to change notification settings - Fork 26
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
Move redis adapter to separate npm library #87
Conversation
Move redis adapter to separate npm library
Hi @mannyv123 thanks for taking care of this. For context: The As you removed the redis example, we no longer need to validate that one -> can remove it from the test as you did. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code-wise: LGTM
But we need to decide if we want to release this as a breaking change or not.
- A) When we want to actually remove the adapter:
AddBREAKING CHANGE
to one of the commit messages (or to the squash merge message) in order to issue a new major version. See semantic-release#commit-message-format for details. - B) When we don't want to create a new major version just yet:
Keep the adapter in and@deprecate
. The docs can stay updated to point towards the new package.
Leaving this decision to you @kentcdodds
Thanks for this @mannyv123! I would prefer that we also remove the Redis 3 adapter as part of the breaking change. I don't think it's necessary for anyone to create a library for that because it's so little code people can just copy it themselves. I think we can keep the lru-cache adapter though. That one is quite simple, unlikely to change, and makes this package useful out of the box. So if you also remove the Redis 3 adapter code then I'm good to go with a major version bump as a part of this PR. Thanks! |
BREAKING CHANGE: remove redis adapter to external package
Thanks everyone for the feedback! I've made the following changes:
I also added Thanks again! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, the more I think about it, the more I think I would prefer to delete all adapters. And I think the README could be cleaned up as well. Do you have time and interest in doing that please @mannyv123?
Hey hey! I'll probably be helping @mannyv123 with this 😄 So the plan for the work going forward:
|
I think we can remove that. Thanks @tearingItUp786! |
Voting to keep I've created that one because I tend to forget keeping (readme) examples up to date and wanted to have a safeguard. |
I think it's a cool idea, but with removing all the adapters and moving the docs for them into their respective packages combined with the level of complexity there, I don't think it makes sense to keep it around. |
The adapters were actually the examples that have not been tested :) |
Hi all! I've made the changes as discussed:
@kentcdodds you mentioned during the last office hours that it would be good to update the usage example in the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is looking good to me. Definitely simplifies things for us development-wise and reduces the chance that we'll need breaking changes in the future.
Just one thing on the example in the README and then I'll be ready to merge this.
README.md
Outdated
/* defines options for the set method for lru cache */ | ||
interface LRUishCache extends Omit<Cache, 'set'> { | ||
set( | ||
key: string, | ||
value: CacheEntry<unknown>, | ||
options?: { ttl?: number; start?: number }, | ||
): void; | ||
} | ||
|
||
/* creates a wrapper for the lru cache so that it can easily work with cachified and ensures the lru cache cleans up outdated values itself*/ | ||
function lruCacheAdapter(lruCache: LRUishCache): Cache { | ||
return { | ||
set(key, value) { | ||
const ttl = totalTtl(value?.metadata); | ||
return lruCache.set(key, value, { | ||
ttl: ttl === Infinity ? undefined : ttl, | ||
start: value?.metadata?.createdTime, | ||
}); | ||
}, | ||
get(key) { | ||
return lruCache.get(key); | ||
}, | ||
delete(key) { | ||
return lruCache.delete(key); | ||
}, | ||
}; | ||
} | ||
|
||
const lru = lruCacheAdapter(lruInstance); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove this stuff and just set lru
to the cache instance itself. So something like:
const lru: Cache = {
set() {},
get() {},
delete() {},
}
Of course you'll want to fill that in with working code and make sure the types are happy too. I think you'll want to use the Cache generic maybe.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree with Kents feedback. Rest: LGTM!
Thank you for the feedback! I've updated the usage example now. Please let me know if this is in line with what you were thinking or if any changes are needed. Thanks again! |
delete(key) { | ||
return lruInstance.delete(key); | ||
}, | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This implementation has no benefit over
const cache = new LRUCache<string, CacheEntry>({ max: 1000 })
cachified({ cache, /* ... */ });
One of the main reasons for an adapter is to set the TTL on the cached item in a way that the cache will clean it up itself without involving cachified
.
Proposing to either not create an on the fly adapter (example above) or do the following:
/* lru cache is not part of this package but a simple non-persistent cache */
const lruInstance = new LRUCache<string, CacheEntry>({ max: 1000 });
const cache: Cache = {
set(key, value) {
const ttl = totalTtl(value?.metadata);
return lruCache.set(key, value, {
ttl: ttl === Infinity ? undefined : ttl,
start: value?.metadata?.createdTime,
});
},
get(key) {
return lruInstance.get(key);
},
delete(key) {
return lruInstance.delete(key);
},
};
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would vote for the latter option. In real apps this is quite critical in order to not bloat your cache db.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, I agree. Let's update the example to the one which sets the ttl 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ahh that makes sense! Thank you for the help!
I've updated it now to the above example.
Hi all, I resolved the merge conflicts that were in the package.json file. Also, I made an update to the Thanks! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 🐋
Good catch with the src
folder. The other option would have been to remove the src/
in the "type"
paths in package.json but I'm fine with either 👍
🎉 This PR is included in version 5.0.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Hi, everyone! 👋 I just updated the package to |
@caiocmpaes opened up a PR for my bud :) |
@caiocmpaes @tearingItUp786 PR is now merged! :) |
Description
This change removes the redis 4 adapter from
@epic-web/cachified
and links to a separate npm library I set up:cachified-redis-adapter
Why
This helps avoid issues when updating the adapter and causing breaking change releases for the entire
@epic-web/cachified
package.Details
cachified-redis-adapter
basically takes the code that was in@epic-web/cachified
for the redis adapter and moves it into a separate library@epic-web/cachified
to remove the redis adapter code and update thereadme.md
file to link tocachified-redis-adapter
Note: I wasn't too sure how to update the
extractExamples.js
file. I can see that since thereadme.md
file is update it has removed the redis adapter example from generating. Any suggestions here would be appreciated!@tearingItUp786