Full rewrite proposal

[edorivai]

@smirzaei already hinted that it might be a good idea to do a full rewrite of this library. I've been thinking a bit, and I'd like to share some of my thoughts of what I think would be good to include in a rewrite.

Perhaps we could use this issue as a place to discuss, and as a central place to monitor the progress of these issues once we start implementing them.

A quick overview:

• Getting a clear vision of the purpose boundaries of this library.
• Source code in modern EcmaScript (es2017 for example)
• Split currency and formatting data
• Allow for importing of specific currency and formatting files
• Create browser build (using something like webpack/rollup)

Not code-specific, but perhaps worth discussing:

In detail

Getting a clear vision of the purpose boundaries of this library

We've already had a bit of a discussion regarding the purpose of this library in #30. I think it might be a good idea to formulate the purpose of this library, and the problem scope it takes on. My attempt:

• currency-formatter is a library of currency formats.
• the actual formatting is done by accounting.js.
• currency-formatter defines sane defaults for each currency and locale, but allows for overriding these defaults.

Thinking of our library in this context helps us constrain the use cases that we want to support. For example, we currently go through a bit of effort to export the list of currencies as an array. I don't think we should do this. IMHO we should expose each format we have, but leave it up to users to wrap it into one big array if they want to.

Source code in modern EcmaScript

Would involve setting up babel, since modules should provide an ES5 compatible version.

Split currency and formatting data

I think we might want to revise the data structure of our formats. As @Gwened pointed out in #37, a currency can have different formats, even within a single country:

For example, I think Canadians speaking French will use 5$ while English speakers write $5.
In a similar way, if I'm targeting brits traveling in Europe, I would write €5 instead of 5 €.

I think it might be a good idea to split our formatting data up into currency, and locale formatting. Where currency data is (surprise) specified per currency, while locale formatting data should perhaps be specified per language ('en', 'es', 'de', ...etc) or even per locale ('en-US', 'en-GB', 'de-DE', 'de-AT', ...etc). These currency and locale formats could look like this:

Currency:
{
	code: 'USD',
	symbol: '$',
	decimalDigits: 2
	defaultLocale: 'en-US'
}

Locale:
{
	locale: 'en-US',
	thousandsSeparator: ' ',
	decimalSeparator: '.',
	symbolOnLeft: false,
	spaceBetweenAmountAndSymbol: true,
}

We could map the current list of currencies to these two types of files. A nice benefit that comes with this structure would be that a lot of redundant formatting data can be captured in "language" level formats ('en', 'de') as opposed to duplicating all of the number formatting information. For example; 85 currencies share the exact same values as USD for thousandsSeparator, decimalSeparator, symbolOnLeft, and decimalDigits:

Object.values(currencies).filter(c => (
	c.thousandsSeparator === ',' &&
	c.decimalSeparator === '.' &&
	c.symbolOnLeft === true &&
	c.decimalDigits === 2).length
))
// > 85

We could just write a single formatting file with locale: 'en' and set defaultLocale: 'en' for all of these 85 currencies.

Allow for importing of specific currency and formatting files

As requested in #34.

I personally think this will be the most important, as well as the most challenging feature. I've recently gone through the process of optimizing the performance of my own app, and stripping unused code is the first place to start. Allowing users to whitelist currencies would be a straightforward way to reduce the amount of unused bytes.

To make this happen we need to split formatting data into separate files, and provide a way for users to add currency and locale formatting data. (As a side note, we should probably have a convenience build or entrypoint which contains all formatting data.)

I'm a fan of how react-intl does this:

import {addLocaleData} from 'react-intl';
import en from 'react-intl/locale-data/en';
import fr from 'react-intl/locale-data/fr';
import es from 'react-intl/locale-data/es';

addLocaleData([...en, ...fr, ...es]);

If we end up splitting currency and locale data, we would need two functions; addCurrencyData and addLocaleData. Given the defaultLocale, we could have currency files auto-import their default locale. This would result in a simpler API for users that don't care about locale formatting (they'll never have to call addLocaleData). Downside; if you only need to support a small set of locales, but all currencies, we might end up importing a lot of locale formats which will never be used.

Create browser build (using something like webpack/rollup)

Things to discuss:

• What bundler to use
• Do we create separate bundles for every formatting file?
• Do we publish the browser bundles to a CDN?

[smirzaei]

Great sum up. Thanks @edorivai as always 😁 and I agree with pretty much every point you made 🙂

