fix: match the format argument like an input unit - #73
Merged
Merged
Conversation
`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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The problem
Input units are normalized before lookup — lower-cased, then retried without a trailing
s:The
formatargument went through neither step:So a unit spelling that parses perfectly well as input silently fails as a format and falls through to the
|| 1ms fallback:parse('2h', 'M')is the nastiest case.Mis a legitimate minute spelling on the input side in any casing, so the caller gets7200000— 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 advertises1hr 20mins).The fix
Extract the existing input-side normalization into
lookupUnitand use it on both sides, so one spelling rule governs the whole API: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 on0as well asundefined.lookupUnituses??so a locale that legitimately defines a0unit 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).index.jsand keeping the new tests fails exactly the 6 assertions covering the bug, so they're not vacuous.parse('2h', 'bogus') === 7200000pins the ms fallback so this can't quietly turn into a throw later.Also added one readme line next to the existing
custom output formatexample, since the format argument's matching rules weren't documented.