Added better typings for combineAll and combineLatest

[david-driscoll]

I'm just checking the temperature here to see what the feels are of this change before I go and fix everything. I went around and fixed all the typings to allow for better type inference in RxJS 3/4 and was exploring using RxJS 5, and ran into some type inference issues with the current declarations.

At first I thought it would be possible to just get the types right on each command simply use typeof abc everywhere, sadly for instance methods that are not specifically defined on a class, that can't be done :(.

Once we get microsoft/TypeScript#3694, it might be possible, but I don't know if that will fix what we're looking for.

I feel like the CoreOperators<T> and Observable<T> changes could be automated, by adding it to the build. Basically the only difference between src/operators/combineLatest and Observable<T>.combineLatest is that T comes from the Observable class, instead of being a generic type. We could easily make some code that strips out export function abc<T... and transpose it to abc: <...

[kwonoj]

I tried to try out this changes locally but interestingly with this changes I'm not able to build (tsc stucks), seems travis also does. Just by reading changes there seems not quite suspicious though - anything have in mind?

[david-driscoll]

Probably something small, I'll look at later tonight. That said I'm looking
mainly for input from the team if is something you guys are okay with.
On Thu, Nov 12, 2015 at 5:28 PM OJ Kwon [email protected] wrote:

I tried to try out this changes locally but interestingly with this
changes I'm not able to build (tsc stucks), seems travis also does. Just by
reading changes there seems not quite suspicious though - anything have in
mind?

—
Reply to this email directly or view it on GitHub
#715 (comment).

Thanks,
David

[kwonoj]

Ergonomics wise I like the idea itself, but also want to try out some small code snippets myself before add some comments :) One small concern currently I have is it makes type definition bit increased and would like to have some fancy way such as automating, etcs but can discuss it in further.

[david-driscoll]

So on my way home, I had an idea, that will automate the extraction of the methods, I've updated the branch with that now.

Interestingly after I changed around the properties a bunch of typing issues popped up that had to be resolved. Still a work in progress, at the moment.

[david-driscoll]

I just noticed and interesting side effect, build times and memory consumption have dropped dramatically with these changes. Which honestly worries me, because I'm not entirely sure why. Now that said all tests pass, so it maybe that the typescript compiler is having an easier time reasoning about the generics after the changes.

latest master	pr current
Files: 171	Files: 173
Lines: 28073	Lines: 28592
Nodes: 127301	Nodes: 130572
Identifiers: 42529	Identifiers: 44266
Symbols: 762931	Symbols: 74235
Types: 410721	Types: 23778
Memory used: 645920K	Memory used: 120301K
I/O read: 0.02s	I/O read: 0.02s
I/O write: 0.09s	I/O write: 0.06s
Parse time: 0.70s	Parse time: 0.77s
Bind time: 0.41s	Bind time: 0.45s
Check time: 15.62s	Check time: 1.64s
Emit time: 1.10s	Emit time: 0.52s
Total time: 17.83s	Total time: 3.38s

[david-driscoll]

So... I wasn't planning on making this a huge PR but things just kind of evolved.

No actual code was harmed in the making of this Pull Request... just strictly the TypeScript declarations. This now lets types flow more easily between different chains of code, making it so that as a TypeScript consumer of the library, you should rarely have to specify generic type arguments, they should be inferred for you automatically in most cases.

[kwonoj]

Seeing PR's growing up, including changes outer scope of original operator combineAll and combinelatest. Since this PR is for discuss logistics of change itself, what about limit changes into those for now? I assume anyone want to try out this PR would be bit surprised by seeing +100 files changed.

[david-driscoll]

Things just kind of snowballed due to how the existing operators are
defined, I've been moving over an existing code base that uses a vast
majority of the operators, so as I've been encountering ones that don't
work, or fail to flow types properly, I've went and fixed those ones as
well.

I'm totally open to trying to isolate the changes, and break it down into
smaller PRs if that would help.

On Mon, Nov 16, 2015 at 5:18 PM, OJ Kwon [email protected] wrote:

Seeing PR's growing up, including changes outer scope of original operator
combineAll and combinelatest. Since this PR is for discuss logistics of
change itself, what about limit changes into those only? I assume anyone
want to try out this PR would be bit surprised by seeing +100 files changed.

—
Reply to this email directly or view it on GitHub
#715 (comment).

[benlesh]

A agree with @kwonoj the scope of this PR has gotten really large. 123 files and 17 commits. It's really hard to follow all of that and would take forever to review the changes in earnest.

I still haven't had time to grok what the crux of this PR is. On the surface, it appears to be refactoring type information and interfaces for TypeScript, but there is so much red and green I can't follow what each change is.

[kwonoj]

I've been encountering ones that don't work, or fail to flow types properly, I've went and fixed those ones as well.

: I think these can be address after core of this PR lands into master, can be handled with per each cases. If you don't mind, would you able to reduce PR's scope very core of changes with small examples? You may even does not have to modify current codebase heavily for proposal PRs, such as #643 did creating stub codes for simplify changes to discuss.

[david-driscoll]

Absolutely, I'll see if I can find a good minimum case to get started with later tonight.

[benlesh]

I like the spirit of this PR, and I'd love to see it broken into smaller PRs, but I'm going to close this PR for now, because it's just too big to merge. Really, I'd like just one PR to start with, so I can wrap my head around the change and why it's necessary. Creating an Issue on the matter might help as well.

@david-driscoll This was a TON of work, and I really appreciate that, do you think you can port the work into smaller bites to make it easier for us to get a handle on what the changes are?

Thanks again for being awesome.

[david-driscoll]

I was busy the last bit with other things, but yeah, I plan to split it up into more consumable pieces, hopefully tonight or tomorrow.