fix(scan): fixed declarations to properly support different return types #4598
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
david-driscoll
force-pushed
the
master
branch
4 times, most recently
from
bf45b63
to
b084779
Compare
The build is failing✨ Good work on this PR so far! ✨ Unfortunately, the Travis CI build is failing as of bf45b63. Here's the output:
|
Pull Request Test Coverage Report for Build 8405
💛 - Coveralls |
cartant
requested changes
Mar 16, 2019
compat/operator/reduce.ts
Outdated
| @@ -2,9 +2,8 @@ import { Observable } from 'rxjs'; | |||
| import { reduce as higherOrderReduce } from 'rxjs/operators'; | |||
|
|
|||
| /* tslint:disable:max-line-length */ | |||
| export function reduce<T, R>(this: Observable<T>, accumulator: (acc: R, value: T, index: number) => R, seed?: R): Observable<R>; | |||
There was a problem hiding this comment.
I don't think the seed can be optional if the accumulator has a different type the the source elements. If a seed is not specified, my understanding is that the first element will be used, instead. That means that the initial accumulator value will be a T and not an R as indicator by the call signature.
There was a problem hiding this comment.
You're correct... adding tests and updating.
benlesh
requested changes
Apr 23, 2019
| @@ -6,9 +6,9 @@ import { OperatorFunction, MonoTypeOperatorFunction } from '../types'; | |||
| import { pipe } from '../util/pipe'; | |||
|
|
|||
| /* tslint:disable:max-line-length */ | |||
| export function reduce<T, R>(accumulator: (acc: R, value: T, index: number) => R, seed: R): OperatorFunction<T, R>; | |||
There was a problem hiding this comment.
So there's this messed up scenario I think we need to test for:
of(1).pipe(reduce((acc, value, i) => i === 0 ? [] : [..value], { seed: 'LOL' })) The problem is that acc gets typed wrong.
I'm sure that scan has the same issue.
There was a problem hiding this comment.
So we assume that seed and ReturnType<typeof accumulator> are the same type, if they differ we get an error because accumulator returns an invalid value. Thoughts?
this solves an issue where if you would reduce over `T` you can get `T` out reliably, but you could not get out `R` in anyway without manually typing the reduce method call
|
Would love to see this merged as it is creating quite the nuisance while migrating from |
benlesh
approved these changes
May 10, 2019
BioPhoton
pushed a commit
to BioPhoton/rxjs
that referenced
this pull request
May 15, 2019
…pes (ReactiveX#4598) this solves an issue where if you would reduce over `T` you can get `T` out reliably, but you could not get out `R` in anyway without manually typing the reduce method call
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description:
This solves an issue where if you would reduce over
Tyou can getTout reliably, but you couldnot get out
Rin anyway without manually typing the reduce method callRelated issue (if exists):
#4086
cc @cartant