Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix unsafe and unexpected Duration decoding and conversion behavior #4349

Open
fubhy opened this issue Jan 27, 2025 · 0 comments
Open

Fix unsafe and unexpected Duration decoding and conversion behavior #4349

fubhy opened this issue Jan 27, 2025 · 0 comments

Comments

@fubhy
Copy link
Member

fubhy commented Jan 27, 2025

Some of the conversions and scaling of values in the Duration module can reach into unsafe integer territory.

E.g. when converting nanos to Number for downscaling to seconds, etc.

Avoiding this whilst not losing precision would likely require depending on the BigDecimal module (our second best module).

Or we could do sth. like this:

/**
 * @since 3.8.0
 * @category getters
 */
export const toHours = (self: DurationInput): number =>
  match(self, {
    onMillis: (millis) => millis / 3_600_000,
    // Upscale by `1e6` before division as BigInt, then divide again by `1e6` to preserve up to 6 decimal points
    onNanos: (nanos) => Number(nanos * bigint1e6 / hourInNanos) / 1_000_000
  })

Duration.unsafeDivide

Instead of just copying the behaviour of Duration.divide and making it throw in case of invalid input, it has different semantics for the internal computations. For instance it allows division by zero and turns that into Duration.infinity. It also diferentiates between 0 and -0 (returns Duration.zero).

It should just throw in these cases, imo.

Duration.divide & Duration.unsafeDivide

The current implementations don't handle floating point division of Duration.nanos. Not the end of the world but definitely unexpected.

Duration.decode

Duration.decode can currently unexpectedly throw when given a floating point unit string for nanos or micros:

E.g.

Duration.decode("1.5 micros") // Boom!
Duration.decode("1.5 nanos") // Boom!

Of course, this is a ridiculous use of these APIs and is unlikely to happen in reality.

Duration.times

This one always trips me up. I instinctively search for Duration.multiply and that's also the name we use for multiplication in other places.

It also throws when multiplying with floating point numers which is supported by the type signature.

Duration.times("5 seconds", 2.5) // Boom!
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

No branches or pull requests

1 participant