Full rewrite proposal about currency-formatter HOT 18 OPEN

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 :)

from currency-formatter.

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 😉

from currency-formatter.

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

from currency-formatter.

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

from currency-formatter.

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.

from currency-formatter.

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.

from currency-formatter.

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.

from currency-formatter.

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'
    }

from currency-formatter.

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

from currency-formatter.

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 😄

from currency-formatter.

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'])

from currency-formatter.

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.

from currency-formatter.

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

from currency-formatter.

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.

from currency-formatter.

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 :)

from currency-formatter.

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.

from currency-formatter.

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

from currency-formatter.

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. :)

from currency-formatter.