fix(scan-operator): Remove redundant function overload types for .scan

[hsubra89]

This PR removes redundant function overloads for the .scan operator. It also fixes the type inference when dealing with a source observable representing union types. i.e Consider the following =>

const mergedObservable = Observable.merge(
  Observable.of({ loading: true }),
  Observable.of({ loading: false, data: ['hello'] })
)
.scan((acc, e) => Object.assign({}, acc, e), { loading: false, data: [] as string[] })

// typeof mergedObservable
// { loading: boolean } | { loading: boolean, data: string[] }

// typeof mergedObservable after this change.
// This is the correct return type for the above code. 
// { loading: boolean, data: string[] }

Current Inferred type

Inferred type with the changes from this PR.

[rxjs-bot]

	Messages
📖

CJS: 1338.3KB, global: 737.9KB (gzipped: 138.2KB), min: 145.3KB (gzipped: 30.9KB)

Generated by 🚫 dangerJS

[coveralls]

Coverage remained the same at 97.466% when pulling d3a82ba on hsubra89:scan-function-overload-type into fb3694d on ReactiveX:master.

[kwonoj]

We intentionally introduced those specific signature via #1847 - I'm adding @david-driscoll to review this.

[benlesh]

I tend to agree with @hsubra89, it does look redundant. Perhaps it just helps less robust type inference in TS 2.0? Maybe we could remove this once we get to v6 and we're targetting TS 2.5+?

[hsubra89]

@benlesh Those example screenshots I posted above was using Rxjs's typescript version which is 2.0.10. I'm not really sure about this, but if the type inference worked with one generic parameter in the first place, wouldn't it continue to work in this scenario?

[kwonoj]

I don't recall legacy history but that looks legit explanation. What about target this into next branch right away for v6?

[hsubra89]

Actually, i've just noticed that there are a few incorrect tests in scan-spec.ts. I'll raise another PR that changes them first. Fixing those tests actually makes it obvious why these changes were made in the first place.

[hsubra89]

PR that fixes tests in scan-spec.ts #2902

[cartant]

This PR's removal of the first two overload signatures creates a problem - albeit one that those overloads don't specifically address.

With only the last signature, if the optional seed is not specified, R will be inferred as {} - which is not ideal.

The problem can be fixed by adding a single additional signature to the change proposed in this PR and by making the seed parameter in the second signature non-optional (technically, this is not required, as if seed is not specified, the first signature will be matched, but I think it's clearer if it's made no-longer-optional).

 /* tslint:disable:max-line-length */
 export function scan<T>(this: Observable<T>, accumulator: (acc: T, value: T, index: number) => T): Observable<T>;
 export function scan<T, R>(this: Observable<T>, accumulator: (acc: R, value: T, index: number) => R, seed: R): Observable<R>;
 /* tslint:enable:max-line-length */

That is, if there is no seed, the accumulator and resultant observable must be of type T.

Also, a similar PR is required for reduce.

[hsubra89]

@benlesh @kwonoj Now that the tests PR has been merged, I attempted to reconcile the changes in this PR with those tests and what they represent. We can't really achieve that using TS 2.0. We wouldn't need any of these overloads for more recent versions of TS. In light of that, Should we close this PR and create a new PR that targets a more recent version of TS?

[hsubra89]

@cartant Yep, reduce needs to be fixed as well. Happy to raise a PR to fix reduce after we decide on how we're going to approach .scan.

Making these changes from this PR or the ones that @cartant suggested would break current codebases relying on the behaviour where both the seed and the value represent type T ( Although, i don't imagine that's incredibly useful for most kinds of operations that .scan would typically be used for ).

If we're happy to push this into a minor release where there could potentially be breaking changes to current users of .scan relying on this behaviour, we can implement the changes defined here and roll with it.

[cartant]

@hsubra89 I'm wrong: the first overload is required as-is. Otherwise, code like this:

const reduced = a.reduce<{ a?: number; b?: string }>((acc, value) => {
  value.a = acc.a;
  ... 
  return acc;
}, {});

will only match the overload with R in it and R will be inferred as {} (the type, not the literal) instead of { a?: number; b?: string }.

Interestingly, if only the second array-related overload is removed, everything is fine (with TypeScript 2.0.10). So it seems that the second overload is the redundant one.

[hsubra89]

@cartant We can't really delete the array related one either :

let a: Rx.Observable<{ a: number; b: string }>;
a.scan((acc, value) => acc.concat(value), []);

In the example above, with the array related overload, the type of acc is { a: number; b: string }[] whereas without it, it just becomes any.

IMHO, the current implementation is wrong, but turning it into an any might probably be worse. The best solution would be to just delete all the overloads and force users to explicitly type uses of .scan.

There are much more elegant ways to handle this in more recent versions of TS. i.e, we could do something like this to allow it.

Untested

export function scan<T, R = T>(this: Observable<T>, accumulator: (acc: R, value: T, index: number) => R, seed?: T | R): Observable<R> { ...

[cartant]

@hsubra89 Yeah, you are right. I've been having another look at this, this afternoon. Unfortunately, I can't seem to get it out of my head. Perhaps I should just go for a walk?

And, yes, the signature that uses the more up-to-date TypeScript functionality is better.

I've been doing some tinkering with a simple harness and it seems that TypeScript 2.0 and 2.5 behave in radically different ways depending upon the presence of the second overload. It needs to be there for 2.0 and needs to be removed for 2.5.

I think a signature more like that proposed in your most recent comment would be the best option, if the TypeScript dependency can be bumped to a sufficiently recent version when the next major version of RxJS is released.

Anyway, maybe now I can start thinking about something else.

[benlesh]

It seems like we should add signatures with the required seed as @cartant suggested, and one explicitly for the no-seed case.

scan<T, R>(this: Observable<T>, accumulator: (acc: R, value: T, index: number) => R): Observable<R>
scan<T, R>(this: Observable<T>, accumulator: (acc: R, value: T, index: number) => R, seed: R): Observable<R>

Because in one, the type should be inferred from the return value, and in the other, it's inferred from the seed. I'm not sure we care about the folks that are going to alter the type at each iteration through, we can assume they're just going to type things with any and YOLO their way through it.

Thoughts, @kwonoj? @david-driscoll?

[cartant]

@benlesh Unfortunately, with the first signature R will be inferred as {}. That is, using:

const source = Rx.Observable.of(1, 2, 3);
const scanned: source.scan((acc, value) => value.toString());

The inferred type of scanned will be Observable<{}>.

Only if the type is specified for the acc parameter, will scanned be inferred as Obserable<string>:

const source = Rx.Observable.of(1, 2, 3);
const scanned: source.scan((acc: string, value) => value.toString());

(This is using TypeScript 2.5.3.)

[hsubra89]

@kwonoj @benlesh So what should we do with this?

[kwonoj]

I can't be sure I can reliably guarantee this PR works without breaking regressions in 5. Personally would like hold off and revisit in v6.

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