mcollina / msgpack5

A msgpack v5 implementation for node.js, with extension points / msgpack.org[Node]
MIT License
492 stars 76 forks source link

integer keys / Map encoding #63

Closed ad34 closed 5 years ago

ad34 commented 6 years ago

In JSON and javascript , map keys have to be strings, but in MessagePack keys can be numbers.

Is there a way to force keys in a javascript object to be numbers when msgpack encoding is done?

javascript Map allows to use numbers as keys, but looks like objects encoded from a map cannot be de decoded :

let mapExemple = new Map() mapExemple.set(1,"map") mapExemple.set(3,"test")

let mpackedEx = msgpack.encode(mapExemple)

console.log(msgPackDecode(mpackedEx))

{}

mcollina commented 6 years ago

I’m a bit lost, and if you would like to send a PR it would be fantastic. Il giorno ven 16 feb 2018 alle 17:04 Ad34 notifications@github.com ha scritto:

In JSON and javascript , map keys have to be strings, but in MessagePack keys can be numbers.

Is there a way to force keys in a javascript object to be numbers when msgpack encoding is done?

javascript Map allows to use numbers as keys, but looks like objects encoded from a map cannot be de decoded :

let mapExemple = new Map() mapExemple.set(1,"map") mapExemple.set(3,"test")

let mpackedEx = msgpack.encode(mapExemple)

console.log(msgPackDecode(mpackedEx))

{}

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/mcollina/msgpack5/issues/63, or mute the thread https://github.com/notifications/unsubscribe-auth/AADL4-aJg3hwe-nxLvQsP2Zzrl56h-k1ks5tVacSgaJpZM4SIjHX .

ad34 commented 6 years ago

I will try to write a working encoder and let you know , right now I cannot use msgpack.encode on a javascript Map object

imnotjames commented 6 years ago

I wrote an ext encoder and decoder for this:

    packer.registerEncoder(
      (o) => o instanceof Map,
      (obj) => {
        let buffers = [];
        let length = 0;

        // Pack up all of the key / value pairs
        // and keep track of the ongoing length
        // that we've packed so far.
        for (let [ key, value ] of obj.entries()) {
          buffers.push(packer.encode(key, true));
          buffers.push(packer.encode(value));
          length++;
        }

        let header;

        // There are three types of maps
        //
        // * fixedmap (header byte 0x80 - 0x8F)
        // * map16 (header byte 0xde)
        // * map32 (header byte 0xdf)
        //
        // For further information on the map types,
        // see: https://github.com/msgpack/msgpack/blob/master/spec.md#map-format-family

        if (length < 16) {
          // fixedmap fits up to 15 items in it.
          // The header is only a single byte of
          // 0x80 + the size.
          header = Buffer.allocUnsafe(1)
          header.writeUInt8(0x80 | length);
        } else if (length < 0xFFFF) {
          // map16 fits up to 65535 items in it.
          // The header is a 0xde header byte
          // and a 16 bit unsigned (2 bytes)
          header = Buffer.allocUnsafe(3);
          header.writeUInt8(0xde);
          header.writeUInt16BE(length, 1);
        } else {
          // map32 fits up to 4294967295 items in it
          // The header is a 0xdf header byte
          // and a 32 bit unsigned (4 bytes)
          header = Buffer.allocUnsafe(5);
          header.writeUInt8(0xdf);
          header.writeUInt32BE(length, 1);
        }

        // Combine all of the buffers together (in the right order)
        // to create the output buffer we want.
        return Buffer.concat([ header, ...buffers ]);
      }
    );

However, native support for Map would be really great.

mcollina commented 6 years ago

This seems something we should provide. Would you like to send a PR to address this? Maybe a default encoder that we ship within this module.

imnotjames commented 6 years ago

I'd be happy to open a PR with this feature as a native encoder - it would just be a bit of finnagling to remember what standard ES is :smile:

If I was to do that, however, I have a few questions as it's your library:

mcollina commented 6 years ago

Can we register this as a separate encoded type?

imnotjames commented 6 years ago

We absolutely could - but it would bring it inconsistencies with the msgpack spec, hence my using an ext type where I did. If you'd like to pick out bytes for that I could implement it in that way.

Do you mean registering it as a default ext type? Would that bring BC breaks as well, if someone already used that ext type.

I went ahead and threw the code together (using newer node features, so definitely not ready for any sort of merge) along with some tests showing off the weirdness of Map vs {}. imnotjames/msgpack5@a41ec94e552d799e5627800d03d90b26ecda3a96

mcollina commented 6 years ago

I mean having an ext type that we provide the implementation for but it's not enabled.

imnotjames commented 6 years ago

There was another issue ( #10 ) which asked about having custom implementations of standard types.

Perhaps something like that? Though I suppose it'd need to be benchmarked..

However, an ext type as some separate contrib directory which can be brought in would also be useful.

thephoenixofthevoid commented 5 years ago

By the way:

const map = new Map()
map.set(1, "hello")
map.set("1", "world")
console.log(map.get(1), map.get("1")) 
// will print: hello world

But:

const obj = {}
obj[1] = "hello";
obj["1"] = "world";
console.log(obj[1], obj["1"]) 
// will print: world world

So it's a bad idea, mixing ES6 Maps and object-based hashes.