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

maximumFractionDigits not working as expected #71

Open
doomsower opened this issue Sep 18, 2021 · 10 comments
Open

maximumFractionDigits not working as expected #71

doomsower opened this issue Sep 18, 2021 · 10 comments

Comments

@doomsower
Copy link

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
Copy link
Owner

// @nmoinvaz

@nmoinvaz
Copy link
Contributor

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

sindresorhus added a commit that referenced this issue Sep 19, 2021
@sindresorhus
Copy link
Owner

@nmoinvaz No worries at all.

@sindresorhus
Copy link
Owner

I have added some failing tests for this: 853dc19

@mianlang
Copy link

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
Copy link
Contributor

nmoinvaz commented Dec 27, 2021

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
Copy link
Contributor

nmoinvaz commented Dec 27, 2021

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

@ghost
Copy link

ghost commented Feb 21, 2024

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:

  • The rounding behavior, especially with maximumFractionDigits, isn't quite what we'd expect.
  • The automatic trimming of leading zeros is causing confusion and messing with the formatting consistency.

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
Copy link
Contributor

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
Copy link

ghost commented Feb 22, 2024

@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 👏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants