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

feat: add dragonfly driver #353

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

oritwoen
Copy link
Contributor

πŸ”— Linked issue

Close #294

❓ Type of change

  • πŸ“– Documentation (updates to the documentation, readme, or JSdoc annotations)
  • 🐞 Bug fix (a non-breaking change that fixes an issue)
  • πŸ‘Œ Enhancement (improving an existing functionality like performance)
  • ✨ New feature (a non-breaking change that adds functionality)
  • 🧹 Chore (updates to the build process or auxiliary tools and libraries)
  • ⚠️ Breaking change (fix or feature that would cause existing functionality to change)

πŸ“š Description

This PR adds information about dragonfly in the documentation and an additional export for better DX.

πŸ“ Checklist

  • I have linked an issue or discussion.
  • I have updated the documentation accordingly.

Copy link

nuxt-studio bot commented Dec 10, 2023

βœ… Live Preview ready!

Name Edit Preview Latest Commit
unstorage Edit on Studio β†—οΈŽ View Live Preview a170f67

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",
Copy link
Member

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)

Copy link
Contributor Author

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?

Copy link
Member

@pi0 pi0 Dec 21, 2023

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.

Copy link
Contributor Author

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.

@pi0 pi0 changed the title feat(driver): add export and info about dragonfly feat: add dragonfly driver Dec 12, 2023
@pi0 pi0 added the driver label May 1, 2024
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.

Dragonfly driver (Redis)
2 participants