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

Add AMQP Adapter #12

Merged
merged 3 commits into from
Apr 21, 2016
Merged

Add AMQP Adapter #12

merged 3 commits into from
Apr 21, 2016

Conversation

tinque
Copy link
Contributor

@tinque tinque commented Apr 19, 2016

I added a AMQP Adpater. (tested with RabbitMQ)

@tinque tinque closed this Apr 19, 2016
@tinque
Copy link
Contributor Author

tinque commented Apr 19, 2016

Problem with params uri (not enough test) sorry

@tinque tinque reopened this Apr 19, 2016
@ekryski
Copy link
Member

ekryski commented Apr 19, 2016

Oh hell yeah! Stoked to check this out!

@daffl
Copy link
Member

daffl commented Apr 19, 2016

This is great! Do you have an idea how we could test this on Travis?

@tinque
Copy link
Contributor Author

tinque commented Apr 19, 2016

To be honest ! I have no idea ...

@ekryski
Copy link
Member

ekryski commented Apr 19, 2016

I think I do. I've done a bunch with RabbitMQ.

msg.ev = ev;
msg.data = data;
//amqp name
if(ch){
Copy link
Member

Choose a reason for hiding this comment

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

I'd like to see these variables longer. I know that ch means channel but people new to RabbitMQ may not.

@ekryski
Copy link
Member

ekryski commented Apr 19, 2016

@tinque this is solid! Thanks! I made a couple minor comments but I am also fine making those changes and looking into getting tests running on travis. This is good to 🚢 ! 🎉 🍻

ch.consume(q.queue, function(msg) {
var temp = JSON.parse(msg.content.toString());
onmessage(temp.ev,temp.data);
}, {noAck: true});
Copy link
Member

Choose a reason for hiding this comment

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

We may later on, want to turn on "acks" but for now this is totally fine.

@ekryski
Copy link
Member

ekryski commented Apr 21, 2016

I'll figure out what's going on with the tests. I'll probably add some more and fix them up over the next couple days.

@ekryski ekryski merged commit ebda566 into feathersjs-ecosystem:master Apr 21, 2016
@ekryski ekryski mentioned this pull request Apr 21, 2016
@petermikitsh
Copy link

@ekryski @daffl Can we get a release of feathers-sync that includes this functionality? I'm stoked to get my feathers app to fly. Thanks!

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