msgpack.js icon indicating copy to clipboard operation
msgpack.js copied to clipboard

Doesn't handle uint64 particularly well

Open pfeatherstone opened this issue 5 years ago • 7 comments

This library unfortunately doesn't handle 64 bit integers particularly well. It could be because it's not using BigInt where appropriate

pfeatherstone avatar May 23 '20 12:05 pfeatherstone

Can you suggest an "appropriate" use?

ygoe avatar May 25 '20 15:05 ygoe

Maybe the readint and readuint functions could be a bit like readfloat. Use dataview and use the getUintx/getintx Methods. These include bigint versions for 64 bit integers. I would do a similar thing for the write methods but that would require loads more refactoring since your using appendbytes structure.

pfeatherstone avatar May 25 '20 19:05 pfeatherstone

Basically I would use dataview and its methods as much as possible and keep the dynamic array resizing you’ve done.

pfeatherstone avatar May 25 '20 19:05 pfeatherstone

I could submit a PR next week but it all depends on how forgiving you are on my more or less detailed commit messages...

pfeatherstone avatar May 25 '20 19:05 pfeatherstone

Yes, a PR would be welcome. I've never used BigInt nor had the need to. A question would be: could the decoding return either type (Number or BigInt) depending on the actual value found in the MessagePack data? Or should the returned type be more predictable? As MessagePack doesn't care about such language limitations of JavaScript, it can't give any hints here. IIRC the encoder is always free to choose the smallest integer type possible for each value (and in fact should do to minimise the data size).

As for encoding, the situation is easier. We'd probably need a type check and convert a BigInt differently than a Number, without (implicitly) converting it to a Number first.

I care more about code quality than commit messages. ;-) Messages are only relevant for more complex changes. And we still have this issue for reference.

ygoe avatar May 29 '20 18:05 ygoe

i'm sorry i haven't had time to work on the PR. My main problem was handling UNIX timestamps in nanoseconds which exceeds the maximum number supported by javascript: 2^53. For now, the quick fix is working in microseconds which is below the 2^53 mark. so i'm gonna have to put the BIGINT idea on hold.

pfeatherstone avatar Jun 08 '20 15:06 pfeatherstone

No need to be sorry. I'll keep this issue open until either one of us (or somebody else) finds the time to work on it. BigInt looks like something worth supporting, especially for compatibility with other languages. I just didn't have the need for it yet.

ygoe avatar Jun 14 '20 14:06 ygoe