Skip to content
This repository was archived by the owner on Apr 11, 2024. It is now read-only.

feat: use Number.toLocaleString to support localization#216

Open
pimlie wants to merge 6 commits into
bestiejs:mainfrom
pimlie:feat-locale-support
Open

feat: use Number.toLocaleString to support localization#216
pimlie wants to merge 6 commits into
bestiejs:mainfrom
pimlie:feat-locale-support

Conversation

@pimlie

@pimlie pimlie commented Jan 2, 2019

Copy link
Copy Markdown

Resolves: #210

This PR add locale support by using Number.toLocaleString for number formatting.

Btw, as we discussed in the corresponding issue I did some work on adapting a polyfill for Number.toLocaleString but unfortunately I couldnt get it to fully work correctly in the time I had set for this (the polyfill works actually but when used as a dependency it doesnt download the correct cldr-data due to an issue in the cldr-data-npm package). As most (current?) browsers fully support number localization already I think the full-icu solution will work just fine :)

@jdalton

jdalton commented Jan 2, 2019

Copy link
Copy Markdown
Member

Thank you @pimlie!

It looks like related unit tests are failing. Could you double check them please.

@pimlie

pimlie commented Jan 2, 2019

Copy link
Copy Markdown
Author

Hi @jdalton,

PhantomJS doesnt seem to support toLocaleString (see their issue 12581, it just uses toString). Also the project is currently archived, so what solution do you prefer? Skip the numberFormat tests? Do a manual comparison on user-agent and use the old formatNumber code for phantomjs (and skip the numberFormat tests with a locale)?
The polyfill I adapted doesnt seem to work either when I hack it, it uses libraries which are not phantomjs compatible it seems.

Also I simplified the travis config a bit (although opinions may vary), it work nicely (except for the phantomjs issues) but before I 'finalize' that config maybe you are opposed against using npm scripts like this and prefer the old config?

@jdalton

jdalton commented Jan 2, 2019

Copy link
Copy Markdown
Member

Dropping phantom is fine 😄

@pimlie

pimlie commented Jan 2, 2019

Copy link
Copy Markdown
Author

Great thanks, even better 👍

Let me know if you have any other remarks!

Comment thread benchmark.js
(number[1] ? '.' + number[1] : '');
function formatNumber(number, locale) {
return number.toLocaleString(locale);
}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you add a check during the creation of formatNumber that Number.prototype.toLocaleString is a function and if it's not then make formatNumber fallback to the legacy formatter?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is like this ok or did you mean something else with during the creation of formatNumber?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That'll do!

@prantlf

prantlf commented Jan 31, 2020

Copy link
Copy Markdown

PhantomJS doesnt seem to support toLocaleString (see their issue 12581, it just uses toString).

You could switch to Puppeteer. With some additional test-refactoring effort, of course...

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Development

Successfully merging this pull request may close these issues.

Locale support to format numbers

3 participants