jsmreese / moment-duration-format

Format function plugin for the Moment Duration object.
MIT License
967 stars 121 forks source link

Add `Intl.NumberFormat` support, and cache `NumberFormat` objects when reused #122

Closed ticky closed 5 years ago

ticky commented 5 years ago

This implements use of Intl.NumberFormat over toLocaleString where it is available.

It runs the same feature tests (except for the “locale support” test) on Intl.NumberFormat to determine if it works, and if it suffers from the same bugs as toLocaleString.

Use of Intl.NumberFormat is governed by the same rules as toLocaleString, meaning it is controlled by the useToLocaleString option in addition to its feature tests. I considered either renaming the useToLocaleString option, or adding a second setting, but the intent of the setting seems to be “give me the fallback,” so while I think it would make sense to rename it in future, I didn’t want to do that myself! 😄

Finally, it implements a basic caching mechanism for the Intl.NumberFormat instances. The cachedNumberFormat function maintains a cache of Intl.NumberFormat instances, keyed by their locale and configuration. In this implementation caches on first use when Intl.NumberFormat is used. I have also experimented with a branch (https://github.com/ticky/moment-duration-format/compare/numberformat-support...numberformat-support-multi-use-cache) which caches on the second use of a particular configuration, which gives a similar performance gain when used repeatedly, but doesn’t cache instances which are used only once.

Performance

I’ve compared performance in several ways:

With this patch applied, the existing test suite’s reported run time is reduced from approximately 400-500 milliseconds to 200-300 milliseconds. It varies by browser, of course.

Additionally, I prepared a jsperf comparison between the current npm version, this patch, the aforementioned branch which caches on second use, and the current npm version with useToLocaleString set to `false. Chrome, Firefox and Safari showed these results:

image

Across the board, the fallback formatter implementation is fast, but in all three browsers present, these cache implementations remain competitive, and show a performance gain of somewhere between 3 and 5 times, while maintaining fully-featured locale support.

Finishing Up

I consider this pull request mostly finished, however, I would like some advice on how best to implement tests which specifically target the Intl.NumberFormat implementation: I have observed that the current test suite exercises it in my browsers, but of course this means it no longer exercises toLocaleString support.

I believe that this could be achieved through the introduction of a public useIntlNumberFormat setting (to mirror useToLocaleString), but this would constitute a breaking API change, as existing implementors using useToLocaleString to switch to the fallback would instead end up using Intl.NumberFormat.

fixes #120, likely helps a lot with #107

jsmreese commented 5 years ago

Hi Jessica, thank you for this PR!

Things have been pretty crazy recently, but I'd definitely like to get back to this project soon and get your work into a published version.

ticky commented 5 years ago

Thanks @jsmreese! I know how it can be sometimes. 😄

For what it’s worth we’ve been running this patch in production since August with no hiccups so far!

sanex3339 commented 5 years ago

Any news?

ticky commented 5 years ago

Still running this patch in production, and still happy with it! :)

jsmreese commented 5 years ago

@ticky @sanex3339 I've published version 2.3.0 with this change. Sorry I didn't do that months ago!

ticky commented 5 years ago

@jsmreese wonderful, thank you! I know how this stuff can be, though. Thanks for accepting my wall of text 😄