Remove buffer-more-ints requirement

[amqp-node]

Remove buffer-more-ints requirement

[squaremo]

In this case, the added procedures don't exist elsewhere, and if they did exist, would do exactly the same thing (one should hope). But I take your point, monkey patching does often lead to trouble.

[skeggse]

Definitely.

I would recommend a path similar to node-mysql. This has the added benefit that you don't lose precision for full 64-bit integers and gives developers the output they need. If they really want to deal with half-integer arithmetic, they can convert from bignumber to native numbers.

[skeggse]

Also related, it looks like you're depending on util._extend in connect.js. I absolutely agree that Node's util library should provide an extend function, but given that it's a private function you might want to avoid that for now. It unnecessarily restricts the version of Node on which amqp.node can run and is essentially using a function that may disappear.

[squaremo]

Also related, it looks like you're depending on util._extend in connect.js.

I had a look around, and indeed it does appear that require('util') as a whole is considered off-limits to third-party modules (though perhaps inherit is forgivable).

I've replaced the use of _extend with an artisanal clone function.

[skeggse]

I had a look around, and indeed it does appear that require('util') as a whole is considered off-limits to third-party modules (though perhaps inherit is forgivable).

That's probably because the util module does not provide much beyond inherits that would be useful for developers, apart from _extend. The module itself is safe for any developer to use, provided they stick to the public API.

[squaremo]

In master the dependency has been updated to buffer-more-ints v0.0.2, which doesn't monkey-patch Buffer (unless you tell it to, which I don't).

Still thinking about bignumber.js support. Not sure I like optionality of that kind (and it's not really optional, since the internals also use 64-bit integers in at least one place, message content size).

[skeggse]

In master the dependency has been updated to buffer-more-ints v0.0.2, which doesn't monkey-patch Buffer (unless you tell it to, which I don't).

Perfect. That means this issue is technically closed.

Still thinking about bignumber.js support. Not sure I like optionality of that kind (and it's not really optional, since the internals also use 64-bit integers in at least one place, message content size).

If message content size is the only place, it seems like bignumber.js support could still be optional, as long as the user is correctly informed that without said support the maximum message content size will be slightly limited. I cannot imagine any real-world message being that large--a message queue isn't a data storage device--but if it's useful, the user could enable it.

Not sure I like optionality of that kind.

That's fine.

You could return strings representing really large integers--have that a configurable option or something. Just a thought.

[squaremo]

If message content size is the only place, it seems like bignumber.js support could still be optional, as long as the user is correctly informed that without said support the maximum message content size will be slightly limited. I cannot imagine any real-world message being that large--a message queue isn't a data storage device--but if it's useful, the user could enable it.

And presently, you can only send a buffer, which can't be that big anyway.

Bignumbers would be useful for timestamps and "general purpose" 64-bit integers; that is, values that aren't used in the protocol but may be encoded in user-supplied data. It seems sensible to support them -- it's switching support on and off that I'm not keen on.