Use unknown instead of any to catch more mistakes

[OliverJAsh]

Description:

I recently shipped a bug to production which TS should have caught. It looked something like this: https://stackblitz.com/edit/rxjs-jlgx8q?devtoolsheight=60

import { of } from "rxjs";
import { mergeMapTo } from "rxjs/operators";

const source = of(10);

const fn = () => {};
// Expected type error, but got none.
source.pipe(mergeMapTo(fn)).subscribe();

The reason TS doesn't catch this is because:

• mergeMapTo uses ObservableInput<any>
• ObservableInput<any> uses ArrayLike<any>
• TS has a "bug" whereby ArrayLike<any> is assignable to a function Functions are compatible with ArrayLike<any> microsoft/TypeScript#18757

We can workaround this by replacing ObservableInput<any> with ObservableInput<unknown>, as @cartant suggested on Twitter: https://twitter.com/ncjamieson/status/1318501262162239489.

I had a quick go at replacing all usages across the codebase, but I ended up with a lot of failing type tests. Perhaps we will only be able to switch to unknown in certain places like this. WDYT?

Related issue (if exists):

[cartant]

AFAICT, the problem with some of the other places is that there are some assertions that need to be added within the implementations. For example, in defer, the inferred type for what's returned by innerFrom is Observable<unknown> instead of Observable<any>:

rxjs/src/internal/observable/defer.ts

Line 54 in 8ef4fee

innerFrom(observableFactory()).subscribe(subscriber);

So an as Observable<ObservedValueOf<R> is needed before the subscribe call.

I'm happy to approve this if you want to mark it as ready for review. Other places can be changed in separate PRs.

[cartant]

LGTM. Thanks for the PR.

[OliverJAsh]

So an as Observable<ObservedValueOf<R> is needed before the subscribe call.

Alternatively, we could do this, so we don't need the assertion:

export function defer<E, R extends ObservableInput<E>>(observableFactory: () => R): Observable<E> {
  return new Observable<E>((subscriber) => {
    innerFrom(observableFactory()).subscribe(subscriber);
  });
}

… or:

export function defer<E>(observableFactory: () => ObservableInput<E>): Observable<E> {
  return new Observable<E>((subscriber) => {
    innerFrom(observableFactory()).subscribe(subscriber);
  });
}

WDYT?

[benlesh]

Okay, so the root of this is that () => {} passes for ArrayLike<any> because of length and the fact it can be indexed (even if the indices mean nothing). It's interesting to me that this is the "fix" here, because it certainly doesn't fix the larger problem.

For example:

from(() => {}) is invalid at runtime and still passes type checks.

However, I'm not okay with walking away from ArrayLike, as we've always been able to handle other things. arguments, NodeList, etc. I think the real fix here is to do something where we allow "ArrayLike" but somehow exclude functions? Otherwise we need to narrow ObservableInput to Array only, which sucks a little, but it's arguably nothing people aren't used to.

[cartant]

I think the real fix here is to do something where we allow "ArrayLike" but somehow exclude functions?

I disagree. There should be no reason why we cannot use unknown - the real top type - in the signatures instead of any. If that's done, the problem goes away.

[benlesh]

Oh. I'm not saying this isn't a valid fix. Just noticing the other problem.

[cartant]

Just noticing the other problem.

Yeah. I'm not sure about the solution(s) above for defer. I mean, it might be okay for defer, but the approach that uses the ObservedValueOf type is needed elsewhere. And I'm also very cautious when it comes to using extends constraints between type parameters. I'll have a look at it outside the scope of this PR, but, TBH, these parts of TS give me the heebie jeebies.

[OliverJAsh]

the approach that uses the ObservedValueOf type is needed elsewhere

Can you elaborate a little? If you could tell me the name of an operator that needs it, I can have a look and see what I come up with.

I'll have a look at it outside the scope of this PR

I'm happy to help! I would like to get this issue fixed for all other operators, but I'm a bit uncomfortable sending a PR which adds a lot of type assertions. It would be good if we could find a way to avoid the need for them.

[cartant]

the approach that uses the ObservedValueOf type is needed elsewhere

The issue is that ObservableInput needs to be used as an element type for tuples, like this:

export declare function merge<Sources extends readonly ObservableInput<unknown>[]>(...sources: Sources): Observable<ObservedValueUnionFromArray<Sources>>;

in this PR. AFAICT, there is no possibility of splitting out the element type, as done in the defer example. And, TBH, I am not sure about the first of the two defer examples. And I can't recall why we switched the second of the two defer examples - I'm pretty sure it would have originally looked like that.

I mentioned to Ben that I was planning on coming back to this once the N-args business is done.

... I'm a bit uncomfortable sending a PR which adds a lot of type assertions.

TBH, ATM, I'm more comfortable with the type assertion that I mentioned above than the between-two-type-parameters-constraint that's in the first of the two defer examples above. If you could point to any 'official' TypeScript typings that use this approach - or guidance from the TS team - I'd be happy to change my mind, but I have bad memories of things getting messed up with 'tricky' type parameter usage.

There is a very good changes that there are a whole bunch of operators that can have any constraints replaced with unknown and I'd be happy to approve those individually (or in small groups). I'm sure they'd be merged promptly, too.

[cartant]

Regarding defer, it used to support factories which returned void - which made no sense:

rxjs/src/internal/observable/defer.ts

Lines 55 to 67 in a0bc359

	export function defer<R extends ObservableInput<any> | void>(observableFactory: () => R): Observable<ObservedValueOf<R>> {
	return new Observable<ObservedValueOf<R>>(subscriber => {
	let input: R | void;
	try {
	input = observableFactory();
	} catch (err) {
	subscriber.error(err);
	return undefined;
	}
	const source = input ? from(input as ObservableInput<ObservedValueOf<R>>) : EMPTY;
	return source.subscribe(subscriber);
	});
	}

(That types in that implementation are ... 🙈)

It's now a straightforward ObservableInput-returning factory, so we should be able to use the second of your suggestions:

rxjs/src/internal/observable/defer.ts

Line 52 in 2337342

export function defer<R extends ObservableInput<any>>(observableFactory: () => R): Observable<ObservedValueOf<R>> {

If you want to make that change, that would be fine by me.

[OliverJAsh]

there is no possibility of splitting out the element type, as done in the defer example

Hmm, we might actually be able to do this. How about this pseudo code?

type Observable<T> = { t: T };

declare function merge<Sources extends readonly unknown[]>(
    ...sources: { [K in keyof Sources]: Observable<Sources[K]> }
): Observable<Sources[number]>;

declare const ob1: Observable<number>;
declare const ob2: Observable<string>;
// Observable<string | number>
const result = merge(ob1, ob2);

[cartant]

Nice! It works with variadic tuples, too

[OliverJAsh]

Where does this leave us then? Maybe we could update all operators to do something like my 2nd defer example?

export function defer<E>(observableFactory: () => ObservableInput<E>): Observable<E> {
  return new Observable<E>((subscriber) => {
    innerFrom(observableFactory()).subscribe(subscriber);
  });
}

[cartant]

Where does this leave us then?

What we need to do is:

1. Figure out whether or not defer can be simplified and if it can, we should address any other operators that don't involve multiple inputs (i.e. tuples). There are a whole bunch. Searching I find catchError, concatMap, etc.

However, I recall now that one of the reasons ObservedValueOf was introduced was to handle situations like this:

rxjs/spec-dtslint/observables/defer-spec.ts

Lines 15 to 17 in 1e6092e

	it('should support union type returns', () => {
	const a = defer(() => Math.random() > 0.5 ? of(123) : of('abc')); // $ExpectType Observable<string | number>
	});

It's entirely possible that we won't be able to replace it in all situations. Or maybe even at all.

2. Regarding multiple inputs (i.e. tuples) I've asked Ben to take a look, as it relates to his open PR.

[cartant]

FWIW, I've just noticed/remembered that the mechanism you've pointed out was introduced recently. See combineLatest, which uses ObservableInputTuple instead of ObservedValueOf. So the types declarations for observables and operators that use multiple sources should eventually be converted to use the former instead of relying upon the latter.

IMO, we will still need ObservedValueOf for functions that could return Observable unions that need be converted to an Observable with a union element type, as mentioned above.