Would be nice to have OrderedMap and OrderedSet be indexed

[tolmasky]

Order matters for these two collection types, and the collection clearly knows the indexing internally in the _list member (I know, implementation detail, but it would have to know somehow either way), so it is very unfortunate that you can't really treat interface with the indexes in many meaningful ways. To list two instances that I frequently run into this problem:

1. Would of course be nice to just be able to do orderedMap.at(index). This is my main gripe right now, the fact that I need to store out an almost identical separate collection that's just a List to be able to go to index -> item.

2. Would be nice to have index show up in things like map and reduce (filter, etc.). Given that the API for .map is already (item, key, collection), perhaps we could do (item, key, collection, index) to not have any backwards-incompatible changes.

[tolmasky]

I'll add that it's kind of bizarre that OrderedMap and OrderedSet kind of half-heartedly incorporate some aspects of Indexed Collections, namely first and last. These are implemented somewhat strangely internally (this.find(returnTrue) for the former), showing that this was kind of an afterthought, but implementation aside, it's just weird that they expose an easy way to get at their first and last elements, but nothing else (without jumping through hoops that the average person may not understand the performance implications of, like .valueSeq() or something).

[tolmasky]

Although this is outside the scope of what we could trivially do just by adding functions on top of the current internal implementation, a slightly modified implementation would could also in theory trivially make OrderedMap.indexOf be O(1) (or O(lg64 N) or whatever it is immutable maps currently are).

[jdeniau]

Hi,

Conceptually, having an ordered Map or Set does not imply that you can access it by index directly. It just means that the order will be kept.

You pointed out some incoherence with the .first and .last method. Those methods are also present on unordered Map and Set. This comes from a difference with the .get method that is implemented of both objects : for iterable, first, get and has works by index.

For Map, .get works with the key, and for Set, .get works with the value.

Implementation

About the implementation of .at, I would tend to implement it for OrderedSet, OrderedMap and Set as first and last are implemented.
I think that we should not implement it on List nor Seq as the .get method works in that case, but it's arguable.

It feels weird for Map, I think that we should even deprecate first and last on this.

Do other @immutable-js/maintainers have a opinion on this ?

Feel free to open a PR on this !

index in map function

Why not

performance

If this is doable later, you can open a simple PR to "make it work", and a second one that "makes it fast"

Quickfix

You mentioned a quickfix for your issue, but to be clear if someone wants to do that now, you can do this:

import { OrderedMap, OrderedSet } from 'immutable';

const os = OrderedSet(['b', 'a', 'c', 'b']);
const secondValue = os.valueSeq().get(2);

const om = OrderedMap({ foo: 'FOO', bar: 'BAR' });
const mapValue = om.valueSeq().get(1);

[tolmasky]

Conceptually, having an ordered Map or Set does not imply that you can access it by index directly. It just means that the order will be kept.

Sure, the same is true of Lists of course, see linked lists like lisp lists for an example of that. But I guess my point is that, given that we can, it would be nice to have it mean that in this library, the same way that in this library, List does imply indexability due to it inheriting from Collection.Indexed.

performance

If this is doable later, you can open a simple PR to "make it work", and a second one that "makes it fast"

Quickfix

You mentioned a quickfix for your issue, but to be clear if someone wants to do that now, you can do this:

I actually mentioned valueSeq() as a solution in my follow-up comment, and that is in fact what I was originally doing as a workaround. However, I discovered that this meant that every index access was creating new objects, since unfortunately myMap.valueSeq() !== myMap.valueSeq() as valueSeq is not a cached property. This is one of the things I meant in my comment about the performance implications of valueSeq() being opaque (the other being that in a true "we promise nothing about the indexability of OrderedMap", you may be signing up for ridiculously bad performance, the same as you might get from shoving any non-List-type into a List).

So, to some degree the point of this request is somewhat about performance, in the sense that a public API would imply (or perhaps explicitly state in the documentation), that this is an expected use case of this collection and thus the performance will match your intuition (matching roughly with with List's .get performance), both algorithmically and overhead-wise. Otherwise, my current strategy of caching out valueSeq() (or .toList()) would be better than accessing a .at that generates a bunch of objects every time it's called, if that makes sense. So my interest would lean towards having the internal implementation:

https://runkit.com/tolmasky/indexable-orderedmap-example

I'm certainly happy to file a PR, was just wondering if there was any actual pushback on being able to do this at all. I suppose one of the things I was also hoping could be discussed is whether there was any opinion from an API perspective. Is at() a good name, or perhaps item() to match NodeList in W3C? Do we want something like at and keyAt, since you could potentially be interested in both (to match entrySeq and keySeq)? But if the best way forward is to just present a proposal PR, I'm certainly happy to do that of course.