Move redis adapter to separate npm library

epicweb-dev / cachified

🤑 wrap virtually everything that can store by key to act as cache with ttl/max-age, stale-while-validate, parallel fetch protection and type-safety support

MIT License

916 stars 26 forks source link

Move redis adapter to separate npm library #87

Closed mannyv123 closed 8 months ago

mannyv123 commented 8 months ago

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
this PR then updates @epic-web/cachified to remove the redis adapter code and update the readme.md file to link to cachified-redis-adapter

Note: I wasn't too sure how to update the extractExamples.js file. I can see that since the readme.md file is update it has removed the redis adapter example from generating. Any suggestions here would be appreciated!

@tearingItUp786

Xiphe commented 8 months ago

Hi @mannyv123 thanks for taking care of this.

For context: The extractExamples.js + readme.test.ts ensure that the example code in the readme is correct & working. For the redis adapter examples the setup was too complex to actually run the examples so we just make sure there are not type-issues in there.

As you removed the redis example, we no longer need to validate that one -> can remove it from the test as you did.

kentcdodds commented 8 months ago

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!

mannyv123 commented 8 months ago

Thanks everyone for the feedback! I've made the following changes:

removed redis3 adapter code and related tests
removed redis-mock, @types/redis-mock, redis4 packages

I also added BREAKING CHANGE to the commit message, hopefully formatted it correctly🤞

Thanks again!

tearingItUp786 commented 8 months ago

Hey hey! I'll probably be helping @mannyv123 with this 😄

So the plan for the work going forward:

remove adapters.ts
Update the README
Update the extractExamples.js or do we want to remove this?

kentcdodds commented 8 months ago

I think we can remove that. Thanks @tearingItUp786!

Xiphe commented 8 months ago

Voting to keep extractExamples.js but as the creator of it I'm certainly biased :)

I've created that one because I tend to forget keeping (readme) examples up to date and wanted to have a safeguard. Let me know when you want my help with it.

kentcdodds commented 8 months ago

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.

Xiphe commented 8 months ago

The adapters were actually the examples that have not been tested :) But I'm completely fine with removing it 👍

mannyv123 commented 8 months ago

Hi all!

I've made the changes as discussed:

removed adapters and tests
removed extractExamples.js and related tests
updated the README

@kentcdodds you mentioned during the last office hours that it would be good to update the usage example in the README with the lru adapter code and that we might be able to reduce the amount of code as well. I updated the example with the lru adapter but wasn't too sure how to make it simpler. Would you be able to provide some feedback/guidance on how to update this? Thank you!

mannyv123 commented 8 months ago

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!

mannyv123 commented 8 months ago

Hi all,

I resolved the merge conflicts that were in the package.json file.

Also, I made an update to the tsconfig.json file as when tsc was run, it was no longer creating the dist/src folder structure. It seems this was caused when I removed the readme.test.ts file. Please let me know if my change here was incorrect and I can update.

Thanks!