Just one thing.... Changing the locale structure doesn't seem very straightforward to me and I don't see an easy way to find the defaultLocale here:

Currency:
{
	code: 'USD',
	symbol: '$',
	decimalDigits: 2
	defaultLocale: 'en-US'
}

Plus our locale data is very sparse.

If we keep both object structures the same (currency and locale) then we can use them to override each other (Object.assign). And people can add their locale info eventually.

This makes the most sense to me

Going to copy some parts of my comment

import { format } from 'currency-formatter'
import USD from 'currency-formatter/currencies/USD'
import en_US from 'currency-formatter/locales/en-US'

const customUserOverrides = { symbolOnLeft: false } // this is optional

format(1234, USD, en_US, customUserOverrides)
format(1234, USD, en_US)

Set a default locale:

import { format, addLocale } from 'currency-formatter'
import USD from 'currency-formatter/currencies/USD'
import en_US from 'currency-formatter/locales/en-US'

addLocale(en_US)

const customUserOverrides = { symbolOnLeft: false } // this is optional

format(1234, USD, customUserOverrides)
format(1234, USD)

Set a default for both the locale and currency:

import { format, addCurrency, addLocale } from 'currency-formatter'
import USD from 'currency-formatter/currencies/USD'
import en_US from 'currency-formatter/locales/en-US'

addCurrency(USD)
addLocale(en_US)

const customUserOverrides = { symbolOnLeft: false } // this is optional

format(1234, customUserOverrides)
format(1234)

We also have the flexibility of allowing the user to add as many override objects as (s)he wants. (Who knows, it might come in handy in some situations)

import { format, addCurrency, addLocale } from 'currency-formatter'
import USD from 'currency-formatter/currencies/USD'
import en_US from 'currency-formatter/locales/en-US'

addCurrency(USD)
addLocale(en_US)

const o1 = { symbolOnLeft: false }
const o2 = { thousandsSeparator: ',' }

format(1234, o1, o2)

Some comments

What bundler to use

Up to you. I have very little experience with frontend build tools and bundlers.

Do we create separate bundles for every formatting file?

I don't exactly know what you mean by that.

Do we publish the browser bundles to a CDN?

It would probably makes it easier for people to import it in their web app if they are not using a build tool. Is there an automated way to do this?

And lastly @edorivai if you like, I can add you as a maintainer, you've been a huge help :)

[edorivai]

Very welcome 😄 , happy to help.

Seems like the only thing open to discussion is the data structure. First off, my data structure proposal was mostly meant as an opening to this discussion. Perhaps it's worth it to forget the current data structure for a minute, and think about what would be ideal.

I actually came to the realization that we're currently mixing three types of information:

1. Currency information (code, symbol)
2. Currency formatting (symbolOnLeft, spaceBetweenAmountAndSymbol, decimalDigits)
3. Number formatting (thousandSeparator, decimalSeparator)

Point 1 and 2 are definitely within the scope of this project, but 3 is already solved by others. Actually, it's already been solved within the JS language itself: NumberFormat.

This is also the approach that react-intl takes for formatting numbers. They use the native EcmaScript Intl API. If we would use the Intl API, that would mean we can get rid of thousandSeparator and decimalSeparator altogether! And if a site doesn't care about controlling locale formatting, the browser will just format the numbers according to the user's browser settings, which seems like a proper default to me. Also, the responsibility of adding locales, and providing polyfills will be shifted towards the Intl Polyfill ecosystem. They have excellent documentation on how/when to polyfill, what to do in a Node environment etc. I think it would be great to leverage all the work that has already been done on number formatting; let's not reinvent the wheel!

An obvious downside would be that we lose control over thousandSeparator and decimalSeparator. But I cannot really think of real-world use cases where that would be a requirement.

Now if we choose to go down this road, we can get rid of the dependency on accounting.js, since the only formatting that we need to take care of is basically the placement of the currency symbol.

As for the data structure, as I see it, formatting information still splits into two types of files:

Currency:
{
	code: 'USD',
	symbol: '$',
	decimalDigits: 2
}

Locale:
{
	locale: 'en-US',
	symbolOnLeft: false,
	spaceBetweenAmountAndSymbol: true,
}

What's left is deciding on the default values for symbolOnLeft and spaceBetweenAmountAndSymbol. I'm unsure what the best approach is here. These are the options I see:

