Skip to content

fix: match the format argument like an input unit - #73

Merged
dy merged 1 commit into
jkroso:masterfrom
MFA-G:fix/format-unit-matching
Sep 21, 2026
Merged

dy merged 1 commit into
jkroso:masterfrom
MFA-G:fix/format-unit-matching

Conversation

@MFA-G

@MFA-G MFA-G commented Sep 20, 2026

Copy link
Copy Markdown
Contributor

The problem

Input units are normalized before lookup — lower-cased, then retried without a trailing s:

else units = units.toLowerCase()
prevUnits = units = unit[units] || (units.endsWith('s') ? unit[units.slice(0, -1)] : undefined)

The format argument went through neither step:

return result && ((result / (unit[format] || 1)) * ...)

So a unit spelling that parses perfectly well as input silently fails as a format and falls through to the || 1 ms fallback:

parse('2MIN')          // => 120000   ✓ accepted as input
parse('2h', 'MIN')     // => 7200000  ✗ expected 120
parse('2h', 'mins')    // => 7200000  ✗ expected 120
parse('2h', 'hrs')     // => 7200000  ✗ expected 2
parse('2h', 'Hours')   // => 7200000  ✗ expected 2
parse('2h', 'seconds') // => 7200000  ✗ expected 7200

parse('2h', 'M') is the nastiest case. M is a legitimate minute spelling on the input side in any casing, so the caller gets 7200000 — a plausible number in the wrong unit, 60000× off, with nothing to indicate the format was ignored.

The plural forms matter in practice because 'seconds' and 'mins' are exactly what you'd carry over from the input strings this library is built to read (parse('10 seconds') works, the readme advertises 1hr 20mins).

The fix

Extract the existing input-side normalization into lookupUnit and use it on both sides, so one spelling rule governs the whole API:

const lookupUnit = (unit, name) => {
  name = String(name).toLowerCase()
  return unit[name] ?? (name.endsWith('s') ? unit[name.slice(0, -1)] : undefined)
}

This is behavior-preserving for everything that already resolved: exact lower-case names hit the first lookup unchanged, and an unrecognized format still falls back to ms via the existing || 1.

One deliberate detail: the input path used unit[units] || ..., which retries on 0 as well as undefined. lookupUnit uses ?? so a locale that legitimately defines a 0 unit isn't re-looked-up. No bundled locale has one, so nothing changes today — it just avoids a latent trap for custom locales.

Validation

  • npm test — 102/102 pass (95 before, 7 added).
  • Reverting index.js and keeping the new tests fails exactly the 6 assertions covering the bug, so they're not vacuous.
  • The added case parse('2h', 'bogus') === 7200000 pins the ms fallback so this can't quietly turn into a throw later.

Also added one readme line next to the existing custom output format example, since the format argument's matching rules weren't documented.

`parse('2MIN')` resolves fine because input units are lower-cased and
retried without a trailing `s`, but the `format` argument was looked up
verbatim on `parse.unit`. So `parse('2h', 'MIN')`, `parse('2h', 'mins')`
and `parse('2h', 'hrs')` all silently fell back to ms and returned
7200000 instead of 120 / 120 / 2 — a 60000x error with no signal.

`parse('2h', 'M')` was the worst case: `M` is a valid unit name in any
casing on the input side, so the caller gets a plausible-looking number
in the wrong unit.

Extract the input-side normalization into `lookupUnit` and use it for
both sides. An unrecognized format still falls back to ms.
@dy
dy merged commit 0d517bd into jkroso:master Sep 21, 2026
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants