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

Update typings for combineLatest #871

Conversation

david-driscoll
Copy link
Member

This is an example of the enhanced typings for #870

You can put your focus on src/operator/combineLatest.ts all the other files are related to #870.

@david-driscoll david-driscoll force-pushed the types/operator/combineLatest branch from 45ec41b to b184e18 Compare December 3, 2015 16:38
static range: typeof RangeObservable.create;
static throw: typeof ErrorObservable.create;
static timer: typeof TimerObservable.create;
static zip: typeof zipStatic;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the typeof trick is pretty cool. I didn't know that one. I'm a little concerned about the implications of importing all of the other Observable types in this file, though.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the imports are just use for types, they dissolve when TypeScript is emitting the results.

For example the cjs output for Observable.ts starts with

var Subscriber_1 = require('./Subscriber');
var root_1 = require('./util/root');
var SymbolShim_1 = require('./util/SymbolShim');
/**
 * A representation of any set of values over any amount of time. This the most basic building block
 * of RxJS.
 *
 * @class Observable<T>
 */
var Observable = (function () {

@benlesh
Copy link
Member

benlesh commented Dec 3, 2015

I'd like people who really care about the TS usability of this lib to comment: @IgorMinar, @jeffbcross, @robwormald

@benlesh
Copy link
Member

benlesh commented Dec 3, 2015

@david-driscoll I can see you've put a TON of work and thought into this. I'm really just trying to wrap my head around the needs and use cases this work is meant to meet. Whenever I see this much time and thought put into something, I really want to merge it, but it's on me to make sure it makes sense to do so.

@david-driscoll
Copy link
Member Author

@Blesh I totally understand 100%. I did the same amount of work for RxJS4 just to make my life working with the library that much easier. In some ways the value of TypeScript is kind of lost if your typings don't describe all the features. Sadly it's a case of JavaScript can do so much, so differently, describing it can be hard.

I'll take apart the combineLatest definitions to sort of explain what they do, and an example of how they can help.

Legend:

  • o: Observable
  • i: Iterable
  • ar: Array
  • p: Promise

Today combineLatest is defined as:

export function combineLatest<R>(...observables: Array<Observable<any> |
  Array<Promise<any>> |
  ((...values: Array<any>) => R)>): Observable<R> {
  let project: (...values: Array<any>) => R = null;
  if (typeof observables[observables.length - 1] === 'function') {
    project = <(...values: Array<any>) => R>observables.pop();
  }

  // if the first and only other argument besides the resultSelector is an array
  // assume it's been called with `combineLatest([obs1, obs2, obs3], project)`
  if (observables.length === 1 && isArray(observables[0])) {
    observables = <any>observables[0];
  }

  observables.unshift(this);

  return new ArrayObservable(observables).lift(new CombineLatestOperator(project));
}

Observable.prototype.combineLatest = combineLatest;

So basically it can be called many many different ways. With an array, with a rest param array, without a projection function, with a projection function, etc.

.combineLatest([o])
.combineLatest([p], (res_o) => res_o + 1)
.combineLatest(i)
.combineLatest(ar, (res_ar) => res_ar + 'abc')

This example is fairly arbitrary as, p will only emit one value, but subscribeToResult allows for all Obserable's, Promise's, Iterator's and Array's. All can have their own generic value from the type system, and there is no guarantee that they will be the same type.

The objective for me as a consumer, is that I want to have the type of res_o available as it were the value that is known to be emitted by the Observable<T2>, when I'm working in the project function, preferably without having to declare a type annotation.

For a more concrete demo, I'll use the playground.

Today's typings

In this example we get an error if we don't have annotations, forcing them on consumers.

Enhanced typings

In this example the need for the annotation is gone, because we've told the type system enough about our behavior that it can infer all the types automatically.

<TResult>(project: (v1: T) => TResult): Observable<TResult>;
<TResult>(project: (v1: T) => TResult): Observable<TResult>;

These are duplicated... whoops. 😄

<T2>(second: ObservableOrPromise<T2>): Observable<[T, T2]>;

By default without a project method, we will output what is essentially a Tuple, so here we're typing it as a tuple of T coming from the observable (which is really this) and T2 being the observable or promise that is being handed over.

Today with the code defined as above, we must intentionally say .combineLatest<[T, T2]>(o) to get the desired effect, otherwise all you get is {}. This adds a lot of type annotation noise, and really defeats the purpose behind the value TypeScript is trying to give us.

<T2>(array: [ObservableOrPromise<T2>]): Observable<[T, T2]>;

Arrays are a valid input type, this lets the consumer hand in a tuple, and get an appropriate output type. Otherwise we would require a type annotation again.

<T2, TResult>(second: ObservableOrPromise<T2>, project: (v1: T, v2: T2) => TResult): Observable<TResult>;
<T2, TResult>(array: [ObservableOrPromise<T2>], project: (v1: T, v2: T2) => TResult): Observable<TResult>;

The further along definitions are really where things come together, we can strongly type T and T2 and not have to create any type annotations in our methods, and TResult can simply be the return value of your method.

<T2, T3>(second: ObservableOrPromise<T2>, third: ObservableOrPromise<T3>): Observable<[T, T2, T3]>;

Again we get to now type three different types, without having to declare types.

<T2, T3>(second: ArrayOrIterator, third: ObservableOrPromise): Observable<[T, T2, T3]>;
<T2, T3>(second: ArrayOrIterator, third: ArrayOrIterator): Observable<[T, T2, T3]>;
<T2, T3>(second: ObservableOrPromise, third: ArrayOrIterator): Observable<[T, T2, T3]>;
.... ....
The real complicated bit comes down to what I see as a compiler bug, and I just need to see if there is a bug out there to fix it. Basically if you Union ArrayOrIterator with ObservableOrPromise then type inference breaks down, having the distinct overloads to handle each matrix (T, T2, T3, etc) solves the problem, but it makes the number of overloads grow exponentially.

This is perhaps something we don't need to model, because consumers could simply call Obserable.of. That said I left it for now for demonstration purposes, I don't have a super strong opinion either way, my goal was to make the typings work with all the scenarios vanilla JS could potentially handle, it doesn't mean that we have to do that. I would be happy to consider Observable and Promise first class citizens and delegate Iterable and Array to something that has to be converted to an Observable for TypeScript consumers.

@david-driscoll
Copy link
Member Author

Doing some reading and microsoft/TypeScript#5738 looks like it might solve the issue with array inference, I'll do some testing with typescript@next and see if that's the case.

@david-driscoll david-driscoll force-pushed the types/operator/combineLatest branch from 01acce0 to b766a66 Compare December 11, 2015 23:02
@lock
Copy link

lock bot commented Jun 7, 2018

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock bot locked as resolved and limited conversation to collaborators Jun 7, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants