Adding plumbing to support more granular and maintainable typings #870

david-driscoll · 2015-12-03T16:29:33Z

This is based on the original #715, I've broken it out into 2 branches so far, with other branches to follow after getting some feedback.

Today Rx has 3 or 4 places where the typings have to be updated, for the consumer to be able to actually get type information and consume that type information.

Observable.ts
Rx.KitchenSink.ts
CoreOperators.ts
The operator in question

The goal here is to be able to add well defined typings for an operator, and then extract that type information using a common convention, into another interface. This extraction is an automated process, such that as changes are made to the operator the interface will automatically be updated.
This new interface for the operator can then be used where it needs to be, by simply using typeof.
#715 was a proof of concept for me, and I only added all the typings I did because I was testing it with omnisharp-atom and omnisharp-client, both which use Rx4 extensively. Once all the work was done, the compile times were reduced, and there were minimal changes that needed to be made to the source for things to "compile" correctly (I ran into other issues, but that isn't important to this case)

Pros

All the type information now lives with the operator
Only have to make a change to 1 file, to update the consumer.
Allows us to have enhanced types for the operators, things like tuples for combineLatest instead of `Observable<any[]>

Cons

Additional step in the build workflow.
Adds some magic contributors need to know about
The operator injection onto Observable might be obsolete with TS 1.8
- I don't think we'll have the ability to type this yet, which is still one thing this tooling would be valuable for.
Currently doing this one operator at a time breaks the build, for no other reason than a stackoverflow. :(
I'm sure there are cons I'm missing at the moment

benlesh · 2015-12-08T18:54:03Z

src/Observable.ts

+import {TimerObservable} from './observable/timer';
+import {RangeObservable} from './observable/range';
+import {InfiniteObservable} from './observable/never';
+import {ErrorObservable} from './observable/throw';


I think this is going to introduce circular dependencies. If you rebase, you can run npm run check_circular_dependencies and see.

They'll get thrown away, because they're just used strictly for the types.

Would you able to share how this will be removed?

when it's transpiled to JS, it'll strip anything that's only used for type definitions (but retained in the .d.ts file)

import {Foo} from './foo' function doStuff(foo: Foo){}

=>

function doStuff(foo){}

I see. I'm feeling this specific changes (import types then assign static method with bring it via typeof) is orthogonal and can be separated - is my understanding correct? (at least just taking this changes locally into TOT seems working)

Yeah, those changes by themselves don't really depend on everything else

What do you think about separate this change into another PR? this change's quite straightforward and small, can be checked easily. Also it'll reduce this PR's scope.

benlesh · 2015-12-08T18:56:16Z

I'm still not 100% about this PR. It looks like something that will require maintenance over time, and the benefit seems dubious. I'm willing to admit I could be totally wrong, though. I'm also really waiting for more feedback from heavy TypeScript consumers like the Angular team. @IgorMinar @jeffbcross @robwormald

david-driscoll · 2015-12-08T21:28:09Z

I would love some more input as well because right now the TypeScript experience with RxJS5 is honestly quiet terrible. It forces you down a path of typing everything, when the types should just flow through without issue, if there were proper typings. In fact in some situations, if the class / interface isn't exported, you may not even be able to explicitly type anything without having to redefine the type yourself.

Additionally with the completed solution, I was seeing compile times reduced significantly, only because I think the compiler didn't have to work as hard keeping track of all anonymous types. So once it's all finished, there may be some compile perf wins as well.

benlesh · 2015-12-08T21:45:55Z

I would love some more input as well because right now the TypeScript experience with RxJS5 is honestly quiet terrible.

Same here, honestly. I'm mostly consuming this as ES6 with Babel or with Node and CJS.

Additionally with the completed solution, I was seeing compile times reduced significantly, only because I think the compiler didn't have to work as hard keeping track of all anonymous types.

That sounds good.

My only concern is whether the additional maintenance is worth the benefits. So far I have very little evidence to work off of. I did a little work with TypeScript and Angular using RxJS 5 for AngularConnect a few months ago, and I didn't notice any TS ergonomics issues.

Perhaps as we move the unit tests over the TypeScript (one proposal here #826), the issues this is attempting to solve will become more apparent. /cc @kwonoj

trxcllnt · 2015-12-08T21:46:30Z

@Blesh I disagree, this PR looks great. I had no idea TS let you export types like this. Great work @david-driscoll. Perhaps the fileResult data in typingsgen.js could be in a separate .ts (so we don't have to maintain a string as code in a script file)?

kwonoj · 2015-12-08T21:56:39Z

In general I like essence of this PR itself. at least aspect of precise type definition of interface, results created by this PR (or extended PR #871 to illustrate example) would serve better than current type declarations. But also have few curious regarding ergonomics of development like when someone try to add new operator, or if it's required to modify signatures of few operator only, etcs and other I might not able to think of by lean on code based interface generation.

robwormald · 2015-12-08T22:06:01Z

I definitely like the end result.

The pain points @david-driscoll mentions are real, in terms of having types properly propagate through operators. I'm less excited about the manual code generation bit, if only because it adds complexity to the process for contribs. I'll pull this down this evening and have a proper play with it. Either way, nice work!

benlesh · 2015-12-08T22:27:48Z

typingsgen.js

+import {GroupedObservable} from \'./operator/groupBy-support\';\n\
+import {GroupByObservable} from \'./operator/groupBy\';\n\
+import {TimeInterval} from \'./operator/extended/timeInterval\';\n\
+import {ObservableOrPromise, ArrayOrIterator, _Selector, _IndexSelector, _SwitchMapResultSelector, _ObservableMergeMapProjector, _IteratorMergeMapProjector, _Predicate, _PredicateObservable, _Comparer, _Accumulator, _MergeAccumulator} from \'./types\';\n\n\


Is there any way we can not have to manually maintain this?

It would still have to be maintained in some way most likely. That said we have a few things we could do to change how it works.

Change it around such that we edit the existing file, instead of writing a new one. That way if any new definitions bring in additional Observables / Operators that need to be referenced, we can just update the file itself, and no rely only on the file generation.

david-driscoll · 2015-12-08T23:17:30Z

In reality we need the following issues in TypeScript to land, to avoid having to deal with the code generation.

microsoft/TypeScript#5290 and microsoft/TypeScript#5269 plus we also need one one of microsoft/TypeScript#3508 (function bind syntax ::) or microsoft/TypeScript#3694 (ability to type this for a function)

david-driscoll · 2015-12-08T23:23:43Z

It may also be worth investigating microsoft/TypeScript#5290 to see if that could help (perhaps have the interfaces live inside the operators themselves?)

kwonoj · 2015-12-08T23:27:48Z

It may also be worth investigating microsoft/TypeScript#5290 to see

: would you able to elaborate usecases for this PR to those feature?

david-driscoll · 2015-12-08T23:52:01Z

I don't know if it it'll work, may be blocked by some of the comments from microsoft/TypeScript#5292

I just tried, and it only works if the class and interface declarations happen to be in the same file currently, I even tried export {Observable} from './operators/combineLatest'; to see if it would work, and while TypeScript wouldn't complain, it also wouldn't propagate type information.

david-driscoll · 2015-12-09T00:24:13Z

Rebased onto origin/master

benlesh · 2015-12-09T00:29:32Z

I'm not sure why my other comment didn't make it.

I'm by no means "against" this PR. I just don't want to make uneducated decisions and wanted to lean on the larger community of people that spend more type using RxJS 5 with TypeScript. I'm going to hand off handling of this to @kwonoj, and I'll review it before it's merged. My only concerns lay around maintenance, so if we can keep that as smooth as possible, I have no qualms.

david-driscoll · 2015-12-09T01:14:22Z

@Blesh My tone may have sounded a little harsher than I was intending, sorry about that 😄.

My goal is to help make something that we call an appreciate for years to come, by no means is it the only, or even the best solution, I do feel it fits in with the overall design that is currently here today.

david-driscoll · 2015-12-09T01:16:04Z

src/operator-typings.ts

+import {TimeInterval} from './operator/extended/timeInterval';
+import {ObservableOrPromise, ArrayOrIterator, _Selector, _IndexSelector, _SwitchMapResultSelector, _ObservableMergeMapProjector, _IteratorMergeMapProjector, _Predicate, _PredicateObservable, _Comparer, _Accumulator, _MergeAccumulator} from './types';
+
+/* ||| MARKER ||| */


As part of my rebase I also made a tweak here, any generate-rated interfaces will be but between the two markers, allowing maintenance to happen directly in the operator-typings.ts file, if we need to add in a new import for example.

…le methods

…e by default, to come from one

lock · 2018-06-07T14:02:34Z

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.

david-driscoll mentioned this pull request Dec 3, 2015

Update typings for combineLatest #871

Closed

benlesh reviewed Dec 8, 2015
View reviewed changes

david-driscoll force-pushed the common-type-plumbing branch from c765ebe to 0dc5ee2 Compare December 9, 2015 00:21

david-driscoll reviewed Dec 9, 2015
View reviewed changes

david-driscoll force-pushed the common-type-plumbing branch 4 times, most recently from afb2636 to 58b12fc Compare December 10, 2015 19:15

david-driscoll mentioned this pull request Dec 10, 2015

style(typings): Use typeof to capture typings for all static Observab… #1036

Closed

david-driscoll added 5 commits December 11, 2015 17:41

style(typings): Use typeof to capture typings for all static Observab…

4627e3d

…le methods

chore(typings): Adding plumbing to allow the types added to Observabl…

ad583eb

…e by default, to come from one

style(typings): Added ObservableInput type

b007210

chore(tsconfig): Remove codeFormatOptions, moving to separate PR

065cd5c

style(typings): Added the ability to compute typings

5fd05a2

david-driscoll force-pushed the common-type-plumbing branch from c991a7b to 5fd05a2 Compare December 11, 2015 22:54

david-driscoll closed this Jan 9, 2016

lock bot locked as resolved and limited conversation to collaborators Jun 7, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding plumbing to support more granular and maintainable typings #870

Adding plumbing to support more granular and maintainable typings #870

david-driscoll commented Dec 3, 2015

benlesh Dec 8, 2015

david-driscoll Dec 8, 2015

kwonoj Dec 9, 2015

robwormald Dec 9, 2015

kwonoj Dec 9, 2015

david-driscoll Dec 9, 2015

kwonoj Dec 9, 2015

benlesh commented Dec 8, 2015

david-driscoll commented Dec 8, 2015

benlesh commented Dec 8, 2015

trxcllnt commented Dec 8, 2015

kwonoj commented Dec 8, 2015

robwormald commented Dec 8, 2015

benlesh Dec 8, 2015

david-driscoll Dec 8, 2015

david-driscoll commented Dec 8, 2015

david-driscoll commented Dec 8, 2015

kwonoj commented Dec 8, 2015

david-driscoll commented Dec 8, 2015

david-driscoll commented Dec 9, 2015

benlesh commented Dec 9, 2015

david-driscoll commented Dec 9, 2015

david-driscoll Dec 9, 2015

lock bot commented Jun 7, 2018

Adding plumbing to support more granular and maintainable typings #870

Adding plumbing to support more granular and maintainable typings #870

Conversation

david-driscoll commented Dec 3, 2015

Pros

Cons

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benlesh commented Dec 8, 2015

david-driscoll commented Dec 8, 2015

benlesh commented Dec 8, 2015

trxcllnt commented Dec 8, 2015

kwonoj commented Dec 8, 2015

robwormald commented Dec 8, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

david-driscoll commented Dec 8, 2015

david-driscoll commented Dec 8, 2015

kwonoj commented Dec 8, 2015

david-driscoll commented Dec 8, 2015

david-driscoll commented Dec 9, 2015

benlesh commented Dec 9, 2015

david-driscoll commented Dec 9, 2015

Choose a reason for hiding this comment

lock bot commented Jun 7, 2018