Repository navigation
fix[trim, trimStart, trimEnd, startsWith, endsWith]: coerce arguments like lodash - #2104
Archibald1948 wants to merge 6 commits into
Conversation
`toString` carried a private `baseToString` helper that mirrors lodash's internal function of the same name. The string functions need it too: lodash coerces `chars` and `target` with `baseToString`, which differs from the public `toString` on nullish values (`null` becomes `'null'`, not `''`). Move it to `_internal/baseToString.ts` unchanged and import it back into `toString`, so the behaviour of `toString` is untouched. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0129BhQdChg7wJry7bWvRHHm
`trim` called `str.toString().trim()` and treated a nullish `chars` as
"no characters given", so a number, array or object threw, `-0` lost its
sign, and `trim(' a ', null)` trimmed whitespace instead of the
characters of the string `'null'`.
Lodash coerces the string with `toString` and `chars` with
`baseToString`. Do the same, which also means an array `chars` is
stringified rather than flattened, so `trim('a,b', ['a', 'b'])` trims the
comma too, as in lodash.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0129BhQdChg7wJry7bWvRHHm
Same coercion as `trim`: the string goes through `toString`, so `-0` keeps its sign and non-strings no longer throw, and `chars` goes through `baseToString`, so `null` is the string `'null'`. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0129BhQdChg7wJry7bWvRHHm
Same coercion as `trim`: the string goes through `toString`, so `-0` keeps its sign and non-strings no longer throw, and `chars` goes through `baseToString`, so `null` is the string `'null'`. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0129BhQdChg7wJry7bWvRHHm
`startsWith` returned `false` whenever the string or the target was
nullish and called `str.startsWith`, so `startsWith(undefined, '')` was
`false` where lodash returns `true`, `startsWith('null', null)` was
`false` instead of `true`, and a number threw.
Coerce like lodash: the string with `toString`, the target with
`baseToString`, and the position with `toInteger` clamped to the string
length. The comparison stays on the native method, which is specified as
`string.slice(index, index + search.length) === search`.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0129BhQdChg7wJry7bWvRHHm
Same as `startsWith`, plus the position rule: lodash only falls back to
the string length when `position` is `undefined`, so `null` coerces to
`0` and `endsWith('xax', 'x', null)` is `false`, while compat treated it
as absent and returned `true`.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0129BhQdChg7wJry7bWvRHHm
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved Lodash evaluation-order and nullish short-circuit mismatches remain in the string helpers.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
This PR aligns compat string helpers with Lodash argument-coercion behavior and adds regression coverage.
Changes:
- Extracts and reuses
baseToString. - Updates trim, prefix, and suffix helpers to coerce arguments and positions.
- Adds regression tests for non-string and edge-case inputs.
File summaries
| File | Summary |
|---|---|
src/compat/util/toString.ts |
Reuses the extracted coercion helper. |
src/compat/string/trimStart.ts |
Updates leading-trim coercion. Moderate (2 votes): preserve the nullish-input short circuit before coercing chars. |
src/compat/string/trimStart.spec.ts |
Adds leading-trim regression tests. |
src/compat/string/trimEnd.ts |
Updates trailing-trim coercion. Moderate (2 votes): preserve the nullish-input short circuit before coercing chars. |
src/compat/string/trimEnd.spec.ts |
Adds trailing-trim regression tests. |
src/compat/string/trim.ts |
Updates trim coercion. Moderate (2 votes): avoid coercing chars before the !string short circuit. |
src/compat/string/trim.spec.ts |
Adds trim regression tests. |
src/compat/string/startsWith.ts |
Updates prefix matching. Moderate (2 votes): calculate the clamped index before coercing target. |
src/compat/string/startsWith.spec.ts |
Adds prefix regression tests. |
src/compat/string/endsWith.ts |
Updates suffix matching. Moderate (1 vote): calculate the end position before coercing target. |
src/compat/string/endsWith.spec.ts |
Adds suffix regression tests. |
src/compat/_internal/baseToString.ts |
Extracts the shared Lodash-compatible coercion helper. |
Review details
Suppressed comments (1)
src/compat/string/endsWith.ts:26
- Lodash coerces and clamps
positionbefore it callsbaseToString(target), but this code convertstargetfirst. That order is observable whenposition.valueOf()mutates state read bytarget.toString(), and can change the boolean result; calculateendbefore convertingtarget.
const search = baseToString(target);
const length = string.length;
const end = position === undefined ? length : Math.min(Math.max(toInteger(position), 0), length);
- Files reviewed: 12/12 changed files
- Comments generated: 4
- Review effort level: Lite
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| const search = baseToString(target); | ||
| const index = position == null ? 0 : Math.min(Math.max(toInteger(position), 0), string.length); |
| const chrs = baseToString(chars); | ||
|
|
||
| if (!string || !chrs) { | ||
| return string; | ||
| } | ||
|
|
||
| return trimToolkit(string, chrs.split('')); |
| const chrs = baseToString(chars); | ||
|
|
||
| if (!string || !chrs) { |
| const chrs = baseToString(chars); | ||
|
|
||
| if (!string || !chrs) { |
|
Thanks for the detailed write-up. We're closing this one, though. All of the inputs here ( We've written this down as a rule: https://github.com/toss/es-toolkit/blob/main/.github/CONTRIBUTING.md#fixing-behavior-that-differs-from-lodash. We accept a compat difference only if the call compiles against If you find a project that passes such input, please reopen with the permalink. |
|
Thank you for the review. Checked it. |
The difference (as code)
trim,trimStart,trimEnd,startsWithandendsWithassume their arguments are already strings, so anything else throws, and the few non-string cases they do handle disagree with lodash:Cause
Lodash coerces each argument before using it, with three different helpers:
toString:nullandundefinedbecome'', and-0becomes'-0';chars/targetgo through the internalbaseToString, which differs on nullish values:nullbecomes'null', not'';positiongoes throughtoIntegerand is clamped, sonullis0and'1'is1.es-toolkit/compatinstead calls string methods on the raw argument (str.toString().trim(),str.startsWith(...)) and returns early when an argument is nullish:So a number, boolean, array or plain object throws instead of being stringified,
-0loses its sign ((-0).toString()is'0', while lodash'stoStringspecial-cases it), anulltargetorcharsshort-circuits instead of becoming'null', and anullpositionis treated as "absent" rather than0.Changes
Coerce the arguments the way lodash does, without changing any public signature.
toStringalready implements lodash'stoStringexactly, and it had lodash'sbaseToStringas a private helper in the same file. That helper is nowsrc/compat/_internal/baseToString.tsso the string functions can use it too;toStringkeeps calling it and is unchanged.trimStartandtrimEndget the same shape, andendsWithmirrorsstartsWithwith lodash'sposition === undefined ? length : ...rule.One consequence worth pointing out:
charsgiven as an array is now stringified bybaseToStringrather than flattened, sotrim('a,b', ['a', 'b'])trims the comma too and returns'', which is what lodash returns (baseToString(['a', 'b'])is'a,b').Verification
Focused grid against
lodash@4.18.1— 16 string values x 14chars/targetvalues (x 11positionvalues forstartsWith/endsWith), 5,600 calls:2,585 of those 2,655 were "es-toolkit throws, lodash returns a value"; the remaining 70 were wrong values of the four shapes shown at the top.
Broad sweep, every 1- and 2-argument combination of a 35-value pool (1,260 calls per function):
trimtrimStarttrimEndstartsWithendsWithtoStringtoStringis unchanged, before and after, which is the check that movingbaseToStringout of it changed nothing.Regression tests were added to all five spec files. On
mainthey fail with:Full suite: 714 files, 5069 passed / 2 skipped.
tsc --noEmitclean,yarn lint0 errors (no new warnings in the touched files), type-tests pass — the public signatures are untouched, so they still match@types/lodash.Benchmarks
The added work is one
toString/baseToStringcall per invocation. The comparison itself still runs through the nativeString.prototypemethods:string.startsWith(search, index)is specified asstring.slice(index, index + search.length) === search, andendsWith(search, end)matches lodash's position arithmetic, so there is no need to build substrings by hand.Interleaved A/B (
fix, thenmain, five alternating rounds) over the six benchmark files that cover these functions. The Vite dependency cache was cleared before every run, and after every run I checked the pre-bundledes-toolkit/compatfor which implementation had been measured (10/10 matched the intended side). Each row is also divided by the lodash row of the same file in the same round, to cancel machine drift:trimtrimStarttrimEndstartsWithendsWithtoString(unchanged code)A caveat on how much these numbers are worth: my machine was busy while measuring (load average around 6 on 8 cores), single rounds ranged from -40% to +17%, and the untouched lodash controls moved by -5.3% to +2.8%.
toStringis the useful reference here — its code is identical on both sides, and it still reads -2.2%. So I would summarise this as no measurable change beyond the noise, with a small constant coercion cost that the benchmarks cannot resolve. I am happy to re-run on a quiet machine if you would like tighter numbers.(An earlier revision of this branch compared with
slice(...) === targetinstead of the native methods and measured clearly worse — that is why the code keepsstartsWith/endsWith.)🤖 Generated with Claude Code
https://claude.ai/code/session_0129BhQdChg7wJry7bWvRHHm