[DRAFT] Feature: Add findIndex method to SortedSet.

[bglgwyng]

This pull request adds the findIndex method to SortedSet, complementing the existing getAtIndex method. However, it is not yet complete; I plan to include tests and extend the same method to SortedMap.

Before proceeding, I have a few questions:

• Is the addition of findIndex necessary, and is this pull request acceptable?
• Could you provide clarification on the purpose of isComparable in the comp? I introduced an error-throwing mechanism when the key is not comparable just to pass type check, as seen here. But I'm unsure why the argument type should be like that so that this step becomes necessary.

Thank you for your guidance.

All Submissions:

• Have you followed the guidelines in our Contributing document?
• Have you checked to ensure there aren't other open Pull Requests for the same update/change?

New Feature Submissions:

1. Does your submission pass tests?
2. Have you lint and formatted your code locally prior to submission?

Changes to Core Features:

• Have you added an explanation of what your changes do and why you'd like us to include them?
• Have you written new tests for your core changes, as applicable?
• Have you successfully ran tests with your changes locally?

[vitoke]

Hi, thanks for working on this feature! As far as I'm concerned, if you see a use case for findIndex in your code, I'm happy with adding it.

As to your question about isComparable, the method is intended for a comparator to indicate whether it can compare the type of object that is passed. For instance, if the comparator can only compare strings, giving a number to isComparable should return false.

So if a SortedSet uses a comparator specifically for strings, then findIndex can directly return a "not present" value when it receives an object that is not a string. In general, I would prefer findIndex to return number | undefined so that if the element is not present, it would return undefined instead of an exception. I know this is a matter of style, but this keeps the interface more consistent with, for example Stream.indexOf .

Let me know if you have further considerations.

[bglgwyng]

Thank you for your kind reply!

I'll explain my usage of findIndex first. I'm using SortedSet for the data to be rendered in a large list with infinite scroll. I'd like to make it possible to move the scroll to the element by its ID, which is used as the key of SortedSet. So I need a way to get its index from its ID.

Now I understand the purpose of isComparable. However, this design feels a bit unnatural to me since rimbu supports TypeScript very well, so that, the obvious mistake like passing an invalid key is easily avoidable. I suppose that isComparable can hide potential bugs.

findIndex returns -(the index where the element should have if it exists + 1) for now. This behavior is the same as the internal findIndex and a famous binary search library.
Also, I've been already utilizing this behavior in my use case. When the focused item(imagine the one matches activeId) in the list gets deleted, the next item takes over the focus from it. If findIndex returns undefined, then I need an additional method with this behavior.

[vitoke]

My apologies, I did not fully explain the reason for isComparable. Even though indeed the TypeScript type system is fully capable of enforcing correct types for function calls, many Rimbu collections have a notion of "type variance".

For example, SortedSet<string> is invariant. It is not allowed to define const set1: SortedSet<string | number> = SortedSet.of<string>('a') as this would, as you point out, not be sound. In the same way, it is of course not allowed to define const set2: SortedSet<string> = SortedSet.of<string | number>('a').

However, as long as it's not possible to "modify" the collection (even if it's already immutable), it's perfectly fine to pass a SortedSet<string> object into a function that expects a ...Set<string | number>. The function expects a set containing strings or numbers, it will obviously only get strings in this case, but that's fine. However if we only have the SortedSet type, writing such a function is not possible.

To support this, Rimbu has variant versions of collections. For instance, SortedSet inherits from RSet, which is still invariant but not specifically sorted, which again inherits from VariantSet, which obviously is variant. VariantSet does not have add and remove methods, so it's safe to accept a wider element type.

With VariantSet it is possible to write more generic functions, for example:

import type { VariantSet } from '@rimbu/collection-types';
import { SortedSet } from '@rimbu/sorted';

const myStringSet = SortedSet.of('a', 'b');

function doSomethingWithSet(set: VariantSet<string | number>): boolean {
  return set.has(3) || set.has('c');
}

console.log(doSomethingWithSet(myStringSet));
// writes 'false'

Now for simple types this is not extremely useful. But consider if you are putting objects with some inheritance into the set:

interface PricedItem {
  name: string;
  price: number;
}

interface Fruit extends PricedItem {
  countryOfOrigin: string;
}

const mySet = SortedSet.of<Fruit>({
  name: 'Apple',
  price: 1.39,
  countryOfOrigin: 'Belgium',
});

function calculatePrice(set: SortedSet<PricedItem>) {
  return set.stream().fold(0, (result, item) => result + item.price);
}

function calculatePrice2(set: VariantSet<PricedItem>) {
  return set.stream().fold(0, (result, item) => result + item.price);
}

calculatePrice(mySet); // not allowed, type error

calculatePrice2(mySet); // no problem

This example is also convoluted since for this case you would probably use a Stream or Iterable which is variant too, but it's not hard to imagine some generic function that would really like to use some functionality of a set.

This is the fundamental reason for isComparable, since without it we could not have these type-level benefits. In the same way, the Hasher interface also has an isValid method for the same reasons.

[vitoke]

By the way, for the findIndex method you describe above, I think throwing an exception would indeed be fine, now that I understand the purposes better. The only way a user can pass an invalid argument into the method is by casting it into something it is not. And after a hard cast.. all bets and guarantees are off anyway.