add Bun.ms: ms-style duration parse/format API#31613
Conversation
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
|
Yep — this intentionally supersedes #23162 (and its follow-up #23724), which implemented the same |
01cc9b1 to
53897e2
Compare
46d53b6 to
e78489e
Compare
53897e2 to
0be36c9
Compare
0be36c9 to
37e71ed
Compare
37e71ed to
f18b3df
Compare
b9effe8 to
cbb40a7
Compare
cbb40a7 to
a30583f
Compare
There was a problem hiding this comment.
All prior feedback addressed and the implementation looks solid — deferring to a human since this ships a new public Bun.* API surface (and is stacked on two unmerged PRs), which is a product call worth a maintainer's sign-off.
Extended reasoning...
Overview
This PR adds Bun.ms(), a new public utility on the Bun global modeled on the npm ms package: string → milliseconds parsing (reusing bun_core::parse_ms from earlier in the stack) and milliseconds → string formatting (new bun_core::format_ms). Touches: ~76 lines of new Rust in fmt.rs + a ~67-line host fn in BunObject.rs, one-line wiring in the C++ LUT (BunObject.cpp / BunObject+exports.h), a re-export in lib.rs, two .d.ts overloads, two docs pages, and a 165-line test file (13 tests).
Prior review history
I left five rounds of inline comments on earlier revisions (sub-second truncation, non-number coercion, i64 saturation, undefined-vs-throw on unparseable strings, boxed-string handling, dead exception check, missing docs, a TS2769 in the test, and a broken docs example). Every one was addressed in follow-up commits (53897e2, 37e71ed, f18b3df, cbb40a7, a30583f) and all threads are resolved. The current bug-hunting pass found nothing new.
Security risks
None identified. The function is a pure string/number converter with no I/O, no filesystem/network access, and no auth/crypto/permission implications. Inputs are bounded (parse_ms operates on the byte slice directly with no regex backtracking; format_ms is constant-time per call). The one unsafe block (from_utf8_unchecked on dtoa output) is sound — WTF's dtoa only emits ASCII.
Level of scrutiny
Moderate. The implementation itself is small, well-commented, and now thoroughly tested. What pushes this above my auto-approval bar is that it introduces new public API surface on the Bun global — the overload shapes, the undefined-on-unparseable contract, and the documented intentional divergence from npm ms (lenient whitespace/+ parsing) are product decisions that become hard to change once shipped. That's a maintainer's call, not a bot's.
Other factors
- Stacked on #31608 → #30529, both of which must merge first; merge order is a human concern.
- CI shows musl LTO link failures and one Windows
bun-install.test.tsfailure that look unrelated/infrastructural, but a human should confirm before merge. - Test coverage is good: parsing (compact/long/case-insensitive/negatives), formatting (compact/long/rounding/pluralization/sub-second), strict type rejection, round-trips, and non-finite rejection.
CI statusBuild 60210 (rebased tip, sha d3ea742): 269+ jobs passed; all 15 failing lanes fail on the same single test — This needs a tree-wide fix (pin the Angular version in the test, or investigate the
|
aeb24c9 to
79616b0
Compare
`Bun.ms` mirrors the npm `ms` package:
- `Bun.ms("2 days")` → 172800000 (parse a duration string to milliseconds)
- `Bun.ms(60000)` → "1m" (format milliseconds to a compact string)
- `Bun.ms(60000, { long: true })` → "1 minute" (verbose form)
Parsing reuses `bun_core::parse_ms`; formatting adds `bun_core::format_ms`
with JavaScript `Math.round` tie-breaking (round half toward +∞) and the same
unit scale as the `ms` package (a year is 365.25 days; formatting emits at most
days, never weeks/years). Invalid strings and non-finite numbers throw a
TypeError.
Wires the host function into BunObject (Rust `ms` + LUT entry + exports),
adds the `Bun.ms` TypeScript overloads, and tests the parse/format/rounding
behavior against the `ms` package.
Closes #18597
29c0f0c to
fa4b4b2
Compare
79616b0 to
d3ea742
Compare
There was a problem hiding this comment.
All my earlier feedback has been addressed and the implementation looks solid; deferring for a human to sign off on adding Bun.ms to the public API surface (and on the stacked #30529 dependency).
Extended reasoning...
Overview
This PR adds Bun.ms() — an ms-package-compatible duration parse/format utility — as a new public function on the Bun global. It touches: src/bun_core/fmt.rs (new format_ms + js_math_round + write_js_number helpers), src/runtime/api/BunObject.rs (the host fn), src/jsc/bindings/BunObject.cpp / BunObject+exports.h (LUT wiring), packages/bun-types/bun.d.ts (two overloads + MsOptions), docs (bun-apis.mdx, utils.mdx), and a new 165-line test file. The PR is stacked on #30529.
Security risks
None identified. The function is pure string/number conversion with no I/O, no auth, no filesystem/network access. Inputs are strictly type-checked (primitive string or finite number only); the parser operates on a bounded byte slice with no allocation proportional to untrusted input beyond the input string itself. The one unsafe block (from_utf8_unchecked over WTF dtoa output) is sound — dtoa only emits ASCII.
Level of scrutiny
Moderate-to-high, primarily because this adds new public API surface to the Bun global. The implementation itself is small, self-contained, and well-tested (13 tests covering parse/format/edge cases/type strictness), and went through five rounds of review feedback in this thread — all of which the author addressed (sub-second truncation, i64 saturation, type coercion, undefined-on-unparseable, boxed-string handling, dead-code cleanup, docs, doc-example correctness, TS overload typecheck). The remaining decision is product/API-shape, not correctness: whether Bun.ms belongs on the global, and whether the documented one intentional divergence (slightly more lenient whitespace/+ parsing than npm ms) is acceptable.
Other factors
- The author's own final comment explicitly requests "maintainer sign-off on the new
Bun.mspublic API." - Stacked on #30529, which must merge first.
- CI is green on Linux/Windows; darwin failures are unrelated infra issues (missing Postgres role, OOM-killed runner).
- No CODEOWNERS conflict apparent for these paths.
Given that new public API surface is a design decision a human maintainer should ratify, I'm deferring rather than approving.
Stacked on #30529 (merge that first; this PR's diff is shown against its branch). Reuses the
parse_mslanded in #30529.Requested in #18597; supersedes the earlier attempt #23162 (which predated the Rust rewrite by ~1200 commits).
What
Bun.ms— modeled on the npmmspackage:How
bun_core::parse_msfrom install: accept ms-style duration strings for minimumReleaseAge #30529.bun_core::format_ms, kept inf64throughout and stringified with WTF's dtoa (JSNumber.prototype.toStringsemantics): JavaScriptMath.roundtie-breaking (round half toward +∞), pluralization at ≥ 1.5× in the long form, and the same unit scale (a year is 365.25 days).ms-package behavior matchedundefined(not a throw), soBun.ms(input) ?? fallbackworks. Empty string / non-string-non-number inputs throw (matchingms'stypeofchecks — boxednew String(...)throws too).Bun.ms(1.5)→"1.5ms"); months are not a unit (Bun.ms("1mo")→undefined).One intentional difference: the underlying
parse_ms(shared with bunfig /--minimum-release-ageparsing in #30529) is slightly more lenient thanms's anchored regex — it trims surrounding whitespace and accepts a leading+. It's strictly a superset, so anythingmsparses,Bun.msparses identically.Verification
test/js/bun/util/ms.test.ts— 13 tests pass on debug+ASAN: compact/long/case-insensitive parsing, undefined-on-unparseable, empty-string/boxed-object/wrong-type throwing, compact/long formatting, JS-round tie-breaking, raw sub-second, parse↔format round-trips, and non-finite rejection.cargo clippyclean onbun_core+bun_runtime; debug + release both build clean. Docs added todocs/runtime/utils.mdx+bun-apis.mdx.