Skip to content

fix: return NaN, not 0, for an unparseable quantity ("an hour" === 0) - #49

Open
MildlyMeticulous wants to merge 1 commit into
agenda:masterfrom
MildlyMeticulous:fix/unparseable-quantity-returns-zero
Open

fix: return NaN, not 0, for an unparseable quantity ("an hour" === 0)#49
MildlyMeticulous wants to merge 1 commit into
agenda:masterfrom
MildlyMeticulous:fix/unparseable-quantity-returns-zero

Conversation

@MildlyMeticulous

Copy link
Copy Markdown

The bug

humanInterval('an hour') returns 0.

Not NaN, not 3600000 — zero. Same for 'a day', 'a few minutes', 'some seconds', 'half an hour', 'couple of hours'.

const humanInterval = require('human-interval')

humanInterval('hour')          // 3600000
humanInterval('one hour')      // 3600000
humanInterval('an hour')       //       0   <-- expected 3600000
humanInterval('a few minutes') //       0   <-- expected NaN
humanInterval('foobar')        //     NaN   <-- the documented contract

That last line is the one that makes this a bug rather than a missing feature: the suite already asserts

test('Returns NaN when given unknown string', macro, 'foobar', Number.NaN);

so unrecognised input is supposed to be NaN. 'a few minutes' is just as unrecognised, but it silently reports zero instead.

Why it matters

The README points at Agenda and Bree, and both treat the return value as an interval. Zero is not an inert "nothing happened" value there — it means fire again immediately. So a typo'd or slightly-too-natural interval string doesn't throw, doesn't warn, and doesn't get skipped; it turns into a hot loop. NaN fails loudly, which is what you want.

Cause

numbered.parse() returns 0 for anything it can't parse, with no way to distinguish that from a real zero:

numbered.parse('one')  // 1
numbered.parse('zero') // 0   <- genuine
numbered.parse('an')   // 0   <- failure
numbered.parse('xyz')  // 0   <- failure

humanInterval takes that 0 at face value and multiplies it by the unit.

Fix

const article = /^an?$/;

const parseWords = words => {
  if (article.test(words)) {
    return 1;
  }

  const number = numbered.parse(words);
  return number === 0 && !/\bzero\b/.test(words) ? Number.NaN : number;
};

Two changes, both following behaviour the library already has:

  • a / an → 1. 'one hour' already works and a bare 'hour' already defaults to 1, so 'an hour' agreeing with them is the consistent outcome.
  • Unrecognised quantity → NaN. Matches the existing 'foobar' contract. An explicit 'zero seconds' still returns 0.

Verification

All 59 existing tests pass unchanged. 6 tests added (65 total); against the unpatched index.js 5 of them fail.

Note on compatibility

This is a behaviour change for input that previously returned 0. I'd argue every one of those cases was already wrong — and dangerously so given the scheduler use case — but if you'd rather have 'a'/'an' support without the NaN half, say the word and I'll split the commit.

numbered.parse() returns 0 for any words it cannot parse, which is
indistinguishable from a genuine zero, so "an hour" and "a few minutes"
both came back as 0. For a scheduler an interval of 0 means "fire again
immediately", so this failed open into a hot loop rather than an error.

Treat "a"/"an" as 1, consistent with the bare-unit default and with the
already-supported "one hour", and return NaN for a quantity that is not
recognised, consistent with the existing humanInterval("foobar") === NaN
contract.
@changeset-bot

changeset-bot Bot commented Jul 25, 2026

Copy link
Copy Markdown

⚠️ No Changeset found

Latest commit: 6ad8f7c

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

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.

1 participant