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

Improve filter type definition #460

Merged
merged 1 commit into from
Jun 26, 2017
Merged

Conversation

thomas-jeepe
Copy link
Contributor

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.

If someone is using a typeguard, this changes the type of the stream and makes it easier to use. Example:

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

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
Copy link
Contributor Author

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
Copy link
Collaborator

TylorS commented Jun 26, 2017

From a type perspective this should be 👍 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
Copy link
Contributor Author

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

@briancavalier
Copy link
Member

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

@briancavalier briancavalier merged commit 834fecf into cujojs:master Jun 26, 2017
This pull request was closed.
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

Successfully merging this pull request may close these issues.

3 participants