fix(scan/reduce): Typings correct for mixed seed/value types #4858
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
Member
benlesh
commented
Jun 12, 2019
benlesh
commented
- Adds dtslint tests to cover various mixtures of seeds, accumulator results, and value types
- Refactors scan a little bit, as types needed to be updated in the implementation
- Resolves a performance issue where scan was calling next on the destination Subscriber when it had already errored
- Adds dtslint tests to cover various mixtures of seeds, accumulator results, and value types - Refactors scan a little bit, as types needed to be updated in the implementation - Resolves a performance issue where scan was calling next on the destination Subscriber when it had already errored
benlesh
force-pushed
the
fix_reduce_types
branch
from
09876a7 to
7bc49f6
Compare
kwonoj
reviewed
Jun 12, 2019
| * The accumulator function called on each source value. | ||
| * @param {T|R} [seed] The initial accumulation value. | ||
| * @return {Observable<R>} An observable of the accumulated values. | ||
| * @param {V|A} [seed] The initial accumulation value. |
There was a problem hiding this comment.
https://github.com/ReactiveX/rxjs/pull/4858/files#diff-bcface33933d00ae116bcf19e16b2b7cR55 specifies seed as S but it doesn't extends V|A as comment says - maybe this needs to be S?
There was a problem hiding this comment.
uh welp maybe above overloading does its job
There was a problem hiding this comment.
Yeah. These docs need an overhaul, probably.
kwonoj
approved these changes
Jun 12, 2019
cartant
approved these changes
Jun 12, 2019
| * result of accumulating the values emitted by the source Observable. | ||
| * @method reduce | ||
| * @owner Observable | ||
| */ | ||
| export function reduce<T, R>(accumulator: (acc: T | R, value: T, index?: number) => T | R, seed?: T | R): OperatorFunction<T, T | R> { | ||
| export function reduce<V, A>(accumulator: (acc: V | A, value: V, index?: number) => A, seed?: any): OperatorFunction<V, V | A> { |
There was a problem hiding this comment.
Does seed really need to be any? It seems to be S in the implementation signature for scan.
There was a problem hiding this comment.
Given the hoops you need to jump through in the implementation if it's S, I just made it any. This is the implementation, not the type signature.
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.