Normalize the decimal separator on every component, not just the first#68
Conversation
parse() normalized only the first decimal separator: String.prototype
.replace with a string argument replaces a single occurrence. In locales
whose decimal separator is "," (de, fr, es, ru, pt, ...), every component
after the first kept its comma and was mis-tokenized:
parse.unit = de
parse('1,5 s 1,5 s') // 6501 (should be 3000)
parse('2,5h 3,5h') // 27180000 (should be 21600000)
Use replaceAll so each component's separator is normalized. The en "."
decimal is unaffected (replaceAll('.', '.') is a no-op).
|
Cases are a bit confusing to me - can you give examples of something more real, why would you expect values like |
There was a problem hiding this comment.
Pull request overview
This PR fixes a locale parsing bug where only the first decimal separator was normalized, causing multi-component durations (e.g., 1,5 s 1,5 s) to be mis-tokenized in comma-decimal locales.
Changes:
- Normalize decimal separators across the entire input string (not just the first occurrence).
- Extend the existing
locale separatorstest with multi-component comma-decimal cases to prevent regressions.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
index.js |
Updates decimal normalization logic to apply to all components in the input. |
test.js |
Adds assertions covering multi-component inputs in comma-decimal locales. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| .replaceAll(parse.unit.decimal, '.') // normalize decimal separator | ||
| .replace(durationRE, (_, n, units) => { |
|
Fair point, import parse from 'parse-duration'
import de from 'parse-duration/locale/de.js'
parse.unit = de
parse('1,5h 30,5min') // 7500000 (wrong; the 30,5min keeps its comma)
// expected 7230000 = 1.5h + 30.5min
|
Problem
parse()normalizes the decimal separator withString.prototype.replace(parse.unit.decimal, '.'). With a string first argument,replaceonly replaces the first occurrence. So in any locale whose decimal separator is,(de, fr, es, ru, pt, id, ...), every component after the first keeps its comma and is mis-tokenized:Trace of
'1,5 s 1,5 s'(de):'1,5 s 1,5 s'→ replace first,→'1.5 s 1,5 s'→ tokenized as1.5 s(1500) + bare1(no unit → 1ms) +5 s(5000) = 6501. Single-component inputs (the existinglocale separatorstest) happen to work because there is only one separator.Fix
Use
replaceAllso every component's separator is normalized:replaceAllwith a string does a literal global replacement, so the en.decimal stays safe (replaceAll('.', '.')is a no-op). Available on all CI-tested runtimes (Node 18/20/22).Verification
locale separatorstest (1,5 s 1,5 s→ 3000,2,5h 3,5h→ 21600000). They fail onmasterand pass with the fix.tapesuite: 82/82 (80 existing + 2 new).