• Stick to the current setup, and keep the values for symbolOnLeft and spaceBetweenAmountAndSymbol that are currently defined in each currency object.
• Define a default locale per currency. As you mentioned, this will require some thought. One way to approach this problem would be to use locale-currency: localeCurrency.getLocales('USD') // [ 'AS', 'BQ', 'EC', 'FM', 'GU', 'IO', 'MH', 'MP', 'PR', 'PW', 'TC', 'TL', 'UM', 'US', 'VG', 'VI' ].
• Somehow detect the user's preferred locale, maybe something like this: http://stackoverflow.com/a/674570/971091. Although doing this is probably best left to the developer.

From these options I think I like the first the best. This serves projects that don't care so much about correct locale-specific formatting; they'll just have to specify the currency code. And we could easily provide a way to set the default locale.

So for the API, I think we agree on this: provide a way to set defaults for currency and locale, but allow to override with an arbitrary number of formatting objects for every call to format(). Exactly as you outlined before:

import { format, addCurrency, addLocale } from 'currency-formatter'
import USD from 'currency-formatter/currencies/USD'
import en_US from 'currency-formatter/locales/en-US'

addCurrency(USD)
addLocale(en_US)

const o1 = { symbolOnLeft: false }
const o2 = { thousandsSeparator: ',' }

format(1234, o1, o2)

I think I'll experiment a bit, to get stuff more concrete.

And lastly @edorivai if you like, I can add you as a maintainer, you've been a huge help :)

Sure, if that makes stuff easier for you 😉

[edorivai]

I made a quick 'n' dirty proof of concept: https://github.com/edorivai/currency-formatter/tree/rewrite

[edorivai]

Out of time now, but ran into a couple of issues which we would need to address. I'll write them up later.

[smirzaei]

Awesome! I was a little skeptical about using the Intl APIs but looks like it has everything we need and I'm all for removing dependencies 😁

Also I agree with the points you made... I really don't think anyone wants to override thousandSeparator and decimalSeparator and keep the values for symbolOnLeft and spaceBetweenAmountAndSymbol

With this design in mind... I think we can delete our locale data as well.

[edorivai]

I think we can delete our locale data as well.

I'm not following you here. Locale data is useful if a single currency is displayed differently across locales. The Euro is a very good example for this: €12,34 in nl-NL, 12,34 € in de-DE. This is the whole reason why I wanted locale data in the first place. I mean, I could easily maintain my own locale specific library of formats for the locales we support in our project, but these formats are so general, that I think it's a good idea to take them into this library.

