Skip to content

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

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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

thw0rted
Copy link

@thw0rted thw0rted commented Mar 7, 2022

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.

@anko
Copy link
Owner

anko commented Mar 7, 2022

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! 😄 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()
})

@thw0rted
Copy link
Author

thw0rted commented Mar 8, 2022

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.

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

Successfully merging this pull request may close these issues.

2 participants