-
-
Notifications
You must be signed in to change notification settings - Fork 244
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
feat(rabbitmq): add support for multiple connections #411
feat(rabbitmq): add support for multiple connections #411
Conversation
@underfisk the most recent commit should have cleaned this up a decent amount. When you have a few minutes to review, let me know what you think of this approach and whether you have any changes/suggestions! |
2c9850b
to
fdd10a4
Compare
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 to me overall, thanks for submitting this, I haven't yet ran the tests but we'll need them for sure to assert that everything still workings fine
Let's also wait for @WonderPanda review on this PR
Would really like this to get merged =) |
Great work, I like the approach taken here. However it seems like the integration tests are failing in CI for some reason |
Will take a look at those integration tests today! |
@WonderPanda 34cca4b should fix the failing integration tests, looks like there were 2 tests using the same exchange and queue names. |
Great work! Updated today, but got a error on my project. Is someone experiencing the same issue ? |
@verti4cal Can you tell us which version you were in before and you are now and also what versions are your nestjs dependencies, I'd like to know when it stopped working. Also if you can give a small repro, that would be good but we should avoid adding more here so please open a new github issue and we track the problem there |
Hey people. I hope you're doing well. Could anyone explain how could I specify the connection I need to connect to for publishing and the connection I want to use to subscribe in the controllers with the option enableControllerDiscovery as true? Thank you! |
Fixes #393
WIP and still mostly untested, but wanted to put up a PR in case anyone has suggestions on how this can be improved. At a high-level, this adds a
name
property for each connection, and allows us to reference the specific connection in decorators. For example:This also adds a new
AmqpConnectionManager
class and provider to access the array of connections. This is implemented similarly tonestjs-redis
's method for accessing multiple connections:this.amqpConnectionManager.getConnection('connection-name')
.I'm also striving to keep this backwards-compatible, so as not to require any major bumps. All the existing unit and integration tests pass, so I think I've succeeded there, but I'll need to do some additional testing to make sure.