-
-
Notifications
You must be signed in to change notification settings - Fork 37
Allow multiple instances registering #29
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
Conversation
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.
LGTM
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.
Nice feature!
I would add only one more test as solidity check since some user could have already registered multiple redis in different scopes:
fastify.register(function (instance, options, next) {
// register one redis without namespace
next()
})
fastify.register(function (instance, options, next) {
// register two redis with namespace
next()
})
As suggested by @Eomm - Test included - Documentation updated
Hi @Eomm ^^ Also it's to be noted that using this feature won't be a breaking change (at least not really a breaking change) but it requires a little bit of refactoring on existing projects if someone wants to use it. ^^ |
Co-Authored-By: darkgl0w <31093081+darkgl0w@users.noreply.github.com>
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.
LGTM
Applied all the necessary changes and corrected the faulty test. |
Address and close #28 (comment)
No breaking changes were introduced with this PR and the external client behavior has been extended to the named instances.
Checklist
npm run test