cujojs / most

Ultra-high performance reactive programming
MIT License
3.5k stars 231 forks source link

Improve filter type definition #460

Closed thomas-jeepe closed 7 years ago

thomas-jeepe commented 7 years ago

Summary

The filter from rxjs uses typeguards in order to infer the new type of a stream after a filter operation. This isn't in most, so I implemented it.

Example by what I mean:

filter(isString, from([1, 2, 'hello']))
  .map(v => {}) // v at this point is inferred to be a string rather than string | number

I tried it out on my machine as a method of a stream and as a function. It works on both.

I don't think unit tests or documentation are necessary for this change, it falls back to the old typing if no type guard is found.

briancavalier commented 7 years ago

Hey @thomas-jeepe. This looks interesting, thanks for proposing it. I have a few questions, though, since I'm not a heavy TS user and not familiar with this feature.

Does this only allow constraining the type, or does it allow arbitrary type conversions? For example, would this allow someone to use filter to turn a Stream<number> into a Stream<boolean>? I doubt it allows arbitrary type conversion, but I just want to make sure :).

I do like the strong guarantee that filter makes currently: it cannot change the stream's type. That said, further constraining the type seems ok, since it wouldn't break anything downstream.

I tend to consider heterogenous streams to be a bit of an antipattern. Have you run into real world use cases where you've not having the type guard has prevented you from using filter?

thomas-jeepe commented 7 years ago

I tried turning a stream of numbers into a stream of booleans and got this error:

A type predicate's type must be assignable to its parameter's type.
  Type 'boolean' is not assignable to type 'number'.

So, that doesn't allow arbitrary type conversion.

The reason I normally would use it is if I have a stream of something that can be undefined or a certain type and I want to filter out undefined. Filter does this, but after I would have to manually assert the type. Example:

// with type guard
from([1,undefined,2])
  .filter(isNumber)
  .map(v => ...)
// without
from([1,undefined,2])
  .filter(isNumber)
  .map((v: number) => ...)

It is only a quality of life improvement.

TylorS commented 7 years ago

From a type perspective this should be :+1: and shouldn't break anything for anyone. I've used this behavior before actually. I've done similar to the following

const fooOrError$: Stream<Foo | Error> = 
  switchLatest(map(data => recoverWith(just, makeRequest(data)), data$))
// currently requires type-casting
const error$: Stream<Error> = filter(isError, fooOrError$) as Stream<Error>
const foo$: Stream<Foo> = filter(isFoo, fooOrError$) as Stream<Foo>

As far as I can tell, this would allow the removal of the type-casting.

thomas-jeepe commented 7 years ago

@TylorS Yep, rxjs also has a typeguard definition for filter.

briancavalier commented 7 years ago

Thanks @thomas-jeepe and @TylorS. I like it. It seems very useful for filtering sum types.