Add JS Map support for encoding & decoding

[jasonpaulos]

Adds support for encoding and decoding Map objects, as an alternative to plain javascript objects.

The primary advantage is that this adds much better non-string type support for map keys.

[robdmoore]

This is a breaking change, should we keep the existing logic looking for number too?

[jasonpaulos]

It's pretty strange to me that the upstream library was getting away with just coercing number keys to strings on objects, so I wanted to do away with that behavior now that there's a better alternative.

Although, since it is a breaking change, I think I'd be willing to add another decoding option that could control that behavior if it's really what you wanted, since this has been an established practice in the upstream library for a while.

[robdmoore]

If I read this right, does it mean if you are using maps and you set useRawBinaryStrings then it will use a uint8array for the map keys? I wonder if we should allow that to be switchable?

[jasonpaulos]

That's correct, and your suggestion makes sense

[jasonpaulos]

Added two options for this in 7342b02

[robdmoore]

The way I see it, if you have number keys it *works* with in-built objects so fair to support…On 5 Mar 2024, at 11:22 pm, Jason Paulos ***@***.***> wrote: @jasonpaulos commented on this pull request. In src/Decoder.ts:

-const isValidMapKeyType = (key: unknown): key is MapKeyType => { - return typeof key === "string" || typeof key === "number"; -}; +function isValidMapKeyType(key: unknown, useMap: boolean): key is MapKeyType { + if (useMap) { + return typeof key === "string" || typeof key === "number" || typeof key === "bigint" || key instanceof Uint8Array; + } + // Plain objects support a more limited set of key types + return typeof key === "string"; It's pretty strange to me that the upstream library was getting away with just coercing number keys to strings on objects, so I wanted to do away with that behavior now that there's a better alternative. Although, since it is a breaking change, I think I'd be willing to add another decoding option that could control that behavior if it's really what you wanted, since this has been an established practice in the upstream library for a while. —Reply to this email directly, view it on GitHub, or unsubscribe.You are receiving this because you commented.Message ID: ***@***.***>

[jasonpaulos]

Here's why I don't like decoding number keys on plain objects:

• No existing test coverage for it
• Potential for collision with string properties
  • Admittedly this is not a real concern for us, since in Algorand I don't believe we have any map objects that use both numbers and strings as keys. However, I find this behavior by default to be a big oversight for an encoding library
• Breaks for bigints
  • Any numbers decoded as bigints can't be keys. For an encoding scheme like this that supports 64-bit numbers, that means failures can occur just based on the value of those numbers. Depending on which int decoding mode you use, this could mean no numeric keys are supported, or only small numeric keys are supported.
• Inconsistent round trip results
  • Since numeric object keys get coerced into strings, there's no way of re-encoding your object identically. This is a huge pitfall.

Since it does happen to work with plain objects, it was a good way to offer partial support for number map keys. But now that this PR introduces full support, I don't think the advantages for enabling it by default outweigh the cons I listed above.

For backwards compatibility, I did add the supportObjectNumberKeys option in 7342b02. I think this might be a good compromise, since you can easily enable the old behavior

[jannotti]

I haven't spent much time in the tests yet.

[jannotti]

I didn't see an floating point tests, those could be useful in the abstract, but of course Algorand doesn't use them.