array-keyed-map icon indicating copy to clipboard operation
array-keyed-map copied to clipboard

Handle non-array keys as if they were single-element array

Open thw0rted opened this issue 2 years ago • 2 comments

The current documented behavior expects Arrays for the path argument to all relevant methods, but actually uses a for-of loop so any iterable will appear to work. However, this means that map.set("key", "value") is actually treated as map.set(["k", "e", "y"], "value").

This PR treats all non-array path arguments as a single-item array, allowing get("key") as shorthand for get(["key"]). I think I have unit tests for everything and standard passes.

I also added very rudimentary TypeScript typings, but as a separate commit so it's easy to remove if you would prefer.

thw0rted avatar Mar 07 '22 17:03 thw0rted

I don't want to add run-time type checks.

In a dynamically typed language, I think the only sane way to go about writing a library is to assume the user knows what they're doing, because there are infinite ways to pass invalid input to every function, and therefore potentially infinite error-handling code to eat developer and processor time.

Although unintended, I think keys being allowed to be any iterable is a pretty neat feature! :smile: I think I'd rather resolve this by changing the docs to make it official.

I worry that checking strictly for Array.isArray would make this library not work with Proxy-based objects that emulate arrays, objects that implement @@iterator, Uint32Array and friends, or whatever next iterable array-ish thingy the fine folks of TC39 decide to add to the language in the future. This test fails on your branch, for example:

test('set/get iterator key', (t) => {

  const weirdObject = new (class Iteratorish {
    * [Symbol.iterator] () { yield * ['a', 'b', 'c'] }
  })()

  const p = new AKM()
  p.set(weirdObject, true)
  t.same(p.get(['a', 'b', 'c']), true)
  t.end()
})

anko avatar Mar 07 '22 19:03 anko

I think I understand, but I should emphasize that this feature was intended more to improve ergonomics than as a means of "error handling", per se. I only started using your library yesterday, so far be it from me to assume how people intend to use it as a broad generalization. I would imagine, though, that the most common use case would be to pass a (quite possibly literal) array. Some might compute that array dynamically, building a key at runtime.

For myself, I'm trying to support a map that mirrors the structure of a JSON-compatible object -- path keys descend through the structure of the object. The problem is that the objects are frequently flat, and the previous implementation used a string-keyed map with some workarounds to handle deeper paths, translating keys between array and string. (As you say in the readme, that way madness lies...) It would save me a lot of refactoring if I could continue to get / set with a string to correspond to top level properties, and it occurred to me that this might be more ergonomic for what I imagined might be a common use case here as well.

I see a couple of options to proceed. I could just use my own fork indefinitely, or I could change all my calls to your library to conditionally wrap keys (maybe with a subclass?). If you'd consider it, the behavior I'm looking for could also be officially implemented behind a flag, or with a two-argument method call (either set(weirdIterable, true) or set(justAString, true)), but I admit that feels a bit clunky.

Oh! Also, for my own reference, are you interested in distributing Typescript typings? I can submit a separate PR with just the basic ones in this branch if you'd like.

thw0rted avatar Mar 08 '22 10:03 thw0rted