Skip to content

added example implementing custom store using Sequelize ORM for PostgreSQL database; renamed example-custom to example-knex; added links of examples implementing custom-store to README.md#119

Merged
mcollina merged 6 commits intofastify:masterfrom
the-vishal-kumar:master
Dec 16, 2020

Conversation

@the-vishal-kumar
Copy link
Contributor

…reSQL database

renamed example-custom to example-knex
added links of examples implementing custom-store to README.md

Checklist

…reSQL database

renamed example-custom to example-knex
added links of examples implementing custom-store to README.md
@the-vishal-kumar the-vishal-kumar marked this pull request as ready for review December 14, 2020 10:56
Copy link
Member

@jsumners jsumners left a comment

Choose a reason for hiding this comment

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

👎

Docs should not link out to a third party repository.

@the-vishal-kumar
Copy link
Contributor Author

Docs should not link out to a third party repository.

I've updated the links in README.md to Fastify-Rate-Limit Repository

Copy link
Member

@Eomm Eomm left a comment

Choose a reason for hiding this comment

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

I think this example is really useful and I would like to suggest a little refactoring to apply the best practice we sponsor thought the plugin system!

}
)

function RateLimiterStore (options) {
Copy link
Member

Choose a reason for hiding this comment

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

I think that moving this class to a file would let the user understand better how to organize the source.

Moreover, this would avoid having a global sequelize object and it would be really more useful to users to adapt to their fastify instance

(I saw there is the same issue in the knex example and I think it should be improved as well - not an issue with this PR)

the-vishal-kumar and others added 2 commits December 16, 2020 14:36
Co-authored-by: Manuel Spigolon <behemoth89@gmail.com>
Copy link
Member

@jsumners jsumners left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

Lgtm

@mcollina mcollina merged commit c9cbb5b into fastify:master Dec 16, 2020
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.

4 participants