Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Reduce logs as array, but erroring as object #2338

Closed
rohitsodhia opened this issue Feb 7, 2017 · 9 comments · Fixed by #2382
Closed

Reduce logs as array, but erroring as object #2338

rohitsodhia opened this issue Feb 7, 2017 · 9 comments · Fixed by #2382
Labels
TS Issues and PRs related purely to TypeScript issues

Comments

@rohitsodhia
Copy link

rohitsodhia commented Feb 7, 2017

RxJS version:
5.1.0

Code to reproduce:

this.searchSubscription = this.filtersSubject
  .takeLast(2)
  .reduce((acc, cur) => {
    console.log(acc);
    acc.push(cur);
    return acc;
  }, [])
  .debounceTime(1000)
  .subscribe(x => this.emitFilters());

Error:
Code fails to compile because error claiming acc is an object (thus, no push method), even though the console.log shows acc to be an array.

Additional information:
I was told by this StackOverflow answer that this is a bug with RxJS? If I'm wrong, please ignore.

@trxcllnt
Copy link
Member

trxcllnt commented Feb 7, 2017

pinging our resident typings police @kwonoj @david-driscoll

from the StackOverflow answer:

Your code is correct but there is a bug in rxjs5 that is preventing it from type checking.

Looking at the source code of reduce we see the following signature

export function reduce<T>(this: Observable<T>, accumulator: (acc: T, value: T, index: number) => T, seed?: T): Observable<T>;

export function reduce<T>(this: Observable<T>, accumulator: (acc: T[], value: T, index: number) => T[], seed?: T[]): Observable<T[]>;

export function reduce<T, R>(this: Observable<T>, accumulator: (acc: R, value: T, index: number) => R, seed?: R): Observable<R>;

When writing overloads in TypeScript it is necessary to order them from greatest to least specificity with respect to their types including their type arguments. Is it because TypeScript intentionally picks the first matching overload. The last declaration of the overload set actually needs to come first.

@kwonoj
Copy link
Member

kwonoj commented Feb 7, 2017

So this is dupe to #2339?

@rohitsodhia
Copy link
Author

Yah, the gentleman who answered my q also put up a report it seems. Close whichever is fit :)

@kwonoj kwonoj added the TS Issues and PRs related purely to TypeScript issues label Feb 7, 2017
@aluanhaddad
Copy link
Contributor

@rohitsodhia, @kwonoj I apologize for the duplicate. I wanted to make sure it got reported.

@jayphelps
Copy link
Member

jayphelps commented Feb 11, 2017

@aluanhaddad @rohitsodhia I'm not able to reproduce this: Playground

Is there something else besides just using reduce that is triggering the incorrect override to be selected?

@aluanhaddad
Copy link
Contributor

aluanhaddad commented Feb 12, 2017

@jayphelps Here is a reproduction: Playground.

For reference, here is the triggering snippet

class Observable<T> {
  reduce<T>(this: Observable<T>, accumulator: (acc: T, value: T, index: number) => T, seed?: T): Observable<T>;
  reduce<T>(this: Observable<T>, accumulator: (acc: T[], value: T, index: number) => T[], seed?: T[]): Observable<T[]>;
  reduce<T, R>(this: Observable<T>, accumulator: (acc: R, value: T, index: number) => R, seed?: R): Observable<R>;
  reduce<T, R>(this: Observable<T>, accumulator: (acc: R, value: T, index?: number) => R, seed?: R): Observable<R> { return new Observable();}
}

let o: Observable<number>;
o.reduce((acc, cur) => {
  console.log(acc);
  acc.push(cur.toString()); // error argument of type 'string' is not assignable to parameter of type 'number';
  return acc;
}, []);

Correcting the order of the overloads fixes this, giving acc the inferred type any[] which matches the behavior of the declarations for Array.prototype.reduce in lib.d.ts. While any is not desirable, the compiler cannot infer better without a type annotation, but the current declarations, by introducing an additional type parameter, clearly want to allow the seed to be of a different type (reduce is pretty limited if it cannot be).

Also note that the overload taking a seed of type T[] is unnecessary as its usecase is fully covered when the overload taking type arguments T and R is actually used which it currently never is.
Edit:
While it is indeed unnecessary, it does provide slightly improved type inference for the case when seed is T[] for some Observable<T>, and does not harm.

benlesh pushed a commit that referenced this issue Apr 3, 2017
* fix(reduce): adjust overload ordering

Fixes #2338

* Make seed required when not of type T

* Added specs for new and old behavior of reduce overloads and fixed failing specs

This adds tests for both existing behavior, under the old overload, and the new behavior enabled by the new overload signatures of the `reduce` operator.
Not that the specific change to the following test case `'should accept T typed reducers'` that removes the seed value. If a seed value is specified, then the reduction is always from `T` to `R`. This was necessary to make the test pass, but also models the expected behavior more correctly.

The cases for `R` where `R` is not assignable to `T` are covered by new tests added in the commit.

Additionally, I have added additional verification to all of the tests touched by this change to ensure that the values returned are usable as their respective expected types.

* Fix ordering as allowed by newly required seeds to fix failing specs

Fix ordering as allowed by newly required seeds to fix failing tests.
I think there is a good argument to be made that the failing tests were failing correctly, as this is how `Array.prototype.reduce` behaves, but as I have made the seed arguments required, for `R` typed reducers, re-ordering the overloads impacts their specificity allowing, the current behavior, correct or otherwise, to be retained.
@WiseBird
Copy link

WiseBird commented Feb 22, 2018

@aluanhaddad Hello, sorry for bothering you but could you give me a small explanation. Currently code snipped like:

import {Observable} from 'rxjs';

let o: Observable<number>;
o.reduce((acc, cur) => {
    console.log(acc);
    acc.push(cur.toString());
    return acc;
}, []);

produces the same error: TS2345: Argument of type 'string' is not assignable to parameter of type 'number'.

Is it expected? By discussion I expected that after declarations rearranging it should go away.

Moreover, test case should accept R typed reducers when R is assignable to T added in the PR causes multiple TS2339: Property 'a' does not exist on type '{}' errors.

@aluanhaddad
Copy link
Contributor

That test was against an old TypeScript 2.2 IIRC. Also I think the signature of Array.prototype.push has been revised substantially and that these tests need to be looked over.

@lock
Copy link

lock bot commented Jun 6, 2018

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.

@lock lock bot locked as resolved and limited conversation to collaborators Jun 6, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
TS Issues and PRs related purely to TypeScript issues
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants