-
Notifications
You must be signed in to change notification settings - Fork 3k
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
Comments
pinging our resident typings police @kwonoj @david-driscoll from the StackOverflow answer:
|
So this is dupe to #2339? |
Yah, the gentleman who answered my q also put up a report it seems. Close whichever is fit :) |
@rohitsodhia, @kwonoj I apologize for the duplicate. I wanted to make sure it got reported. |
@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? |
@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 Also note that the overload taking a seed of type |
* 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.
@aluanhaddad Hello, sorry for bothering you but could you give me a small explanation. Currently code snipped like:
produces the same error: Is it expected? By discussion I expected that after declarations rearranging it should go away. Moreover, test case |
That test was against an old TypeScript 2.2 IIRC. Also I think the signature of |
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. |
RxJS version:
5.1.0
Code to reproduce:
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.
The text was updated successfully, but these errors were encountered: