senecajs / seneca-amqp-transport

Official AMQP transport plugin for Seneca
MIT License
68 stars 25 forks source link

Support more than 2 arguments for .add / .act callbacks #78

Open bra1n opened 7 years ago

bra1n commented 7 years ago

Example:

// listener
require('seneca')().use('seneca-amqp-transport')
.add({a:1, b:2}, function (msg, reply) {
    reply(null, {z: msg.z}, 123) // <--- 3 arguments here
}).listen({type: 'amqp', pin: 'a:1,b:2', url: 'amqp://guest:guest@localhost:5672/'});

// client
require('seneca')().use('seneca-amqp-transport')
.client({type: 'amqp', pin: 'a:1,b:2', url: 'amqp://guest:guest@localhost:5672/'})
.act('a:1,b:2,z:9', console.log); // should log all arguments

Expected output:

null { z: 9 } 123

Actual output:

null { z: 9 } { id: 'wmkrp7ebp8yd/tjrznrcu10hm',
  accept: 'tkxxb46prhmh/1487592318189/4167/3.3.0/-',
  track: [ '2j0b5xk44e4z/1487592318564/4167/3.3.0/-' ],
  time:
   { client_sent: 1487592318854,
     listen_recv: 1487592318885,
     listen_sent: 1487592318893,
     client_recv: 1487592318896 } }

Seneca supports more than 2 arguments in act callbacks since #340.

nfantone commented 7 years ago

@bra1n Hi, there.

Thanks for letting me know of this. Wasn't aware of the feature. It should definitely be supported by the plugin.

I'll start working on it.

bra1n commented 7 years ago

Thanks for the quick response, looking forward to it! :-)

nfantone commented 7 years ago

@bra1n On a first, shallow look into this, it seems like support for this is missing in seneca-transport. Still unsure, though.

Will keep on digging.

nfantone commented 7 years ago

Ok, I can confirm this, @bra1n. seneca-transport -which is what all transports are/should be based on- does not support multiple arguments.

Take a look at this line. That's the actual .act call that ends up invoking your remote action. See that it completely ignores arguments other than the very first in the callback (called out here).

bra1n commented 7 years ago

Huh, interesting. I opened an issue there as well. Thanks for the reasearch, @nfantone 👍

nfantone commented 7 years ago

@bra1n Seems like we both did.

https://github.com/senecajs/seneca-transport/issues/152

bra1n commented 7 years ago

Haha, I'll close mine then.

nfantone commented 7 years ago

@bra1n No problem at all. Thanks.

Let's leave this open to track progress and alert other people, since it's affecting the transport.

Good spot, BTW.