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

feature: add sqlite support #14

Open
wants to merge 5 commits into
base: 2.x
Choose a base branch
from

Conversation

muco-rolle
Copy link

❓ Type of change

  • ✨ New feature (a non-breaking change that adds functionality)

📚 Description

This PR upgrades node-rate-limiter-flexible to the latest version, which includes SQLite support. It also updates the store configuration to integrate SQLite as a supported option.

Background

Our project uses SQLite as the main database, and some features require rate limiting. To enable this, I contributed SQLite support to node-rate-limiter-flexible. With this update, we can now leverage SQLite as a storage backend for rate limiting within this Adonis package.

Changes

  • Upgraded node-rate-limiter-flexible to the latest version
  • Updated the rate limiter store to support SQLite

Testing

  • Verified that the rate limiter works correctly with SQLite as the storage backend
  • Ensured that existing functionality remains unaffected

Let me know if any adjustments or improvements are needed!

@muco-rolle
Copy link
Author

Hello @thetutlage, could you please take a look at this?

@@ -20,6 +20,7 @@
"test": "npm run test:pg && npm run test:mysql && mkdir coverage/tmp && cp -r coverage/*/tmp/. coverage/tmp && c8 report",
"test:pg": "cross-env DB=pg c8 --reporter=json --report-dir=coverage/pg npm run quick:test",
"test:mysql": "cross-env DB=mysql c8 --reporter=json --report-dir=coverage/mysql npm run quick:test",
"test:sqlite": "cross-env DB=sqlite c8 --reporter=json --report-dir=coverage/sqlite npm run quick:test",
Copy link
Member

Choose a reason for hiding this comment

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

This script should be added to the Github workflows for tests to run for SQLite

@thetutlage
Copy link
Member

Can you please fix the type-checking errors? https://github.com/adonisjs/limiter/pull/14/files#file-tests-define_config-spec-ts-L166

@muco-rolle
Copy link
Author

@thetutlage Hi,

I've addressed the comments.

However, it looks like the toMatchTypeOf matcher is deprecated. Do you think we should remove it?

@thetutlage
Copy link
Member

I see the SQLite tests are failing. Can you please look into them?

@thetutlage
Copy link
Member

However, it looks like the toMatchTypeOf matcher is deprecated

We can replace it with the toEqualTypeOf

@muco-rolle
Copy link
Author

I see the SQLite tests are failing. Can you please look into them?

Hello @thetutlage

Redis was missing in the test-sqlite job, which seemed to be the reason the tests were failing. I've added it now, and it should be resolved. Let me know if everything looks good.

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.

2 participants