manual typings for bindCallback.create

[david-driscoll]

Comparing against generation of types to spur on discussion of the best approach. I picked a single file here just as an example case. See #1087

As a reference for TypeScript itself, these typings can't be simplified much more without losing the enhanced experience of operating against them.

microsoft/TypeScript#5453 may be the answer, not sure if it answers all the possible use cases. It is currently on the road map for 2.0.

I realize there will be tons of opinions my only goal is to make the TypeScript experience with RxJS5 as good as we have gotten the experience in RxJS4.

[benlesh]

@david-driscoll I think you proposed this at an inopportune time (before the holidays).

Do my heavy TypeScript users have any opinions? @robwormald @jeffbcross @kwonoj

[kwonoj]

Discussion at the moment makes this discussion pause until #1077 is landed in codebase, to separate scope of changes.

Shortly, I still need spend some time to review these changes.

[kwonoj]

Just to ensure, PR intended to be checked in is #1087, this PR shows handcrafted changes to compare with original PR.

[robwormald]

the better the types in the distribution, the better, as far as i'm concerned. i'll take an extra few kb's of npm download for better intellisense. Up to y'all in terms of maintainability.

[david-driscoll]

@kwonoj yes, I would prefer to check in is #1087, but I offered both to allow for comparing the difference between the two.

I have also rebased against master.

[david-driscoll]

@kwonoj / @luisgabriel

After doing some work today, I'm thinking this branch might be the preferred method, I added some "unit tests" as well (basically to ensure that things compile as expected).

[kwonoj]

this branch might be the preferred method,

meaning between #1087 and this?

I added some "unit tests" as well

Would you able to separate those? You may able to see there are pending PR in discussion #826 #667 for enabling test against type definitions, haven't concluded which way'll go. If your changes separated, that can be considerable option too.

[david-driscoll]

Correct, this over #1087, it's a little more manual work, but the quality will probably be better.

If you look at bindCallback-tpec.ts for example, it doesn't actually do anything meaningful, in terms of testing functionality, it just validates that interface defined for bindCallback will compile as expected with the correct type inferences, etc.

[kwonoj]

If you look at bindCallback-tpec.ts for example,

yes, aware of those. Current discussion regarding test is about organization of test since it's testing definition only - so it'll be matter of trying to use existing PR's mechanism, or following your PR's mechanism to invoke those. Once it's decided, test can be added fairly quickly.

[david-driscoll]

@kwonoj done!

[kwonoj]

Thanks! I still need to look changes to add some inputs.. (my work-for-pay-bill blocks me, sorry for late response)

[kwonoj]

this patch operator's now auto generated, so instead of changing this script need to be updated - is that correct?

[luisgabriel]

Is this change needed?

[david-driscoll]

yes, because combineLatest (the method) no longer has the same definition as combineLatest the prototype method.

I plan to update the script to add this in, and that's when I found out that the generation script didn't work the other day.

[luisgabriel]

Hmn. Makes sense.

[david-driscoll]

I've updated this in the last third commit as something separate from the other work. (Hence the larger number of files changed)

[masaeedu]

I'm not sure this should be needed. The implementation function should be assignable to the interface. If it doesn't do that, it means there's some overloads in the interface that the implementation isn't equipped to handle.

In the case of combineLatest, the interface is advertising to users that they can pass in iterators, promises or arrays wherever observables are expected, however the function isn't written to expect these and will explode. This is causing the assignability error, which is a good thing.

[masaeedu]

I would recommend narrowing ObservableInput<T> to just Observable<T> until we can figure out a way to generically deal with iterators, arrays and promises in the actual implementations (which would be absolutely magical!)

[kwonoj]

Let me ask few questions as 'newcomer' point of view, let's say I'm completely new to codebase and compose new operator require similar function signatures

• How do I determine if operator requires this 1 to n type definition or not?
• Where can I refer to create those signatures? such as base templates, examples, etcs
• How do I correctly verify once it's composed to send out PR? (Yes we do have Add test for type definition validation? #661, questions about 'how to compose those test')

There might be some other questions I could possible missing.

[kwonoj]

• would you agree to close generated typings for bindCallback.create #1087 in case if this PR's expected to be checked in? (confirming if I understood correctly)

[david-driscoll]

It only apply to methods that have a method that has some sort of applied / combined output or input. Which is actually only a few, but they are the important ones to capture, because it makes type inference that much better. Exaples are combineLatest, zip, withLatestFrom, etc.

The only other most common case is methods with an optional selector. You'd want one overload without the selector, because generally those return the incoming type T in some form, and the one with the selector would return R.

[luisgabriel]

I don't now what do you guys think but I'd prefer to split this into two separate commits: one for biundCallback and another for combineLatest.

[david-driscoll]

@kwonoj rebased! I was able to eliminate the calls, so that's already been removed from the changes.

[masaeedu]

