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

[jayphelps]

Array#reduce supports undefined as a valid seed value, so we should too. This was mostly an oversight.

of(1, 2, 3).reduce((acc, x) => acc + ' ' + x, undefined);
// "undefined 1 2 3"

of(1, 2, 3).scan((acc, x) => acc + ' ' + x, undefined);
// "undefined 1"
// "undefined 1 2"
// "undefined 1 2 3"

fixes #2047

Cc/ @Blesh

[coveralls]

Coverage increased (+0.0009%) to 97.041% when pulling a34a76d on jayphelps:scan-seed into 5460e77 on ReactiveX:master.

[kwonoj]

Implementation side it looks fine, just need to clarify if this is expected behavior. (for me it seems legit to align with native)

[trxcllnt]

@jayphelps @kwonoj I've always wondered, why do we use a getter/setter for the seed value? Is it faster?

[jayphelps]

I'm betting there is no reason other than that's what the person who wrote it used.

[kwonoj]

I'm betting there is no reason other than that's what the person who wrote it used.

: Pretty much this case. Using getter / setter in this codebase is mostly depends on preference / chocies of writer. At least it does not gives any perf or so.

[mattpodwysocki]

LGTM, we need to be careful as undefined is as valid a value as null in terms of initial values, such as startWith etc. And Function.prototype.bind doesn't change the number of parameters in the end going to the function, so we're fine with checking the param count.

[benlesh]

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?

[jayphelps]

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.

[mattpodwysocki]

👍

[trxcllnt]

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

[jayphelps]

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

[trxcllnt]

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

[coveralls]

Coverage increased (+0.0009%) to 97.041% when pulling 34e2e4f on jayphelps:scan-seed into 8ebee66 on ReactiveX:master.

[coveralls]

Coverage increased (+0.0009%) to 97.041% when pulling 0827628 on jayphelps:scan-seed into ece1b89 on ReactiveX:master.

[coveralls]

Coverage increased (+0.0008%) to 97.217% when pulling 0df3dc9 on jayphelps:scan-seed into fea08e9 on ReactiveX:master.

[jayphelps]

Approved during our core meeting. I don't like to self-merge, but I was tasked to merge all the approved PRs so #YOLO

[lock]

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.