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

feat: reconnect and cancelAll consumers #179

Merged
merged 3 commits into from
Aug 27, 2021

Conversation

luddd3
Copy link
Collaborator

@luddd3 luddd3 commented Aug 27, 2021

Provided that they consumed with the channel wrapper function
consume

luddd3 added 2 commits August 27, 2021 10:38
Provided that they consumed with the channel wrapper function
`consume`
Copy link
Owner

@jwalton jwalton left a comment

Choose a reason for hiding this comment

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

You practically read my mind. I was thinking about doing something almost identical to this. I was also thinking about making it so calls to channelWrapper.assertQueue() would add a setup function that asserts the queue (although that's a little trickier, as it won't work if you're trying to create an { exlusive } queue and get the name of the queue back...)

src/ChannelWrapper.ts Show resolved Hide resolved
consumerTag: string | null;
queue: string;
onMessage: (msg: amqplib.ConsumeMessage) => void;
consumeOptions?: amqplib.Options.Consume;
Copy link
Owner

Choose a reason for hiding this comment

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

What do you think of merging these two sets of options objects into one?

Suggested change
consumeOptions?: amqplib.Options.Consume;
consumeOptions?: amqplib.Options.Consume & { prefetch?: number };

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It might read better. I was a little bit torn about this because I couldn't decide if it was better to keep the ordinary consume options separated from the node-amqp-connection-manager specific options or not.

I can rewrite it so we get a feel for your alternative.

Copy link
Owner

Choose a reason for hiding this comment

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

I know what you mean, because this is definitely "cleaner" from how we call into amqplib, but the API is a little weird with two sets of options...

If you do something like:

const { prefetch, ...amqpLibOptions } = options;

you can cleanly extract our "custom" options, and anything extra will get passed down to amqplib, so if amqplib adds extra options we don't have to explicitly add them.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ah, you read my mind =)

@luddd3
Copy link
Collaborator Author

luddd3 commented Aug 27, 2021

You practically read my mind. I was thinking about doing something almost identical to this. I was also thinking about making it so calls to channelWrapper.assertQueue() would add a setup function that asserts the queue (although that's a little trickier, as it won't work if you're trying to create an { exlusive } queue and get the name of the queue back...)

Sounds like a good idea!

@jwalton jwalton merged commit 8655a33 into jwalton:master Aug 27, 2021
@luddd3 luddd3 deleted the feat/reconnect-consumers branch August 27, 2021 14:55
@jwalton
Copy link
Owner

jwalton commented Aug 27, 2021

🎉 This PR is included in version 3.6.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@rhsobr
Copy link
Contributor

rhsobr commented Sep 7, 2021

Thanks for this!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants