-
Notifications
You must be signed in to change notification settings - Fork 135
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: add dragonfly
driver
#353
base: main
Are you sure you want to change the base?
Conversation
β Live Preview ready!
|
src/index.ts
Outdated
@@ -12,6 +12,7 @@ export const builtinDrivers = { | |||
cloudflareKVBinding: "unstorage/drivers/cloudflare-kv-binding", | |||
cloudflareKVHTTP: "unstorage/drivers/cloudflare-kv-http", | |||
cloudflareR2Binding: "unstorage/drivers/cloudflare-r2-binding", | |||
dragonfly: "unstorage/drivers/redis", |
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.
We might need a entry file even if it is simple reexport
At least defaults such as tls
and port
could be enabled by default also this way (haven't tried, if needed)
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.
@pi0 Your own export is a good idea.
However, in my opinion, first the Redis interface in the repository needs to be better organized. There are more and more libraries that have a virtually identical interface and unstorage will certainly support more of them in the future.
The difference between Redis
, Dragonfly
, VercelKV
, Upstash
etc. is nominal and it is possible to create a universal interface for all drivers that use redis.
With dragonfly unstorage
would already have 3 drivers using redis, so it's high time to organize it better.
I have an idea to create a separate PR in which the current drivers (Redis and VercelKV) would be separated into utils
and thus, for each new driver that uses redis, it would be possible to import ready-made functions with their own configurations instead of constantly repeating the code.
And then go back to this PR, do a separate export and use these utils. What do you think about it?
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.
We can extend from redis and refactor other drivers later if can save us maintenance time but give same possibilities why not π― (I prefer feat > refactor)
However keep in mind, while all these are redis-compatible drivers, some like vercel one, depends on specific SDK like @vercel/kv
that essentially wraps with default environment variables. Yes, we can remove this layer but it requires us to keep in sync with what env variables vercel provides.
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.
@pi0 I currently don't have a good idea or time on how to deal with the topic mentioned in the comments above.
So I made a clone of the redis driver for dragonfly and corrected and updated the pull request.
Close or merge whatever you think is right. Due to these comments, if something comes to my mind, I will open a discussion or a new pull request.
dragonfly
dragonfly
driver
π Linked issue
Close #294
β Type of change
π Description
This PR adds information about dragonfly in the documentation and an additional export for better DX.
π Checklist