Type inference not working due to Observable<any> in switchmap

[masaeedu]

The RxJS 5 beta has a new switchMap operator that is supposed to replace flatMapLatest. The signature for switchMap, on line 76 of CoreOperators.ts, looks like this:

switchMap?: <R>(project: ((x: T, ix: number) => Observable<any>), projectResult?: (x: T, y: any, ix: number, iy: number) => R) => Observable<R>;

Note the Observable<any> return type for the project func parameter. This means the R type won't be inferred if the method is called with a single observable-returning func. Unless I'm misunderstanding, the signature should be:

switchMap?: <R>(project: ((x: T, ix: number) => Observable<R>), ...)
                                                _____________

Similar changes would be made for switchMapTo (and possibly other operators I'm missing).

[david-driscoll]

There are many areas where type inference is a problem, something that was solved in RxJS4's typings. The deficiencies have been noted, there is just a long haul to get them all in through review for the RxJS5 team to take ownership of them.

I've got a set that at one point was working correctly, #1050, again all the pieces need to be reviewed.

[masaeedu]

@david-driscoll Would it be a good idea to just start cleaning these up with small PRs as we come across them? There's no behavioral changes involved, the generic types get erased anyway.

[david-driscoll]

That's the end game, but right now you have to modify the same definition in 3 places (at least it was when I last looked) which is a maintenance nightmare. I have a proposed automated system, but the team is still unsure if they want to do that or not.

[benlesh]

but the team is still unsure if they want to do that or not.

@david-driscoll I think it sounds good. It was just a huge PR to review. I'm delegating to @kwonoj for 👍 or 👎

[david-driscoll]

@Blesh I'm fine with breaking things apart, I know I'm not working on the library day to day. 😄

Holidays had me busy, I'm working on #1077 tonight.

[kwonoj]

As already commented in PR, I also in favor of essence of @david-driscoll 's proposal. What's remaining is to make it happen in codebase such as reviewing changes (taking time due to dependent change such as #1077 and nature of change itself is quite amount), etcs. (Means I'm 👍 in those proposal)

Though I expect @david-driscoll 's PR eventually be landed, due to reasons I think it won't happen in short timeframe such as day or two. So @masaeedu , if this is blocking your usecase I think it's OK to have specific PR to resolve this first while working on final solution for this.

[masaeedu]

@kwonoj I'll use explicit typings or any to work around the lack of inference until David's PR gets pulled. Thanks all!

[kwonoj]

Thanks for confirmation. Let me close this issue for now then.

[masaeedu]

@kwonoj It looks like @david's change has been merged, but the type signature for switchMap and other operators (e.g. flatMap) seem to be unchanged. It might be that I'm just misunderstanding how the typings are supposed to work. For flatMap, for instance, I think the typing should not be:

flatMap: <R>(project: ((x: T, ix: number) => Observable<any>), ...) => Observable<R>;

But rather:

flatMap: <R>(project: ((x: T, ix: number) => Observable<R> | R[]), ...) => Observable<R>;

The first change (Observable<any> to Observable<R>) enables type inference, and is what this issue is about. The second change (the addition of | R[]) is related to the fact that flatMap/mergeMap work with array returning functions as well as observable returning functions, and can possibly be considered in a separate issue.

Is this actually an oversight or am I just not grokking how the typings work?

[kwonoj]

@masaeedu , changes just checked in is about enabling compiler to not allow implicit any in codebase for prerequisite dependency to actual type definition enhancement. Those discussions are in PR #1087, you may able to see what'd be expected result of type definition.

Long story in short, just checked in PR does not bring type definition change and it'll happen in other PR currently opened. Is that answers your question?

[masaeedu]

@kwonoj It does, thanks. I'm digging into the codebase, will see if I can contribute some small fixes for the most painful methods while this automation effort is still underway.

[kwonoj]

@masaeedu appreciate for it, as we're trying to improve type definitions those will be great help :)

[sbley]

@masaeedu Thank you for finding these wrong type definitions for flatMap. I couldn't figure it out myself. I am using rxjs-5.0.0-beta2 which has the incorrect type definitions.

Is this fixed in a new version? If not, how can I work around this?

[masaeedu]

@sbley There's a bit of churn right now in the type signatures for various operators. @david-driscoll has been cleaning up a bunch of them, and once #1351 and #1352 are merged the user facing typings (including flatMap) should work more nicely.

If you're impatient you can clone and npm link https://github.com/david-driscoll/RxJS-1/tree/addObservableInterfacePatches-squash, which works pretty well for me.

[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.