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.

Sign up for GitHub

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

Jump to bottom

fix(reduce/scan): both scan/reduce operators now accepts undefined itself as a valid seed #2050

Merged
merged 4 commits into from Oct 24, 2016
Merged

fix(reduce/scan): both scan/reduce operators now accepts undefined itself as a valid seed #2050

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
d34e49b
fix(scan): scan operator now accepts `undefined` itself as a valid se…
jayphelps Oct 18, 2016
34e2e4f
fix(reduce): reduce operator now accepts `undefined` itself as a vali…
jayphelps Oct 19, 2016
0827628
Merge branch 'master' into scan-seed
jayphelps Oct 24, 2016
0df3dc9
Merge branch 'master' into scan-seed
jayphelps Oct 24, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
30 changes: 30 additions & 0 deletions spec/operators/reduce-spec.ts
View file
Open in desktop
Expand Up @@ -35,6 +35,36 @@ describe('Observable.prototype.reduce', () => {
expectSubscriptions(e1.subscriptions).toBe(e1subs);
});

it('should reduce with a seed of undefined', () => {
const e1 = hot('--a--^--b--c--d--e--f--g--|');
const e1subs = '^ !';
const expected = '---------------------(x|)';

const values = {
x: 'undefined b c d e f g'
};

const source = e1.reduce((acc: any, x: string) => acc + ' ' + x, undefined);

expectObservable(source).toBe(expected, values);
expectSubscriptions(e1.subscriptions).toBe(e1subs);
});

it('should reduce without a seed', () => {
const e1 = hot('--a--^--b--c--d--e--f--g--|');
const e1subs = '^ !';
const expected = '---------------------(x|)';

const values = {
x: 'b c d e f g'
};

const source = e1.reduce((acc: any, x: string) => acc + ' ' + x);

expectObservable(source).toBe(expected, values);
expectSubscriptions(e1.subscriptions).toBe(e1subs);
});

