Skip to content

Conversation

@darkgl0w
Copy link
Member

@darkgl0w darkgl0w commented May 1, 2019

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

  • run npm run test
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message and code follows Code of conduct
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

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.

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
@darkgl0w
Copy link
Member Author

darkgl0w commented May 2, 2019

Hi @Eomm ^^
I implemented one more check to prevent the behavior you described (as it may be considered a bad practice :d) and I made the plugin throw an error. I also updated the documentation to point out the fact that this new feature implies to namespace all the registered instances of fastify-redis.

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. ^^

Eomm and others added 2 commits May 2, 2019 15:29
Co-Authored-By: darkgl0w <31093081+darkgl0w@users.noreply.github.com>
Copy link
Member

@delvedor delvedor left a comment

Choose a reason for hiding this comment

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

LGTM

@darkgl0w
Copy link
Member Author

darkgl0w commented May 2, 2019

Applied all the necessary changes and corrected the faulty test.

@Eomm Eomm merged commit edfb960 into fastify:master May 2, 2019
@darkgl0w darkgl0w deleted the register-multiple-instances branch May 2, 2019 14:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

4 participants