darrachequesne / notepack

A fast Node.js implementation of the latest MessagePack spec
MIT License
75 stars 19 forks source link

Transparently encode native BigInt as strings #24

Closed zdm closed 3 years ago

zdm commented 3 years ago

Hi, Is it possible to add support to encode BigInt as strings trransparently?

Something like this:

case 'bigint': 
        return _encode(bytes, defers, value.toString());

If this is acceptable, I can create pull request.

darrachequesne commented 3 years ago

Hi!

That's a good question actually. I'm wondering whether we should:

What is your opinion in this?

That being said, you should already be able to use the same trick as with JSON.stringify() (here):

BigInt.prototype.toJSON = function() { return this.toString()  }
zdm commented 3 years ago

BigInt.asUintN has limitations to the number of bits - in some cases we can lost data.

I think better is to encode / decode using custom extension type. Send it as string, and decode to BigInt.

zdm commented 3 years ago

so, what do you think?

darrachequesne commented 3 years ago

Yes, I think the best way is to add support for custom extensions (also requested in https://github.com/darrachequesne/notepack/issues/18).

I won't have much time for that in the short term though, would you have time to implement it? I'll be happy to merge it :+1:

zdm commented 3 years ago

What extension type in hex can be taken for this? Are you already use any numbers from this range [0, 127] (application-specific types)?

zdm commented 3 years ago

I will take 0x01 for BigInt.

PR ready.