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

fix(core): spread storage options #452

Merged
merged 2 commits into from
May 22, 2024

Conversation

vejja
Copy link
Collaborator

@vejja vejja commented May 13, 2024

Closes #433

Types of changes

  • Bug fix (a non-breaking change which fixes an issue)
  • New feature (a non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Description

The Rate Limiter options need to be spread in order to support all the unstorage drivers.

Previously, only the options property could be provided to the underlying driver, because this driver has only one property with the name options.

With this PR, any property name can be provided under the options property.

Checklist:

  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes (if not applicable, please state why)

Copy link

vercel bot commented May 13, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
nuxt-security ✅ Ready (Inspect) Visit Preview 💬 Add feedback May 21, 2024 10:39am

@vejja vejja self-assigned this May 13, 2024
@vejja vejja linked an issue May 13, 2024 that may be closed by this pull request
Copy link
Owner

@Baroshem Baroshem left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for working on it. Unfortunately it will be a breaking change but with a proper release notes, I think we can have it in 2.0.0-rc.1

@vejja
Copy link
Collaborator Author

vejja commented May 17, 2024

I'm not entirely sure it's a breaking change, it could actually be a bug fix.
It's very difficult to understand how nitro sets the unstorage options, and then how unstorage forwards these options to the underlying drivers. I'll do some local tests and will let you know.

@Baroshem
Copy link
Owner

I think it will be a breaking change purely because the interface for how certain feature will change. Wwith this change, users that uset the previous version of storage options, would have it not working anymore.

But no worries about that. This is not a huge breaking change as long as we provide release notes and migration guide.

@vejja
Copy link
Collaborator Author

vejja commented May 17, 2024

I think it will be a breaking change purely because the interface for how certain feature will change. Wwith this change, users that uset the previous version of storage options, would have it not working anymore.

Yes that's right

@vejja
Copy link
Collaborator Author

vejja commented May 21, 2024

@Baroshem : Update after further review
The bug @numselli found out for the redis driver was also a problem for all other drivers. The options were actually never passed to the underlying driver.
I'm fixing the TypeScript definitions to clarify how the options must be passed and what values are available for each driver.

Marking as ready now

@vejja vejja marked this pull request as ready for review May 21, 2024 10:45
@Baroshem
Copy link
Owner

Thanks for finding it and fixing it @vejja 💚!

I can't wait for this release. it will be 🔥

@Baroshem Baroshem merged commit d40ced0 into chore/2.0.0-rc.1 May 22, 2024
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Rate limiter driver options not being properly passed
2 participants