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

[Bug]: "space-comma format" failing in node 18.18.x and 21.x #1937

Closed
1 task done
twk3 opened this issue Nov 20, 2023 · 2 comments
Closed
1 task done

[Bug]: "space-comma format" failing in node 18.18.x and 21.x #1937

twk3 opened this issue Nov 20, 2023 · 2 comments
Labels
bug Something isn't working

Comments

@twk3
Copy link
Contributor

twk3 commented Nov 20, 2023

Verified issue does not already exist?

  • I have searched and found no existing issue

What happened?

space comma format is not working in nodejs 18.18. This was found while running the tests.

The test fails when running on nodejs 18.18 or nodejs 21.x

This is caused by a nodejs's v8 update, which also brought in ICU version 73. This change to the en-ZA language (the one actual-budget uses for space-comma number format) was introduced in ICU 73 https://unicode-org.atlassian.net/browse/CLDR-16247

Versions of nodejs using ICU 72 still pass.

Likely a different space-comma locale should be used.

What error did you receive?

FAIL  src/shared/util.test.ts
  ● utility functions › number formatting works with space-comma format

    expect(received).toBe(expected) // Object.is equality

    Expected: "1 234,56"
    Received: "1,234.56"

      69 |     let formatter = getNumberFormat().formatter;
      70 |     // grouping separator space char is a non-breaking space, or UTF-16 \xa0
    > 71 |     expect(formatter.format(Number('1234.56'))).toBe('1\xa0234,56');
         |                                                 ^
      72 |
      73 |     setNumberFormat({ format: 'space-comma', hideFraction: true });
      74 |     formatter = getNumberFormat().formatter;

      at Object.<anonymous> (src/shared/util.test.ts:71:49)

Where are you hosting Actual?

Locally via Yarn

What browsers are you seeing the problem on?

Other

Operating System

Linux

@twk3 twk3 added the bug Something isn't working label Nov 20, 2023
@twk3
Copy link
Contributor Author

twk3 commented Nov 20, 2023

I thought switching to fr-FR would be the easiest choice, but fr-FR uses a breaking space rather than a non-breaking space.

no-NO, ru, uk, en-SE, en-FI all seem to work

Currently I'm suggesting en-SE or en-FI. as they are a smaller change from en-ZA, as they share a bunch of the english locale rules.

@MatissJanis
Copy link
Member

Excellent issue description! Thanks for taking your time to explain this so well. Makes my job reviewing that much more easier :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants