RubenVerborgh / AsyncIterator

An asynchronous iterator library for advanced object pipelines in JavaScript
https://rubenverborgh.github.io/AsyncIterator/docs/
Other
48 stars 7 forks source link

[Feature] Overload filter type #30

Closed jeswr closed 3 years ago

jeswr commented 3 years ago

I think it would be quite useful to have an overload on the filter method for the asyncIterator that is

filter<K extends T>(filter: (item: T) => item is K, self?: any): AsyncIterator<K>;

This would then allow use in cases such as the following:

const termIterator: AsyncIterator<RDF.Term> = /* Some iterator */
const namedNodeIterator: AsyncIterator<RDF.NamedNode> = termIterator.filter<RDF.NamedNode>((term): term is RDF.NamedNode => term.termType === 'NamedNode')
RubenVerborgh commented 3 years ago

Good idea, yes!

We probably just need to add that single line then? Happy to take a PR.

rubensworks commented 3 years ago

I just noticed that this breaks a bunch of things in Comunica. I suspect this is because the overloaded method disappears after compilation.

The compiled .d.ts file only contains the following now:

    filter<K extends T>(filter: (item: T) => item is K, self?: any): AsyncIterator<K>;

While it should contain:

    filter<K extends T>(filter: (item: T) => item is K, self?: any): AsyncIterator<K>;
    filter(filter: (item: T) => boolean, self?: any): AsyncIterator<T>;

Overloads always felt a bit shaky to me, so I've never used them myself, and therefore I have no solution at hand for this. @jeswr Any idea what may be going wrong here?

RubenVerborgh commented 3 years ago

@rubensworks I think we explicitly need to add the main function signature too.

jeswr commented 3 years ago

Yeah that's my bad, pretty sure it should be

  filter<K extends T>(filter: (item: T) => item is K, self?: any): AsyncIterator<K>;
  filter(filter: (item: T) => boolean, self?: any): AsyncIterator<T>;
  filter(filter: (item: T) => boolean, self?: any): AsyncIterator<T> {
    return this.transform({ filter: self ? filter.bind(self) : filter });
  }

Might be worth upgrading the tests to TS at some stage to catch this kind of thing.