Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Doesn't handle uint64 particularly well #18

Open
pfeatherstone opened this issue May 23, 2020 · 7 comments
Open

Doesn't handle uint64 particularly well #18

pfeatherstone opened this issue May 23, 2020 · 7 comments

Comments

@pfeatherstone
Copy link

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

@ygoe
Copy link
Owner

ygoe commented May 25, 2020

Can you suggest an "appropriate" use?

@pfeatherstone
Copy link
Author

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
Copy link
Author

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

@pfeatherstone
Copy link
Author

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

@ygoe
Copy link
Owner

ygoe commented May 29, 2020

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.

@pfeatherstone
Copy link
Author

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.

@ygoe
Copy link
Owner

ygoe commented Jun 14, 2020

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants