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

fix(scan): fixed declarations to properly support different return types #4598

Merged
merged 1 commit into from May 10, 2019

Conversation

@david-driscoll
Copy link
Collaborator

commented Feb 26, 2019

Description:
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

Related issue (if exists):
#4086

cc @cartant

@david-driscoll david-driscoll requested review from benlesh and cartant Feb 26, 2019
@david-driscoll david-driscoll force-pushed the david-driscoll:master branch 4 times, most recently from bf45b63 to b084779 Feb 26, 2019
@ci-reporter

This comment has been minimized.

Copy link

commented Feb 26, 2019

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:

npm test
> @reactivex/rxjs@6.4.0 test /home/travis/build/ReactiveX/rxjs
> cross-env TS_NODE_PROJECT=spec/tsconfig.json mocha --opts spec/support/default.opts "spec/**/*-spec.ts"



  ․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․
  ․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․
  ․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․
  ․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․
  ․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․
  ․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․
  ․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․
  ․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․
  ․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․
  ․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․
  ․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․
  ․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․
  ․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․
  ․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․
  ․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․
  ․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․
  ․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․
  ․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․
  ․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․
  ․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․
  ․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․
  ․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․
  ․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․
  ․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․
  ․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․
  ․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․
  ․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․
  ․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․!․․․․․․․․․․․․․․․․
  ․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․,,,․․․,․․․․․․․․․․․․․․․․
  ․․․․․․․․․․․․․․․���․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․
  ․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․
  ․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․
  ․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․
  ․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․
  ․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․
  ․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․
  ․․․․․․���․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․
  ․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․
  ․․․․․․․․․․․․․․․․․․․․․․․���․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․
  ․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․
  ․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․
  ․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․
  ․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․
  ․․․․․․․․․․․․․․․․․․․․․․․․․․․․․

  2604 passing (4s)
  4 pending
  1 failing

  1) reduce operator
       should reduce with index without seed:

      Uncaught AssertionError: expected 1 to equal 0
      + expected - actual

      -1
      +0
      
      at ScanSubscriber.accumulator (spec/operators/reduce-spec.ts:75:30)
      at ScanSubscriber._tryNext (src/internal/operators/scan.ts:114:21)
      at ScanSubscriber._next (src/internal/operators/scan.ts:106:19)
      at ScanSubscriber.Subscriber.next (src/internal/Subscriber.ts:99:12)
      at Observable._subscribe (src/internal/observable/range.ts:60:20)
      at Observable._trySubscribe (src/internal/Observable.ts:231:19)
      at Observable.subscribe (src/internal/Observable.ts:212:14)
      at ScanOperator.call (src/internal/operators/scan.ts:75:19)
      at Observable.subscribe (src/internal/Observable.ts:207:25)
      at TakeLastOperator.call (src/internal/operators/takeLast.ts:68:19)
      at Observable.subscribe (src/internal/Observable.ts:207:25)
      at Context.<anonymous> (spec/operators/reduce-spec.ts:77:9)

I'm sure you can fix it! If you need help, don't hesitate to ask a maintainer of the project!


This comment was automagically generated by ci-reporter. If you see a problem, open an issue here.
@coveralls

This comment has been minimized.

Copy link

commented Feb 26, 2019

Pull Request Test Coverage Report for Build 8405

  • 4 of 4 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 96.856%

Totals Coverage Status
Change from base Build 8397: 0.0%
Covered Lines: 5792
Relevant Lines: 5980

💛 - Coveralls
@@ -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>;

This comment has been minimized.

Copy link
@cartant

cartant Mar 16, 2019

Collaborator

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.

This comment has been minimized.

Copy link
@david-driscoll

david-driscoll Apr 23, 2019

Author Collaborator

You're correct... adding tests and updating.

@@ -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>;

This comment has been minimized.

Copy link
@benlesh

benlesh Apr 23, 2019

Member

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.

This comment has been minimized.

Copy link
@david-driscoll

david-driscoll Apr 23, 2019

Author Collaborator

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
@cah-dunn

This comment has been minimized.

Copy link

commented May 9, 2019

Would love to see this merged as it is creating quite the nuisance while migrating from rxjs-compat.

@benlesh benlesh merged commit 126d2b6 into ReactiveX:master May 10, 2019
3 checks passed
3 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage remained the same at 96.856%
Details
BioPhoton added 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
@lock lock bot locked as resolved and limited conversation to collaborators Jun 9, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
5 participants
You can’t perform that action at this time.