[Fix] reduce/reduceRight: follow Array#reduce when omitting initialValue
#673
Conversation
src/ReactWrapper.jsx
Outdated
| (accum, n, i) => fn.call(this, accum, this.wrap(n), i), | ||
| initialValue, | ||
| ); | ||
| reduce(fn) { |
There was a problem hiding this comment.
could we maybe add the old argument back within comment blocks so we see that it is part of the API. Yes jsdoc helps above, but it gets a bit confusing when see this and then L729
There was a problem hiding this comment.
same for the reduceRight
There was a problem hiding this comment.
I believe @ljharb is trying to preserve the correct .length property on the function, which should only count required arguments. I think elsewhere we just use a default value of undefined, which might be good here.
reduce(fn, initialValue = undefined) {There was a problem hiding this comment.
I can't do that because the native reduce/reduceRight distinguish between undefined and "absent" - or rather, id still have to check arguments.length and ignore initialValue when arguments.length is < 2
There was a problem hiding this comment.
yes yes, i was merely suggesting this: reduce(fn /*, initialValue */) { ... }
There was a problem hiding this comment.
I'll do one of the two - as long as the behavior and function length are correct, I'm happy.
|
@blainekasten bumping versions is indeed cheap, but it's nice to avoid bumping majors because of semver ranges. |
|
I think it's smarter to prevent code from breaking without users expecting it than to worry users will be on old versions of enzyme due to ranges.. |
ljharb
force-pushed
the
fix_reduce
branch
3 times, most recently
from
e5e4f64 to
d45ee73
Compare
|
@ljharb should we get this in for v3? |
|
@nfcampos good call; I'll rebase. |
Closes #667.
I've marked this as a patch - but it might potentially be breaking if someone was relying on omitting the required (incorrectly documented as optional) initialValue being
undefined.Thoughts?