fix(pipe): replace rest parameters overload #3945
Conversation
|
I'll rebase this PR after #3944 is merged. I've created this separately as I thought that would be better than lumping everything into one PR. It's a pity that dependent PRs aren't a thing. |
The build is failing✨ Good work on this PR so far! ✨ Unfortunately, the Travis CI build is failing as of 8be1b88. Here's the output:
|
|
Hmm. This PR is uncovering a bunch of typing problems in the test suite. |
type-specs/Observable-spec.ts
Outdated
| function a<I, O extends string>(inputOrOutput: I | O, output?: O): OperatorFunction<I, O> { | ||
| return mapTo<I, O>(output === undefined ? inputOrOutput as O : output); | ||
| } | ||
|
|
There was a problem hiding this comment.
we actually just hit an issue where the following broke in one of our apps here at Google:
const customOperator = () => <T>(a: Observable<T>) => a;
of('string').pipe(customOperator());NOTE: customOperator is () => <T>(source: Observable<T>) => Observable<T> NOT <T>() => (source: Observable<T>) => Observable<T>, which seems to work fine.
Can we add a test for that here too?
There was a problem hiding this comment.
I narrowed it down to our use of 'noStrictGenerics' flag, which we should be using anyways. Anyways, good idea to add a test for it, though I expect it to just pass in your setup.
|
@benlesh I've added a test (and moved some files that weren't in the renamed directory). The test is at the bottom of the file: it('should support operators that return generics', () => {
const customOperator = () => <T>(a: Observable<T>) => a;
const o = of('foo').pipe(customOperator()); // $ExpectType Observable<string>
}); |
src/internal/Observable.ts
Outdated
| @@ -318,7 +318,7 @@ export class Observable<T> implements Subscribable<T> { | |||
| * .subscribe(x => console.log(x)) | |||
| * ``` | |||
| */ | |||
| pipe<R>(...operations: OperatorFunction<T, R>[]): Observable<R> { | |||
| pipe<R>(...operations: OperatorFunction<any, any>[]): Observable<R> { | |||
There was a problem hiding this comment.
One thing to consider here is pipe<R=any>, so that you don't over-rely on return type for inference.
There was a problem hiding this comment.
@rkirov Can the inference really bubble out of the implementation? I'll take your word for it, as I'm unclear on the actual inference rules. (If you know of any resources - other than the TypeScript source - that explain the workings of TypeScript's type inference, I'd really appreciate if you could direct me to them.)
Anyway, I'm not sure that R is even required here, shouldn't we use something like this?
pipe(...operations: OperatorFunction<any, any>[]): Observable<any>With the any type parameters already being specified in the rest-parameters type and with it being the implementation of an overloaded function, does R really offer any value whatsoever?
There was a problem hiding this comment.
You are right about "bubbling out", my comment was pertaining to line 299, but I got mixed up. The types here are only relevant for 'implementation', not for the consumer.
About learning how type inference works - TS doesn't maintain a spec any longer, so the best one can do if they want to learn the rules is keep testing with small examples (what I do) or read the source code (haven't done that for type inference).
What makes matters hard here is that this is mixing generics and function overloads. Separately each one has odd corners, and together things are twice as hard.
Historically, we (team maintaining TS in google) have been having a lot of issues with generics that are only inferred through the return type of a function - simplest example being f<T>(): T. TS will try to use an context type to pick T, but sometimes there will be no such type to pick, so it will fallback to {}.
What happens is that in simple cases everything works fine:
const x: string = f(); // T is inferred to be string
const y = f(); // y is {}, which is ok, its an opaque object.But what we see the inferred {} can be propagating too much, and into another loosely typed API.
function g() {
return f();
}
function someAPI(x: any) {...}
someAPI(g());Now, if the maintainer tries to change someAPI to be stricter say 'x: string', they are confronted with a {} that flowed through without anybody noticing, which makes it hard to change. That's why we are considering whether it would have been better if the fallback for when TS cannot infer the generic type should have been any instead of the default {} (technically it is the lower bound, i.e whatever is after extends). Paradoxically, even though any is less safe, it can help making other type change stricted.
Maybe it would have been better if TS forced people to write a type like f<Something>, instead of fallback on {}.
We are still figuring out what is most ergonomic for ppl, would love to hear your opinion too.
There was a problem hiding this comment.
Maybe it would have been better if TS forced people to write a type like
f<Something>, instead of fallback on{}.
Yes. I agree wholeheartedly with this.
src/internal/Observable.ts
Outdated
| @@ -296,7 +296,7 @@ export class Observable<T> implements Subscribable<T> { | |||
| pipe<A, B, C, D, E, F, G>(op1: OperatorFunction<T, A>, op2: OperatorFunction<A, B>, op3: OperatorFunction<B, C>, op4: OperatorFunction<C, D>, op5: OperatorFunction<D, E>, op6: OperatorFunction<E, F>, op7: OperatorFunction<F, G>): Observable<G>; | |||
| pipe<A, B, C, D, E, F, G, H>(op1: OperatorFunction<T, A>, op2: OperatorFunction<A, B>, op3: OperatorFunction<B, C>, op4: OperatorFunction<C, D>, op5: OperatorFunction<D, E>, op6: OperatorFunction<E, F>, op7: OperatorFunction<F, G>, op8: OperatorFunction<G, H>): Observable<H>; | |||
| pipe<A, B, C, D, E, F, G, H, I>(op1: OperatorFunction<T, A>, op2: OperatorFunction<A, B>, op3: OperatorFunction<B, C>, op4: OperatorFunction<C, D>, op5: OperatorFunction<D, E>, op6: OperatorFunction<E, F>, op7: OperatorFunction<F, G>, op8: OperatorFunction<G, H>, op9: OperatorFunction<H, I>): Observable<I>; | |||
| pipe<R>(...operations: OperatorFunction<any, any>[]): Observable<R>; | |||
| pipe<A, B, C, D, E, F, G, H, I, R>(op1: OperatorFunction<T, A>, op2: OperatorFunction<A, B>, op3: OperatorFunction<B, C>, op4: OperatorFunction<C, D>, op5: OperatorFunction<D, E>, op6: OperatorFunction<E, F>, op7: OperatorFunction<F, G>, op8: OperatorFunction<G, H>, op9: OperatorFunction<H, I>, ...operations: OperatorFunction<any, any>[]): Observable<R>; | |||
There was a problem hiding this comment.
@rkirov Regarding our conversation that followed your comment, do you think it would be preferable for the signature with the rest parameters to not include R and to return Observable<any>?
With ten (!) type parameters, it really isn't expected that the caller will make them explicit, which means the return type will be Observable<{}>.
Or should the signature use R, but provide a default: R = any?
There was a problem hiding this comment.
@evmar and I were just chatting about the options here. Here is the total space of options (simplified to only talk about the return type).
// using {} (could be changed to unknown post TS 3.0)
declare function f1<T>(): T;
declare function f2(): {};
declare function f3<T={}>(): T;
// using any
declare function g1(): any;
declare function g2<T=any>(): T;He convinced me that picking up 'any' without ever writing it explicitly in their user code is a bad idea. Also, f1 and f3 are equivalent (at least I haven't gotten an example where they can be separated). So what's left is f1 and f2. Evan's point (and I tend to agree) is that f2 is more explicit because:
let x: string = f1(); // ok
let y: string = f2(); // error, needs an 'as' cast
let z = f2() as string; // okSo our suggestion is f2(), i.e. inline Observable<{}> and drop the R. The downside is the syntactic connivence of f<string>() vs f() as Observable<string>. Thankfully, the 'as' casts are expressions like any other, so they can be used anywhere, and we are just talking about syntactic preference.
There was a problem hiding this comment.
Thanks. That makes sense.
cartant
force-pushed
the
issue-3841
branch
2 times, most recently
from
74c17d4 to
87097f4
Compare
|
@benlesh As discussed with @rkirov, the Note that this makes it even more likely that there will be typing issues exposed in users' code when/if a release including this PR's changes is published. |
Replace the rest parameters overload with a signature that also includes the A-I type parameters. Closes ReactiveX#3841
cartant
changed the title
|
Solid add! |
Description:
IMPORTANT: This PR depends upon the dtslint additions in #3944.WARNING: This PR uncovered several typing problems in the test suite, so it's likely that it will also uncover typing problems in user code that were previously hidden. Said problems in the test suite could have been hidden for a number of reasons:
It fixes a problem that was introduced in this commit. Prior to that commit, the
pipeoverload signature for more than 9 arguments looked like this:Which is incorrect, as consecutive arguments have to have matching output/input types - every
OperatorFunctionpassed cannot accept aTand return anR.Unfortunately, fixing that by changing the signature to the following introduced another problem:
With that signature, any usage with 9 or fewer arguments that does not match a signature due to type errors will now match the rest parameters signature. So strong typing is subverted and that's the problem highlighted in #3841.
In a comment in this TypeScript issue, Anders Hejlsberg has pointed out the output/input relationship between arguments means that the overloads cannot be avoided. So to restore strong typing, this PR replaces the rest parameters overload signature with a signature that also includes the preceding 9 type-parameter-specified arguments, like this:
The effect of this is that it will no longer be possible to specify a single
Rtype parameter forpipecalls that involve more than 9 arguments. Instead, an assertion would need to be used like this:This PR also adds dtslint tests for
pipeand the staticpipeutility function that include expectations for type inference and errors.Related issue (if exists): #3841