And then there is the use case provided by @Gwened (formatting currencies that don't naturally correspond to a locale):

For example, I think Canadians speaking French will use 5$ while English speakers write $5.
In a similar way, if I'm targeting brits traveling in Europe, I would write €5 instead of 5 €.

This is why I think it's useful to have these locale formats which are only specifying symbolOnLeft and spaceBetweenAmountAndSymbol.

[smirzaei]

Never mind you are right. I thought we wouldn't need that information since we just pass the locale name when instantiating a new NumberFormat but we still need to read symbolOnLeft, spaceBetweenAmountAndSymbol (maybe more?) properties from our locale data.

[edorivai]

I thought we wouldn't need that information since we just pass the locale name when instantiating a new NumberFormat

This would be true if we could use { style: 'currency' } on the NumberFormat. I mean, you can, and probably a lot of people should if they don't care so much about correctness and consistency of the format.

maybe more?

I wouldn't know what else. I would still like to support passing custom a format, like we do now:

    {
      pos: '%v %s',
      neg: '(%v %s)',
      zero: '%v %s'
    }

[smirzaei]

No I don't think it's a good idea. The only point of using this library over directly using Intl APIs is to have consistent formatting across all browsers. Also NumberFormat with { style: 'currency' } sometimes returns values that are (while correct) weird to see. For example USD $123.00 or CA$123.00

[edorivai]

Aha. Yeah I guess the spec and implementations need to mature. I'll be happy when this library is completely obsolete 😄. I'll see when I can find some extra hours to polish this proof of concept, and write up the issues that we need to work out.

For now a quick one:
You outlined an API that allows an arbitrary number of configs to be passed like so:

import { format } from 'currency-formatter'
import USD from 'currency-formatter/currencies/USD'
import en_US from 'currency-formatter/locales/en-US'

const customUserOverrides = { symbolOnLeft: false } // this is optional

format(1234, USD, en_US, customUserOverrides)
format(1234, USD, en_US)

I really like this, but I also like it if users can just specify the currency and/or locale code:

format(1234, { currency: 'USD', locale: 'fr-CA' })

What would be a good API?

format(1234, { currency: 'USD' }, { symbolOnLeft: false }) // If you want to specify a custom format, you have to specify an options object too
format(1234, { symbolOnLeft: false }, { currency: 'USD' }) // The last argument is always interpreted as an options object
format(1234, { currency: 'USD', symbolOnLeft: false }) // Every argument can contain formatting options, as well as `currency` and `locale` settings.

Food for thought 😄

[smirzaei]

Hmm maybe have a different function if the user just wants to pass the currency code:

formatByCurrencyCode(1234, 'USD', { symbolOnLeft: false }) // Couldn't come up with a better name :D

I think that would be the case when the currency is dynamic so a more consistent approach would be this:

import { format } from 'currency-formatter'
import currencies from 'currency-formatter/currencies'
import en_US from 'currency-formatter/locales/en-US'

const customUserOverrides = { symbolOnLeft: false } // this is optional

format(1234, currencies['USD'], en_US, customUserOverrides)
format(1234, currencies['USD'], en_US)
format(1234, currencies['USD'])

[edorivai]

I like the idea of a different function!

a more consistent approach would be this

That's indeed more consistent, but I like to have an alternative API where only the currency and locale code are specified. My biggest concern with your last snippet is that this requires users to basically add 3 import lines to each file where they need to format a currency. The global addCurrencies() that we discussed earlier in combination with formatByCurrencyCode is IMHO a must in the API.

That said, if we choose to go the formatByCurrencyCode route, there is nothing stopping users from only using the format() syntax.

[smirzaei]

Yeah you are right I didn't think about the global addCurrencies and addLocales functions. If you think we should have a simpler function for people to use formatByCurrencyCode then we definitely should have it ;)

The reason I'm not a big fan of it being in the format function is when I think about the signature of its parameters, it has to be consistent. (Coming from a static typed background alarms are going off in my head 😁)

If it only takes an object shaped like a currency then we know what we are expecting in that function and the user knows what kind of object they need to pass. But if we add an extra currency: 'USD' property, then that function gets more responsibility.

That would allow the user to construct an object like this

{
   "currency": "EUR",
    "code": "USD",
    "symbol": "$",
    "thousandsSeparator": ",",
    "decimalSeparator": ".",
    "symbolOnLeft": true,
    "spaceBetweenAmountAndSymbol": false,
    "decimalDigits": 2
  }

Obviously I don't think anyone would do that 😁 but technically this would be a valid parameter and the worst thing about it is you can't guess what the output is going to be...

Are we going to load EUR currency and use that and throw everything else away (which would format the number in EUR style even though the user wanted a different formatting) or use all the formatting properties that have been set (which would format the number in USD style but the user thought they requested EUR) you see the point I'm trying to make is that if we add optional and custom properties, we are making our code more complex and opening a door for user to make errors.

Now with that in mind, I really want to have support for #31 because I think a lot of people want that but don't know where is the best place to set that option :p

[edorivai]

I completely agree, and share your worries. That's why I wanted to discuss this, my alarm bells are going off at my current implementation 😝

I think we want a fixed order in which configs are applied. This one makes sense to me:

1. Currency
2. Locale
3. Custom overrides

Actually, I'm now starting to feel better about:

format(1234, { currency: 'USD' }, { symbolOnLeft: false })

I think the most common use cases are (in order):

format(1234) // After setting default currency/locale
format(1234, { currency: 'USD' }) // Granular control over basic rendering options (currency/locale/omitZeroes)
format(1234, null, { symbolOnLeft: false }) // Granular control over the formatting

I would say passing null for the options object is the biggest downside to this API. However, this only applies to the last use case, which I expect to really by an edge case.

[smirzaei]

My point was that the format function should only accept a Currency object and { currency: 'USD' } doesn't really fit.

I'll try to create an example a bit later so we can discuss it better :)

[edorivai]

I understood that, and I agree that we should make the function parameters explicit. For me this would be explicit enough:

format(value, options, ...customConfigs)

Where options and customConfigs are optional.

[burrack]

are you still think to rewrite everything ?
also, the API will change ?

[nunocf]

I'm willing to give a hand to get this library rewritten and a bit more customizable if you need any more colaborators.
Send me a message if you're looking for help.
This is an extremely valuable piece of code that helps lots of people. :)