@david-driscoll Right now the implementation for combineLatest accepts Array<ObservableInput<any>>. This won't work once the lift method is fixed, because you can't lift an Observable<Observable<any> | Promise<any> | Iterator<any> | ArrayLike<any> ...> through an Operator<T, R> and produce an Observable<R>.

I'd suggest narrowing ObservableInput to Observable only for now, so that the API exposed to the user reflects what the operators can actually handle.

[david-driscoll]

@masaeedu I don't see any constraints in the code that would cause any compilation problems. To validate I merged with your changes (which merge cleanly btw! ❤️ git).

Do you have a specific example where you have seen issues pop in?

[masaeedu]

That's odd, it doesn't compile for me. The changes I'm making are to remove T from Observable.lift and removing T, R from Operator.call. Once you do that, you get an error on the last line of combineLatest explaining how new ArrayObservable(observables).lift<R> expects an Operator<Observable<any> | Promise<any> ..., R>, which an Operator<T, R> does not satisfy. Could you try making those two changes and attempting a compile? I can provide a patch file if you want.

[david-driscoll]

Want to push your local branch and I'll check it out and see if I can see what you're seeing?

[masaeedu]

@david-driscoll Yup, here you go: https://github.com/masaeedu/RxJS/tree/typings-option-a. Look at the error for combineLatest.ts.

[david-driscoll]

Found the problem.

Your changes removed <T, R> from new ArrayObservable(observables).lift(new CombineLatestOperator(project)); hence why I didn't the problem when merged. 😄

Now that said the real solution isn't actually anything that is wrong, the typings are just incorrect.

Currently ArrayObservable is typed like ArrayObservable<Observable<any>> or with my changes ArrayObservable<ObservableInput<any>>. This isn't actually correct, instead ArrayObservable should container whatever type is contained by the inputs. (Observables, Promises, Iterables, Arrays, etc).

The best way I see to type around that is something like:

new ArrayObservable<T>(<any[]>observables).lift<R>(new CombineLatestOperator<T, R>(project));

This produces the effect that we're looking for. In call cases of ArrayObservable, should probably be typed similarly.

[masaeedu]

@david-driscoll You have to remove at least T from new ArrayObservable(observables).lift, since there's only one free generic parameter in lift after the fix. Specifying R is unnecessary since it can be inferred from the type of the Operator being passed in, but even if you specify it this doesn't fix the problem (at least on my setup).

[masaeedu]

The reason I think the error is understandable and correct is that an Operator<T, R> can only turn an Observable<T> into an Observable<R>. The code does not exist to allow an operator to deal with Promises or Iterables or whatever else. With your typings, a user would assume they can pass promises or arrays into combineLatest where before Observables were expected, but there is no additional code to deal with this and it would cause runtime errors (had it compiled).

[david-driscoll]

Two different problems.

1. The problem you noted was dealing with an Observable of Observable<T>, which should only be Observable<T>. Also for the code itself T isn't really used, so just confuses matters to be honest.

2. Once you dig into combineLatest-support.ts you can see that it just shifts each value from the ArrayObservable into an array. From there on complete it calls through to subscribeToResult, which handles among other things Observables, Promises, Arrays and Iterables.

[masaeedu]

Ah, I wasn't aware subscribeToResult transparently deals with other types of items. Sorry about that; I was assuming that because everything is typed to Observable, there was no code to deal with the other stuff.

I actually have a version of CombineLatestOperator with updated typings here, where the signature is:

class CombineLatestOperator<T extends Observable<U>, U, R> implements Operator<T, U[] | R>

I would just need to update this to:

CombineLatestOperator<T extends ObservableInput<U>, U, R> implements Operator<T, U[] | R>

I'll update stuff in my branch.

[kwonoj]

I've already marked this as lgtm & latest update removes <any> casting resolves some of questions we had. I'll check this in around this afternoon if there's any other suggestions around.

[benlesh]

It looks like there are some merge conflicts... @david-driscoll can you rebase this?

@kwonoj, feel free to merge this whenever you think is appropriate if you feel this is an improvement to our typings.

[kwonoj]

Will do. I've already completed reviewing this and so far I believe good to check in. I'll do it myself once it's rebased.

[masaeedu]

lgtm, for what it's worth. Is this going to be a pattern that is followed uniformly for all the operators or just for the ones with complicated type signatures?

[kwonoj]

@masaeedu Not all of operators. There are some operators allows these kind of n-numbers of parameter s, those need to be updated like zip, withlatestfrom (#1208 , #1209)

[kwonoj]

@david-driscoll , I'll check this in once it's rebased. (Gentle reminder, not poking 😉 )

[david-driscoll]

Since these have been merged, I added definitions for combineLatestStatic that mirror the signatures.

[david-driscoll]

@kwonoj rebased.

[kwonoj]

Thanks! I'll check this in.

[kwonoj]

Merged with 7c9547a, appreciate for huge effort @david-driscoll 😄

[lock]

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.