Update of.ts

[kolodny]

Remove deprecation warning for valid use of of

[cartant]

LGTM

[kolodny]

Actually, the more I think about it, we probably just need to switch the order of the overloads but keep the deprecation in for the of<T>(t: T) case. That way folks doing of(new Foo()) won't get a deprecation warning but doing of<Foo|null>(null) would warn, which is what we really want.

[cartant]

...switch the order of the overloads but keep the deprecation...

Yeah. I agree. Might as well move both of the deprecated signatures so that they follow the rest-parameter signature. They'll only be matched if an explicit type argument is specified.

Although, could that break anyone doing of<string[]>(["foo", "bar"]) as it will match the rest-parameter signature? Does that matter?

[kolodny]

Done, I wish there was a way to add a test for this. The closest I can do is mock the state here and link to a playground where it works

declare function of(): 'OK';
declare function of<A extends Array<any>>(...args: A): 'OK';
declare function of<T>(): 'deprecated';
declare function of<T>(value: T): 'deprecated';

of() === 'OK';
of(1) === 'OK';
of(1, 'test') === 'OK';
of<number>() === 'deprecated';
of<number>(1) === 'deprecated';

of() === 'deprecated'; // sanity check, this line is a type error.

[kolodny]

...Although, could that break anyone doing of<string[]>(["foo", "bar"]) as it will match the rest-parameter signature? Does that matter?

of<string[]>(["foo", "bar"]) === 'deprecated' in the example above which is what we're after, they would need to do of<string[][]>(["foo", "bar"]) === 'OK'. I'm good with that tradeoff

[cartant]

... I wish there was a way to add a test for this ...

Yeah. I'm inclined to look into adding another assertion to dtslint. I guess it should be possible to add $ExpectDeprecation. If it's not too hard, I think we should look into it before the v7 release.

It would also be useful to have its dual: $ExpectNoDeprecation.

[kolodny]

This PR may not be ready due to microsoft/TypeScript#33597

We may need to keep the original order and just remove the deprecation warning for of<T>(t: T)

[cartant]

@kolodny @benlesh Looking at this again, I'm not sure why we even need ValueFromArray. In of, it's always going to be an array and the ValueFromArray type is not used elsewhere. I think the signature should be:

export function of<T>(...args: T[]): Observable<T>;

And all of the other signatures can be re-ordered and deprecated.

Regarding microsoft/TypeScript#33597, if this signature is used the behaviour won't be a problem with of and AFAICT it won't be a problem elsewhere as the widening only happens with literal types (and elsewhere we are are dealing with observable inputs, etc. and not literal types).

Never mind. I just remembered why. It's one of TypeScript's we-decided-it-should-behave-this-way 'features' (see microsoft/TypeScript#30824):

declare function f<T>(...args: T[]): T;
const r = f(1, "2", "3"); // Argument of type '"2"' is not assignable to parameter of type '1'

IMO, this behaviour, in conjunction with microsoft/TypeScript#33597, means that this part of TS is essentially broken. 😡

[kolodny]

I believe this PR is ready to go. This will be a slight breaking change to folks doing

interface MyAction {
  type: 'Action1' | 'Action2'
}

// Error: Type 'string' is not assignable to type '"Action1" | "Action2"'.
const ob$: Observable<MyAction> = of({type: 'Action1'});

But other than that, there shouldn't be much impact. This issue only affected 30 out of ~100k targets within Google

[cartant]

I believe this PR is ready to go.

Given that TS doesn't behave particularly nicely with rest parameters and literals, I'm not convinced that we should strive to remove the overload signatures for of. Particularly, as there is no scheduler-related shenanigans here. I think a handful of non-deprecated, fixed-argument overloads followed by a rest-parameter overload to catch everything else would be a better way to go.

E.g.

export function of(): Observable<never>;
export function of<T>(value: T): Observable<T>;
export function of<T1, T2>(value1: T1, value2: T2): Observable<T1 | T2>;
// maybe up to T3?
export function of<A extends Array<any>>(...args: A): Observable<ValueFromArray<A>>;

This seems a small price to pay to get it to work properly in 95% of cases - and there are only a few RxJS functions that take values like this. IMO, the TS behaviour is not going to change anytime soon.

@benlesh

[kolodny]

I'm OK with that, however I'd suggest we get creative with overloads to make the deprecated usage of of<number>(1) properly warn the user by doing something like this:

declare function of<_T, T extends _T>(t: T): 'OK';
declare function of<T>(t: T): 'deprecated';

const ok = of(1);
ok; // 'OK'

const deprecated = of<number>(1);
deprecated; // 'deprecated'

Thoughts?

[cartant]

Thoughts?

I'd prefer to leave it to a lint rule. Shenanigans with type parameters that don't directly relate to actual parameters makes me a little nervous. And the deprecation would be something that you'd want for all signatures except for the rest parameter one, so I guess that would involve double the number of type parameters for each of the non-rest-parameter signatures.

[kolodny]

Done, we should probably have a story around moving folks away from of<number>(1) though, but that's out of scope for this PR

[cartant]

... we should probably have a story around moving folks away from of<number>(1)

Sure. I have an item in my TODO list - from a discussion with @jwo719 - about creating an issue to deal with brining the RxJS linting rules into this repo. I'll make a note that another issue - relating to recommended rules, etc. - should reference this issue.