Fix sum/sumBy/mean/meanBy to return NaN for non-numeric values (fixes #5818) #6095
Conversation
…odash#5818) - Add internal `isNumeric` function to validate numeric values - Add internal `describeType` function for detailed type descriptions - Modify `baseSum` to return NaN and emit console warning for non-numeric values - Update JSDoc with comprehensive documentation **Behavior:** - Returns `NaN` for non-numeric values (matches issue lodash#5818 expected behavior) - Emits descriptive console.warn with: - Index where non-numeric value was found - Type description (string "hello", array, object, function myFunc(), null, etc.) **Examples:** ```javascript _.sumBy([{ a: 1 }, { a: 'hello' }], 'a') // => NaN // Console: "lodash: sum/sumBy encountered non-numeric value at index 1. // Expected number, got string "hello". Returning NaN." _.sumBy([{ a: 1 }, { a: [1,2,3] }], 'a') // => NaN // Console: "lodash: sum/sumBy encountered non-numeric value at index 1. // Expected number, got array. Returning NaN." ``` **Affected functions:** - `sum`, `sumBy`, `mean`, `meanBy` **Not affected (intentionally):** - `min`, `max`, `minBy`, `maxBy` - support comparing any comparable values Fixes lodash#5818 Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
| assert.strictEqual(func(['1', '2']), '12'); | ||
| assert.ok(isNaN(func(['1', '2'])), 'returns NaN for string values'); |
There was a problem hiding this comment.
this constitutes a breaking change.
There was a problem hiding this comment.
Totally understand the breaking change concern - lodash's reach means even bug fixes need careful consideration.
I can revert this change if needed. But I'm genuinely curious about the philosophy here:
Who is intentionally relying on _.sum(['1', '2']) returning '12'?
It's a Math function documented to return a number. If someone passes ['1', '2'] and gets string concatenation, that's almost certainly a bug in their code that's being silently masked. Returning NaN (or even coercing '1' → 1) would help them find that bug.
I get that semver is semver, but is there a threshold where "fixing a clear contract violation" doesn't require a major bump? Or would you suggest an alternative approach - maybe coercing numeric strings to numbers instead of returning NaN?
By the way - the reason I'm putting effort into this PR is because lodash is my favorite library ever. I'd guess it's the most used library on npm (you'd know better than me). I remember my frustration with underscore back in the day, and lodash was a game-changer. I actually created a library called 8000 datedash inspired by the lodash style for date formatting.
If there's a way around this, or a way through it, it would genuinely be an honor to contribute to lodash. Happy to adjust or withdraw - just want to understand the path forward.
There was a problem hiding this comment.
Hopefully nobody's relying on it :-)
that said, it still should wait until a v5. In the meantime, it would be better to have a PR that adds tests for the current behavior, and updates the docs to match.
…odash#5818) Update JSDoc and add tests to document the current behavior where non-numeric values in sum/sumBy/mean/meanBy result in string concatenation rather than numeric addition. This documents the existing behavior as reported in lodash#5818. The behavior may change in a future major version (see lodash#6095 for the proposed fix). Changes: - Update JSDoc for sum, sumBy, mean, meanBy to note string concatenation behavior - Update @returns to reflect that string may be returned - Add examples showing the non-numeric behavior - Add tests documenting string concatenation with various non-numeric inputs - Reference lodash#5818 in documentation Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Summary
Fixes #5818 where
sumByreturns string concatenation ('1fnull') instead of a numeric value when encountering non-numeric values.Changes
isNumericfunction to validate numeric valuesdescribeTypefunction for detailed type descriptions in warningsbaseSumto returnNaNand emit a descriptiveconsole.warnfor non-numeric valuessum,sumBy,mean,meanByBehavior
Before:
After:
Why NaN + Warning (not TypeError)
NaNas requested in MethodsumByreturns string #5818NaN(which istypeof 'number')Types Covered
'hello'string "hello"[1,2,3]array{foo: 'bar'}objectfunction myFunc(){}function myFunc()nullnulltruebooleannew Date()DateTest Plan
npm run buildpassesnpm testpasses (6808 + 327 = 7135 tests)Not Affected (intentionally)
min,max,minBy,maxBy- these legitimately support comparing any comparable values (strings alphabetically, dates chronologically).🤖 Generated with Claude Code