it('should reduce with seed if source is empty', () => {
const e1 = hot('--a--^-------|');
const e1subs = '^ !';
Expand Down
20 changes: 20 additions & 0 deletions spec/operators/scan-spec.ts
View file
Open in desktop
Expand Up @@ -43,6 +43,26 @@ describe('Observable.prototype.scan', () => {
expectSubscriptions(e1.subscriptions).toBe(e1subs);
});

it('should scan with a seed of undefined', () => {
const e1 = hot('--a--^--b--c--d--e--f--g--|');
const e1subs = '^ !';
const expected = '---u--v--w--x--y--z--|';

const values = {
u: 'undefined b',
v: 'undefined b c',
w: 'undefined b c d',
x: 'undefined b c d e',
y: 'undefined b c d e f',
z: 'undefined b c d e f g'
};

const source = e1.scan((acc: any, x: string) => acc + ' ' + x, undefined);

expectObservable(source).toBe(expected, values);
expectSubscriptions(e1.subscriptions).toBe(e1subs);
});

it('should scan without seed', () => {
const e1 = hot('--a--^--b--c--d--|');
const e1subs = '^ !';
Expand Down
25 changes: 15 additions & 10 deletions src/operator/reduce.ts
View file
Open in desktop
Expand Up @@ -53,16 +53,24 @@ export function reduce<T>(this: Observable<T>, accumulator: (acc: T[], value: T,
export function reduce<T, R>(this: Observable<T>, accumulator: (acc: R, value: T, index: number) => R, seed?: R): Observable<R>;
/* tslint:disable:max-line-length */
export function reduce<T, R>(this: Observable<T>, accumulator: (acc: R, value: T) => R, seed?: R): Observable<R> {
return this.lift(new ReduceOperator(accumulator, seed));
let hasSeed = false;
// providing a seed of `undefined` *should* be valid and trigger
// hasSeed! so don't use `seed !== undefined` checks!
// For this reason, we have to check it here at the original call site
// otherwise inside Operator/Subscriber we won't know if `undefined`
// means they didn't provide anything or if they literally provided `undefined`
if (arguments.length >= 2) {
hasSeed = true;
}

return this.lift(new ReduceOperator(accumulator, seed, hasSeed));
}

export class ReduceOperator<T, R> implements Operator<T, R> {

constructor(private accumulator: (acc: R, value: T) => R, private seed?: R) {
}
constructor(private accumulator: (acc: R, value: T) => R, private seed?: R, private hasSeed: boolean = false) {}

call(subscriber: Subscriber<R>, source: any): any {
return source._subscribe(new ReduceSubscriber(subscriber, this.accumulator, this.seed));
return source._subscribe(new ReduceSubscriber(subscriber, this.accumulator, this.seed, this.hasSeed));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can totally see why you're passing hasSeed to the Subscriber via the constructor. It makes it less polymorphic. However, I do feel like the subscriber itself should be able to determine if it has a seed. How does that effect performance?

Do you even think it matters? I'm on the fence. It probably doesn't. It just feels weird as an API, albeit an internal one to be like seed, hasSeed on the tail end of a call.

Thoughts?

Copy link
Member Author

@jayphelps jayphelps Oct 19, 2016 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we would have to have additional branching, would we not?

if (this.hasSeed) {
  return source._subscribe(new ReduceSubscriber(subscriber, this.accumulator, this.seed));
} else {
  return source._subscribe(new ReduceSubscriber(subscriber, this.accumulator));
}

And then each class downstream would have to duplicate the arguments.length >= 2 check wouldn't they?

I did it this way because it seemed to me the best performant and the other solutions had a lot of duplication. It's very non-obvious at first, but best I can tell you have to atleast check at the original call site otherwise you'll lose that info because of the Operator#call architecture. The additional choices are whether or not you repeat the same arguments.length >= 2 check in the Operator and Subscriber constructors and branch each time, or you just decide once and pass down a totally awkward hasSeed from the top. I chose the later, since it's a private API but yeah, it's awkward.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jayphelps I suppose applying the ScanOperator constructor vis-a-vis new ScanOperator(...arguments) would allow the ScanOperator to determine whether the seed exists, and the only benefit I see there would be for people who manually import the ScanOperator to use with lift instead of the scan function, which I imagine is < 0.0001% of consumers.

Copy link
Member Author

@jayphelps jayphelps Oct 19, 2016 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@trxcllnt sorry, I'm not 100% sure what your suggestion is? Is it to move the arguments.length >= 2 check into the ScanOperator constructor and use new ScanOperator(...arguments)?

A little pedantic of me, but that will perform not as well because new ScanOperator(...arguments) transpiles to something like

new (Function.prototype.bind.apply(ScanOperator, [null].concat(Array.prototype.slice.call(arguments))))();

But this really isn't the typical hot code path, so prolly doesn't matter. I'm open to that, if you all have stronger opinions than me. It's definitely the prettiest solution of them all!

Copy link
Member

@trxcllnt trxcllnt Oct 19, 2016 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jayphelps no you're totally right and I should have clarified I was playing devil's advocate. I'm 100% against doing that.

}
}

Expand All @@ -72,18 +80,15 @@ export class ReduceOperator<T, R> implements Operator<T, R> {
* @extends {Ignored}
*/
export class ReduceSubscriber<T, R> extends Subscriber<T> {

acc: T | R;
hasSeed: boolean;
hasValue: boolean = false;

constructor(destination: Subscriber<R>,
private accumulator: (acc: R, value: T) => R,
seed?: R) {
seed: R,
private hasSeed: boolean) {
super(destination);
this.acc = seed;
this.accumulator = accumulator;
this.hasSeed = typeof seed !== 'undefined';
}

protected _next(value: T) {
Expand Down
27 changes: 16 additions & 11 deletions src/operator/scan.ts
View file
Open in desktop
Expand Up @@ -45,15 +45,24 @@ export function scan<T>(this: Observable<T>, accumulator: (acc: T[], value: T, i
export function scan<T, R>(this: Observable<T>, accumulator: (acc: R, value: T, index: number) => R, seed?: R): Observable<R>;
/* tslint:disable:max-line-length */
export function scan<T, R>(this: Observable<T>, accumulator: (acc: R, value: T, index: number) => R, seed?: T | R): Observable<R> {
return this.lift(new ScanOperator(accumulator, seed));
let hasSeed = false;
// providing a seed of `undefined` *should* be valid and trigger
// hasSeed! so don't use `seed !== undefined` checks!
// For this reason, we have to check it here at the original call site
// otherwise inside Operator/Subscriber we won't know if `undefined`
// means they didn't provide anything or if they literally provided `undefined`
if (arguments.length >= 2) {
hasSeed = true;
}

return this.lift(new ScanOperator(accumulator, seed, hasSeed));
}

class ScanOperator<T, R> implements Operator<T, R> {
constructor(private accumulator: (acc: R, value: T, index: number) => R, private seed?: T | R) {
}
constructor(private accumulator: (acc: R, value: T, index: number) => R, private seed?: T | R, private hasSeed: boolean = false) {}

call(subscriber: Subscriber<R>, source: any): any {
return source._subscribe(new ScanSubscriber(subscriber, this.accumulator, this.seed));
return source._subscribe(new ScanSubscriber(subscriber, this.accumulator, this.seed, this.hasSeed));
}
}

Expand All @@ -64,26 +73,22 @@ class ScanOperator<T, R> implements Operator<T, R> {
*/
class ScanSubscriber<T, R> extends Subscriber<T> {
private index: number = 0;
private accumulatorSet: boolean = false;
private _seed: T | R;

get seed(): T | R {
return this._seed;
}

set seed(value: T | R) {
this.accumulatorSet = true;
this.hasSeed = true;
this._seed = value;
}

constructor(destination: Subscriber<R>, private accumulator: (acc: R, value: T, index: number) => R, seed?: T | R) {
constructor(destination: Subscriber<R>, private accumulator: (acc: R, value: T, index: number) => R, private _seed: T | R, private hasSeed: boolean) {
super(destination);
this.seed = seed;
this.accumulatorSet = typeof seed !== 'undefined';
}

protected _next(value: T): void {
if (!this.accumulatorSet) {
if (!this.hasSeed) {
this.seed = value;
this.destination.next(value);
} else {
Expand Down