search

noodlefrenzy / node-amqp10

amqp10 is a promise-based, AMQP 1.0 compliant node.js client
MIT License
134 stars 56 forks source link

AMQPError and friends are not exposed through the public API #192

Open damonbarry opened 8 years ago

damonbarry commented 8 years ago

There are many places in the amqp10 code where AMQPError objects are returned to the user (e.g., when the user sets up a listener for 'errorReceived' on the client, or on a sender/receiver link). But AMQPError doesn't appear to be exposed through the public API, i.e., I can't reach the object definition through "require('amqp10')". I'd like to be able to access the definition of this class because I'm writing a library that wraps amqp10, and I need to translate AMQPErrors into my library-specific errors. In my tests, I want to be able to create simple AMQPError objects and pass them to my translation function. It would also be useful to access the definition of AMQPSymbol

There is an object at require('amqp10').Errors, but that's different. Perhaps the plan is that all AMQPErrors will eventually be abstracted into transport-agnostic errors under require('amqp10').Errors? If so that's fine. Whatever the official plan is, I just want to be able to map amqp10 errors to my library-specific errors.

noodlefrenzy commented 8 years ago

This makes sense - I'll try and get a new release out today with those enhancements.

mbroadst commented 8 years ago

@noodlefrenzy seeing that the AMQPErrors are the actual descriptors, does it make sense to make a wrapper in lib/errors.js for an AMQPError object (to keep exported error consistency)? Not really sure what the best approach is here

noodlefrenzy commented 8 years ago

@mbroadst yes, that's exactly what I was planning :)

noodlefrenzy commented 8 years ago

@damonbarry I have a PR out for the change, all the code is in https://github.com/noodlefrenzy/node-amqp10/tree/expose-amqp-error-symbol if you want to try it before you buy.

noodlefrenzy commented 8 years ago

Hey there, published. @2.1.3 resolves this.

NOTE: There is a change in the returned error in that I now unpack the condition symbol (so you no longer need condition.contents to get the string). Since this was not part of the public API I didn't feel the need for a 2.2 bump, sorry if I'm a bad person for not doing so.

Obviously let me know (and reopen this) if you run into any issues.

noodlefrenzy commented 8 years ago

I misunderstood the need here - he'd like an isolated constructor to avoid having to take dependencies on internal classes while mocking for tests, so will modify to provide that.