sindresorhus / pretty-bytes

Convert bytes to a human readable string: 1337 → 1.34 kB
MIT License
1.11k stars 82 forks source link

maximumFractionDigits not working as expected #71

Open doomsower opened 3 years ago

doomsower commented 3 years ago

This code

console.log({
    size,
    def: prettyBytes(size),
    max: prettyBytes(size, { maximumFractionDigits: 1 }),
    minMax: prettyBytes(size, {
      maximumFractionDigits: 1,
      minimumFractionDigits: 1,
    }),
  });

Results in following output:

{"def": "60 MB", "max": "59.952784 MB", "minMax": "59.952784 MB", "size": 59952784}

I expect that both max and minMax should be 59.9 MB.

I'm using pretty-bytes@^5.6.0

sindresorhus commented 3 years ago

// @nmoinvaz

nmoinvaz commented 3 years ago

@sindresorhus I am not doing well and will not be able to attend to this for some time.

sindresorhus commented 3 years ago

@nmoinvaz No worries at all.

sindresorhus commented 3 years ago

I have added some failing tests for this: https://github.com/sindresorhus/pretty-bytes/commit/853dc191600a6558b8b698069336d8c4b53029bb

mianlang commented 2 years ago

As I tested in my env 1: pretty-bytes@5.6.0, Windows 11 21H2, node v14.17.6 and env 2: pretty-bytes@5.6.0, Ubuntu 20.04.3 LTS, node v17.0.1,

I got same result:

{ size: 59952784, def: '60 MB', max: '60 MB', minMax: '60.0 MB' }

There seems to be no fraction digit problems on my devices.

I assume the rounding 59952784 -> 60.0 is reasonable. And the failing tests 853dc19 with -> 59.9 might need some modifications. If so, maybe something like 3945, 2 digits -> 3.95 (which my snippet output) could also be added to the tests (or maybe and the readme) to clarify the rounding.

nmoinvaz commented 2 years ago

minimumFractionDigits and maximumFractionDigits functionality is provided by JavaScript's Number.toLocaleString function options parameter (described here in Intl.NumberFormat).

Intl.NumberFormat appears to always perform rounding even though some documentation on the web does not mention it.

nmoinvaz commented 2 years ago

There is an ECMA proposal to support additional rounding options for Intl.NumberFormat options. https://github.com/tc39/proposal-intl-numberformat-v3

ghost commented 7 months ago

Hey @sindresorhus,

I've checked out the issue reported by @doomsower regarding the prettyBytes function in the pretty-bytes package, and I've got some thoughts on it.

Issue Summary: It looks like there are a couple of issues with prettyBytes. Firstly, there are some rounding discrepancies, especially noticeable when maximumFractionDigits is set to 1. Also, there's a problem with automatic trimming of leading zeros.

Analysis: After digging into it, two main issues became clear:

Proposed Solution: I think we should tweak the prettyBytes function to handle formatting manually using Number.toFixed(). This way, we can ensure consistent rounding, plus avoid the automatic trimming of leading zeros.

// Format digits after the decimal point
const numString = number.toFixed(options.maximumFractionDigits);

Given the current limitations and performance issues of Intl.NumberFormat(), I suggest we steer clear of it for now (I know, it sounds a bit extreme, but it's causing more trouble than it's worth). Instead, let's provide an option to format the output as 59,9 instead of 59.9 when needed.

Conclusion: These changes will iron out the kinks in prettyBytes and make it more accurate and consistent, providing a better user experience overall.

Looking forward to your thoughts on this approach.

Cheers, Edu

nmoinvaz commented 7 months ago

https://github.com/tc39/proposal-intl-numberformat-v3 states it was shipped in Chrome 106. I think it would be better just to add support for the rounding options roundingPriority and roundingMode.

ghost commented 7 months ago

@nmoinvaz sollution rocks!

With the options suggested by him, when using minimumFractionDigits and maximumFractionDigits set to 1, the values now became consistent, look:

// Tested on Node.js v21.6.1

Number(60.0000).toLocaleString('fr',{minimumFractionDigits: 0, maximumFractionDigits: 0, roundingMode: 'floor'})
// Returns: '60'

Number(60.0000).toLocaleString('fr',{minimumFractionDigits: 1, maximumFractionDigits: 1, roundingMode: 'floor'})
// Returns: '60,0'

Number(59.952784).toLocaleString('fr',{minimumFractionDigits: 1, maximumFractionDigits: 1, roundingMode: 'floor'})
// Returns: '59,9'

Number(59.952784).toLocaleString('fr',{minimumFractionDigits: 2, maximumFractionDigits: 2, roundingMode: 'floor'})
// Returns: '59,95'

Following the NumberFormat docs, the only change needed to fix the problems mentioned would be to add roundingMode: 'floor' to the default locale options. Quite a simple solution but works like charm 👏