[Feature request]: Same currency of different case is allowed and is considered as a different currency

[dearlordylord]

Is there an existing issue for this?

• [X] I have searched the existing issues

Current behavior

Dinero({
    currency: 'usd' as 'USD',
    amount: 1
}).add(Dinero({
    currency: 'USD',
    amount: 1,
}))

would throw You must provide a Dinero instance with the same currency

Expected behavior

Either don't allow lowercase usd (according to DefinitelyTyped types) or auto convert or allow .add on different-cased currencies

Steps to reproduce

https://codesandbox.io/s/fervent-khorana-fezl6p?file=/src/index.js

Version

1.9.1

Environment

Node 16.15.0

Code of Conduct

• [X] I agree to follow this project's Code of Conduct

[KenyStev]

@sarahdayan I may work on this, we just need to define the acceptance criteria: either allow diff case of enforce upper case

[sarahdayan]

Hi @Firfi, what's your use case? Handling this at the library adds overhead. Why not capitalize strings in your app?

[dearlordylord]

Hi @sarahdayan , thank you for checking this one

The use case is: the inner representation of currency in our app conforms to Dinero, but we work with Stripe, which returns lowercase currencies. It recurringly causes runtime errors when new code or features are implemented.

Why not capitalize: I am capitalizing in the app currently but it has proven to be problematic onboarding middle developers on this codebase: each of them has to be explained and has to memoize this special case.

One of the developers even recently asked me: "Why won't the library that we use to encapsulate the work with currencies makes us work with currencies? Doesn't it add overhead to our app that the library is supposed to free us from?". I sincerely had nothing to say to that.

[johnhooks]

My initial thought was that this doesn't necessarily need to be fixed at a library level. But I thought maybe I should test it. It looks like it's much more performant to test multiple possibility rather than upcase everything or check a regex Though it would require creating pretty elaborate conditional statements.

perf.link

[dearlordylord]

Testing a Capitalized input (Usd/Eur/Etc...) and a Lowercase input (usd/eur/etc...) should be enough, in my opinion. If users have the input like uSd I'd say they brought it on themselves

[johnhooks]

The currency library either gets larger in size, comparison get slower, or it remains an implementation issue.

[sarahdayan]

"Why won't the library that we use to encapsulate the work with currencies makes us work with currencies? Doesn't it add overhead to our app that the library is supposed to free us from?". I sincerely had nothing to say to that.

@Firfi I understand it can be challenging to answer such questions, so thanks for your patience. Let me explain my point of view and see if this helps your team change perspective.

Dinero.js, as a library, abstracts complexity for you regarding money concerns. It enforces contracts such as providing amounts as integers because it encapsulates the concern of floating point math in JavaScript. It also enforces using domain objects and not just numbers because it aims to represent money as a concept accurately.

Overhead is a cost that is deemed necessary. We try to avoid it as much as possible, but sometimes it's better not to. An example is how we normalize objects before performing any mutation to ensure they have the same scale. This overhead is necessary because dealing with objects of different scales in an app makes sense from a domain perspective, and you shouldn't have to think about it.

Uppercasing input currency code is small overhead, meaning it won't cost much performance-wise, whether in your app or the library. However, this caters to a particular use case—using inputs from Stripe.

The library aims to serve as many teams as possible, covering most use cases but also knowing when to keep project-specific use cases out of the core. Not everyone who uses Dinero.js uses Stripe, but everyone is impacted if we capitalize input currency code systematically at the library level.

Another issue is that such behavior is implicit. Nothing hints you that the library could be doing that, and even if we document it, it becomes a gotcha. Implicit behaviors are often harmful and result in nasty bugs if teams don't realize what happens under the hood. It forces everyone to learn more about the library itself instead of reinforcing their knowledge of their own data and services.

Now, let's say we were to abstract this complexity in the library because we think Stripe has established itself enough that we can't ignore it. Even in this case, it would certainly not be at the object creation level but in a utils package with a factory like fromStripe that would take a Stripe object and transform it into a Dinero object. It's more proper from a separation of concerns perspective. It also makes app code much more expressive when you read it and removes any implicit knowledge.

For now, Dinero.js doesn't provide factory utils, but we could someday. In the meantime, I recommend adding a fromStripe or fromStripeToDinero factory to your codebase and communicating with your team that when dealing with Stripe inputs, they should use this function and not dinero directly.

function fromStripe(stripeObject) {
  return dinero({
    amount: stripeObject.amount,
    currency: {
      base: 10,
      code: stripeObject.currency.toUpperCase(),
      exponent: 2,
    },
  });
}

If using two functions is a problem, you could even forbid using dinero directly (with an ESLint rule for example) and enforce using a createDineroObject function that handles Dinero options or Stripe objects.

function createDineroObject(stripeObjectOrDineroOptions) {
  if (typeof stripeObjectOrDineroOptions.currency === 'string') {
    // We're dealing with a Stripe input
    return dinero({
      amount: stripeObject.amount,
      currency: {
        base: 10,
        code: stripeObject.currency.toUpperCase(),
        exponent: 2,
      },
    });
  }

  // We're dealing with Dinero options
  return dinero(stripeObjectOrDineroOptions);
}

If you're using TypeScript in your project, I plan to make Currency.code generic so that the compiler would further prevent using incompatible objects. It would keep any team, yousr included, from passing conflicting inputs.

At the end of the day, it's all about arbitrations, and the line is sometimes blurry. I understand this moves the burden to your codebase right now, but even if this was a Dinero.js concern handled at the library level, the code you'd have to write would be the same—it would just be packaged and tested outside your codebase.

Does it make sense?

[dearlordylord]

Thank you @sarahdayan for taking the time to explain this; this makes perfect sense to me. I went the way of restricting direct import + doing a wrapper over the lib, which is our default approach anyways.

Please note that I don't uppercase and haven't been proposing this. My current approach is fail-fast in the case of currency: 'usd' as 'USD',, i.e. when someone would like compile-time. We keep case casts explicit.

I just wish the lib was doing it as well (failing on the "lowercase lies") but I understand that it could cause backward compat issues without bringing much value 🙂