Skip to content
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

findKey/findIndex is inconsistent with map/list agnosticism of most other methods #740

Closed
jedwards1211 opened this issue Jan 5, 2016 · 5 comments

Comments

@jedwards1211
Copy link
Contributor

One of my favorite things about immutable.js is that most methods like map, filter, even concat, flatten, reduce, join, etc. are present for both KeyedIterable and IndexedIterable, and the object/array distinctions are almost erased. Why then does KeyedIterable have findKey, and IndexedIterable have findIndex? Everyone knows that a JS array index is actually just a hash key, and I find myself writing a findKey wrapper method that works on any immutable Iterable.

It's fine to keep findIndex I suppose, but IndexedIterable should also have findKey.

Would you accept a PR for this?

For that matter, it seems to me that well-defined semantics for sequential index specific methods like indexOf, zip, etc. could have well-defined semantics over the numeric keys of KeyedIterables as well and the two could be fully merged once and for all, except perhaps with some ontologic knowledge of whether toJS should create an array or object. The schizophrenia of JS objects and arrays just causes awkwardness, in my opinion; I think they should either be totally interchangeable or totally uninterchangable.

@jedwards1211
Copy link
Contributor Author

I guess I could just use toMap().findKey(...) everywhere. Would be nice not to have to create a new object though.

@leebyron
Copy link
Collaborator

leebyron commented Jan 5, 2016

I would be happy to consider a PR for including findKey to Indexed collections.

I disagree that the other indexed methods like indexOf make sense on Keyed collections though. It's common that a Keyed collection is not keyed by an integer and then these methods make less sense. Even if a Keyed collection is keyed by an integer, it's not necessarily the case that the integer keys will be iterated over in incrementing order as they are in indexed collections, which at the very least would require different implementations, and at worst could cause unintuitive results.

@jedwards1211
Copy link
Contributor Author

Hmmm. I'm just thinking about the behavior of JS arrays and objects. For instance, given that "numeric" object keys come first in a for in loop, even though typeof reports they are strings, indicates to me that they might be stored as integer keys internally. So I was thinking that it would be consistent for a JS object to have an indexOf that searched only through values of "numeric" keys. The same could work for Immutable.js, but does Immutable.js support a mix of numeric and non-numeric keys in KeyedCollection and IndexedCollection?

@leebyron
Copy link
Collaborator

I think you're right. Thanks for the discussion and sorry for the delay. Check out the commit for details.

@jedwards1211
Copy link
Contributor Author

Oh wow, no, thank you! And I think you're right about indexOf et al, the semantics would be a bit too subtle for keyed collections.

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

No branches or pull requests

2 participants