🐛Fix unformat number in react-i18n

[j-rooks]

Description

Issue: https://github.com/Shopify/intl-new-commerce-patterns/issues/916

Fixes an issue with unformatNumber when the thousand separator symbol is a period.
Ideally, we would use the formatToParts method to fix the issue but that is still experimental.

Before:

After:

Type of change

• react-i18n Patch: Bug/ Documentation fix (non-breaking change which fixes an issue or adds documentation)

Checklist

• I have added a changelog entry, prefixed by the type of change noted above

[j-rooks]

Could not find any examples of languages using a space for the thousand separator and a period for the decimal.
See: https://docs.oracle.com/cd/E19455-01/806-0169/overview-9/index.html

[benoitzohar]

LGTM, but I would add a test (or tests) for getSeparators()

[j-rooks]

LGTM, but I would add a test (or tests) for getSeparators()

@benoitzohar Actually, I'm not seeing any tests for helper functions such as normalizedNumber in i18n.ts. That makes sense to me since testing unformatNumber already tests for normalizedNumber and getSeparators.

[benoitzohar]

@adjnor It's true.
However, it feels weird to mock formatNumber but not getSeparators and normalizedNumber.
I would be consistent in the tests, either mocking all the helpers to only test the function unformatNumber (and have tests for each helper), or not mock formatNumber.
But since formatNumber() is tested already, my favorite solution would be the first one (mock all helpers and add tests).

It might be nitpicking, let me know what you think about this.

[j-rooks]

As we discussed IRL, I'll update the PR to move the logic from getSeparators into numberSymbols since both functions are used for the same thing.

[lukashlavacka]

Good catch and a nice fix. My only concern is the computational cost of numberSymbols. I am not sure if we could memoize something or just define some one-time evaluated const, I assume the i18n formatting cannot change without reloading the page (and therefore the javascript).

[j-rooks]

@lukashlavacka Good point, I added the @memoize() decorator to the numberSymbols method.
The failing CI doesn't seem to be related to my changes, should be fixed once this PR ships

[lukashlavacka]

Changes, tests and 🎩 LGTM!