chore(typings): reorder project overloads

[cartant]

Description:

Most of the TypeScript overloads for operators that take a projection function place the overloads that include projection functions before the overloads that do not. That's the way it should be.

However, there are two that do not:

• the static combineLatest operator; and
• the static zip operator.

The problem is that a projection function can be matched against ObservableInput<any>, as any function will match ArrayLike<any>. That means the overloads that include projection functions are more specific and should be placed first. If they are not, this:

const ob = Observable.combineLatest<any, any>(
    Observable.of(1),
    (value: any) => value
);

will match this:

export function combineLatest<T, T2>(v1: ObservableInput<T>, v2: ObservableInput<T2>, scheduler?: IScheduler): Observable<[T, T2]>;

instead of this:

export function combineLatest<T, R>(v1: ObservableInput<T>, project: (v1: T) => R, scheduler?: IScheduler): Observable<R>;

and the incorrect type will be inferred for the result.

Related issue (if exists): None.

[rxjs-bot]

	Messages
📖

CJS: 1342.5KB, global: 737.3KB (gzipped: 137.9KB), min: 144.6KB (gzipped: 30.7KB)

Generated by 🚫 dangerJS

[kwonoj]

This looks harmless but curious if there's way to test there are no regressions. 🤔

[cartant]

It should be harmless. If there is code that's impacted, the change in this PR should do nothing other than make explicit type assertions redundant.

For example, without the PR, this code:

const ob = Observable.combineLatest<any, any>(
    Observable.of(1),
    (value: any) => value
);

Would see the type of ob inferred as Observable<[any, any]>, which is clearly wrong. For the types to make sense, an explicit assertion would have been required:

const ob: Observable<any> = Observable.combineLatest<any, any>(
    Observable.of(1),
    (value: any) => value
);

With the change, the assertion would be redundant.

A lot of this TypeScript stuff devolves into thought experiments. It's hard to test.

I have some ideas about how some test infrastructure could be added to test for TypeScript errors that should be effected, but I've been side-tracked by a particular TypeScript issue. The fact that a function will match ArrayLike<any> has become a significant pain. (It has killed PR #2860.) TypeScript's consideration of index signatures when matching seems less than consistent. I'm going to create an issue in the TypeScript repo to at least ascertain whether or not it's a TypeScript bug.

[kwonoj]

yeah, as you mentioned most of type validation is based on 'thought process', reason why for me it looks harmless but thinking about sort of test cases. We alreaddy had several cases of multple reviewer thought type doesn't have regression and once it's released found out actual cases we couldn't think of. Especially operator like this have number of overload with specific inference fallback, I'd like to have some sort of guards for these kinds of chnges. just not sure how we can achieve it. we have some level of type definition testing but it is very minimal at this moment.

[cartant]

Yeah, I understand that it's disconcerting.

I will give it some more thought once I've got more information about the function-matching-ArrayLike<any> thing. BTW, if that is a bug and does get fixed, the fixing of that bug would have the same affect on existing code as this PR.

We'll see, I guess.

[kwonoj]

👍

[cartant]

@kwonoj I've written tool - ts-snippet - to compile TypeScript snippets for testing purposes and now I'm wondering what your policy is on adding dev dependencies to this project.

The way I see it:

1. ts-snippet could be added as a dev dependency with a semver range, allowing it be updated eternally.
2. ts-snippet could be added as a dev dependency with a specific semver to prevent it being updated externally.
3. The required source could be pretty much added as-is, with its MIT licence included in the source.
4. I could gut ts-snippet and add an cut-down version that's less general.

Note that option 4 will still require the tsutils MIT licence to be included in the source - I've had to use modified portions of that library, as it's only backwards compatible to TS 2.1.

My intention is to augment the implementation of the type function (which depends upon PR #2877 being merged) so that an RxJS-specific wrapper of the snippet function is passed to type callers that specify it as a parameter in the function passed by the caller. (If the parameter is not specified, type calls can be ignored - the current behaviour.) The RxJS-specific wrapper would accept the code for a single file and would prepend the RxJS import (to keep tests simple and DRY).

With the snippet compiler and expectations in place, I'll be able to test a fair bit of stuff that's currently untestable and will be able to start addressing some of the typing issues that I've found.

[kwonoj]

@cartant we don't have strong policies around devdependencies as long as licenses are allowed, so I don't think that's big concern.

It's more interesting what that dependency actually those - in a high level, compare to current type based assertion what it does resolves to test complex overloads we have?

[cartant]

To clarify, there's nothing wrong with the current type tests. They are useful, but they don't - and can't - test everything. I'm not advocating their immediate wholesale replacement. Rather, I'd like to extend the type function so that compiled snippets can be used where appropriate.

The case for testing with snippets is best put using some examples.

In #2891, I've added tests like this:

type('should infer the type with a type-changing selector', () => {
  /* tslint:disable:no-unused-variable */
  const source = Rx.Observable.of<number>(1, 2, 3);
  const result: Rx.Observable<string> = source.multicast(() => new Rx.Subject<number>(), s => s.map(x => x + '!'));
  /* tslint:enable:no-unused-variable */
});

Tests like these attempt verify that the inferred type is correct. For the most part, they work. However, they don't guard against a reversion in which the return type is inferred as, say, Observable<{}>.

If a snippet were to be used instead, such reversions would be detected:

type('should infer the type with a selector', (snippet) => {
  const s = snippet(`
    const source = Rx.Observable.of<number>(1, 2, 3);
    const result = source.multicast(() => new Rx.Subject<number>(), s => s.map(x => x + '!'));
  `);
  s.toInfer("result", "Observable<string>")
});

Snippets are also useful in issues like the one this PR addresses, as the non-snippet type tests end up testing nothing when any is involved. (And there are a few such problems due to functions being deemed compatible with Array<any> and, therefore, ObservableInput<any>.)

That is, this would test next to nothing:

type('should infer the correct type', () => {
  /* tslint:disable:no-unused-variable */
  const result: Rx.Observable<any> = Rx.Observable.combineLatest<any, any>(
    Rx.Observable.of(1),
    (value: any) => value
  );
  /* tslint:disable:no-unused-variable */
});

Whereas, this is more useful and would not pass with the current codebase (as the overload signatures need to be re-ordered):

type('should infer the correct type', (snippet) => {
  const s = snippet(`
    const result = Rx.Observable.combineLatest<any, any>(
      Rx.Observable.of(1),
      (value: any) => value
    );
  `);
  s.toInfer("result", "Observable<any>");
});

To take another example, there is a problem with the overload signatures for the subscribe method. You might be surprised to hear that, with the current signatures, this is okay:

const input: Rx.ObservableInput<string> = Rx.Observable.of<number>(1);

At the moment, this test would not pass:

type('should not be assignable to observables of incompatible types', (snippet) => {
  const s = snippet(`
    const input: Rx.ObservableInput<string> = Rx.Observable.of<number>(1);
  `);
  s.toFail(/not assignable/);
});

So, if the subscribe signatures are fixed in a future PR, the fix could be accompanied with the above test to guard against reversion. Without compiling snippets, this sort of thing is not possible - as the test is looking for a compilation failure/error.

The changes to the type function would look something like this:

import * as tss from 'ts-snippet';

context.type = function (title: string, fn: (snippet?: (code: string) => tss.Expect) => void): void {
  if (fn && fn.length === 1) {
    function snippet(code: string): tss.Expect {
      const s = tss.snippet({
        'snippet.ts': `
          import * as Rx from './dist/cjs/Rx';
          ${code}
        `
      });
      return s.expect('snippet.ts');
    }
    fn(snippet);
  } else {
    //intentionally does not execute to avoid unexpected side effect occurs by subscription,
    //or infinite source. Suffecient to check build time only.
  }
};

The actual implementation would be a little more complicated, as I'd like to reuse the compiler - to avoid reading and parsing the Rx import with each snippet test. Regarding performance, the snippet tests for inferred types are fast; it's the tests that look for syntactic or semantic errors that are slower - some hundreds of milliseconds for those (on my slow, old laptop).

[cartant]

@kwonoj These are exactly the sorts of reversions that cannot be caught with the current type tests, as Observable<{}> will be compatible with whatever observable type the test assigns to.

[benlesh]

as any function will match ArrayLike

🤦‍♂️

[cartant]

@benlesh Yeah, the function-matching-ArrayLike<any> thing is somewhat surprising. FYI, the TypeScript issue is here.

[coveralls]

Coverage remained the same at ?% when pulling ce32690 on cartant:project-overloads into 2e576dc on ReactiveX:master.

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