-
Notifications
You must be signed in to change notification settings - Fork 3k
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
Update typings for combineLatest #871
Conversation
45ec41b
to
b184e18
Compare
static range: typeof RangeObservable.create; | ||
static throw: typeof ErrorObservable.create; | ||
static timer: typeof TimerObservable.create; | ||
static zip: typeof zipStatic; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 () {
I'd like people who really care about the TS usability of this lib to comment: @IgorMinar, @jeffbcross, @robwormald |
@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. |
@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 Legend:
Today
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.
This example is fairly arbitrary as, The objective for me as a consumer, is that I want to have the type of For a more concrete demo, I'll use the playground. In this example we get an error if we don't have annotations, forcing them on consumers. 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.
These are duplicated... whoops. 😄
By default without a project method, we will output what is essentially a Tuple, so here we're typing it as a tuple of Today with the code defined as above, we must intentionally say
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.
The further along definitions are really where things come together, we can strongly type
Again we get to now type three different types, without having to declare types. <T2, T3>(second: ArrayOrIterator, third: ObservableOrPromise): Observable<[T, T2, T3]>; This is perhaps something we don't need to model, because consumers could simply call |
Doing some reading and microsoft/TypeScript#5738 looks like it might solve the issue with array inference, I'll do some testing with |
bfc6550
to
5a0346f
Compare
5a0346f
to
01acce0
Compare
…e by default, to come from one
01acce0
to
b766a66
Compare
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. |
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.