search

amqp-node / amqplib

AMQP 0-9-1 library and client for Node.JS
https://amqp-node.github.io/amqplib/
Other
3.7k stars 476 forks source link

Remove buffer-more-ints requirement #24

Closed skeggse closed 11 years ago

skeggse commented 11 years ago

Like MySQL, you need to support 64-bit integers. Node, of course, does not play nice with such numbers, content to only play nice with doubles.

The popular node-mysql library handles this by delegating to the bignumber library if the user specifies the supportBigNumbers config option.

amqp.node handles 64-bit integers by altering the global Buffer object and prototype with the buffer-more-ints module. This may seem like a harmless change, but in doing so you alter the Buffer objects of everyone that uses amqp.node and all modules they may depend on. I shouldn't need to explain why this could become problematic, but take bnoordhuis/node-buffertools#39 as an example--you override a common mechanism and the original mechanism is no longer available. If it changes, your users may wish to use the updated version, but you cannot change your code because that may break some other a user's code that doesn't want or use the updated feature.

squaremo commented 11 years ago

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 commented 11 years ago

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 commented 11 years ago

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 commented 11 years ago

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 commented 11 years ago

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 commented 11 years ago

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 commented 11 years ago

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 commented 11 years ago

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.