Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Passing decimals as amount #58

Closed
jsardev opened this issue Mar 14, 2019 · 25 comments
Closed

Passing decimals as amount #58

jsardev opened this issue Mar 14, 2019 · 25 comments
Labels
enhancement New feature or request

Comments

@jsardev
Copy link

jsardev commented Mar 14, 2019

Is your feature request related to a problem? Please describe.

So if you would like to integrate with services like Braintree (or any other service that uses decimals instead of minor currency values) you will have to manually convert those decimals/strings to integers just to make it compatible with Dinero.

Describe the solution you'd like

It would be great if I could just pass a string like 499.99 into Dinero and it would convert it into 49999. This could be achieved for example with a fromFormat function which would take a string mask argument and a value: fromFormat('0.00', '499.99').

Describe alternatives you've considered

Manual conversion.

@jsardev jsardev added the enhancement New feature or request label Mar 14, 2019
@sarahdayan
Copy link
Collaborator

Hey @sarneeh and thanks for the suggestion. You're not the first one to ask and it could indeed be interesting to add this, maybe as a static method or as a separate factory (to keep the bundle size untouched for those who don't need it).

I guess it could auto-detect the precision based on the number of digits after the decimal point, which would be even simpler. Something like:

DineroFactory.fromFloat({ amount: 499.99, currency: 'EUR' })

Would that help?

@jsardev
Copy link
Author

jsardev commented Mar 17, 2019

@sarahdayan Indeed! This idea is a lot better as it keeps the Dinero API consistent! That would definitely help 😃

I could also implement this, but unfortunately not in the nearest 1-2 months (gotta fight deadlines). If you don't mind this late date you could keep this up here for me 😃

@sarahdayan
Copy link
Collaborator

@sarneeh In this case we can share the load and get this out sooner: I can implement it and you could review (should demand less time than implementing). Would this be doable for you?

@jsardev
Copy link
Author

jsardev commented Mar 17, 2019

@sarahdayan I'll find time for a review for sure! Thank you!

@jadamduff
Copy link

Very excited about this feature. I'd love to be able to pass in strings and have them parsed into the object. Ex: "$150" -> {amount: 15000}

@sarahdayan
Copy link
Collaborator

@jadamduff This would be the job of another factory, such as fromString. But this involves a lot more work.

@jadamduff
Copy link

@sarahdayan okay thank you

@captDaylight
Copy link

I'd be interested in taking this on @sarahdayan if you haven't started already.

@fish4j
Copy link

fish4j commented Apr 18, 2019

Can we simply use Dinero({ amount: Math.round(499.99 * Math.pow(10,minor unit)) })?

@sarahdayan
Copy link
Collaborator

@captDaylight On it already, but thanks for offering your help! Would love it if you want to review.
@fish4j Yes, this is a factory that you can definitely build for yourself but which involves that you pass the precision (minor unit in your snippet) by hand. The idea behind fromFloat is to detect it.

@blacktrue
Copy link

Hi @sarahdayan

The factory with fromFloat/fromString methods is already available? I do not find anything about it

Thanks

@sarahdayan
Copy link
Collaborator

@blacktrue Hey, I'm still working on it, but got delayed 🙂 If you subscribe to notifications for this issue you'll get pinged once the PR that fixes it is merged and the version that embarks it is deployed.

@ishegg
Copy link

ishegg commented Jun 10, 2019

@sarahdayan hey, are you in need of a helping hand? If you're swamped with work I might be able to give it a try.

From a first thought, it seems that something like

fromString(amount, currency) {
  var precision = amount.split('.')[1].length;
  var amountInMinimalUnit = Math.round(Number(amount) * Math.pow(10, precision));
  return Dinero({
    amount: amountInMinimalUnit,
    currency: currency,
    precision: precision
  })
}
fromFloat(amount, currency) {
  var precision = amount.toString().split('.')[1].length;
  var amountInMinimalUnit = Math.round(amount * Math.pow(10, precision));
  return Dinero({
    amount: amountInMinimalUnit,
    currency: currency,
    precision: precision
  })
}

in services/static.js would do it. (This is missing pre/postcondition assertions)

@sarahdayan
Copy link
Collaborator

Hey @ishegg, thanks for offering your help! I'm indeed swamped with work, had to put this on the back burner. Feel free to PR 🙂

@sarahdayan
Copy link
Collaborator

Hey! I just published a RFC for Dinero.js v2 🚀 It tackles this specific problem.

I would love to have your feedback on it: dinerojs/rfcs#1

cc @ishegg @sarneeh @jadamduff @captDaylight @fish4j

@Huespal
Copy link

Huespal commented Jul 26, 2019

I am looking exactly for this. When will be released? Thanks.

@bugproof
Copy link

bugproof commented Sep 19, 2019

wouldn't it be better to integrate some other library like decimal.js or bignumber.js or big.js? or how exactly can you use dinero if you're using one of those libraries? If I understand correctly dinero doesn't use any of them and is using integers to do decimal calculations?

@jsardev
Copy link
Author

jsardev commented Sep 20, 2019

@bugproof You probably don't want to use multiple money formatting libraries, as it's only adds unnecessary complexity. The case of this issue is to be able to more easily integrate dinero with different money formats used in available payment gateway (or other) APIs.

@sarahdayan
Copy link
Collaborator

@Huespal I'm currently not able to tackle this because of work, so I can't give you an ETA. Feel free to PR if you need it ASAP, I can take time to review.

@bugproof There are several things to consider. First, it's much better to use non-floats (as number or bigint) because they're more reliable and performant when it comes to performing math on them. Dinero used number so far because bigint weren't available, but now that they are it's going to become an option in v2. Secondly, the point of this issue is to create factories so you can provide a format that is convenient for you. It wouldn't change the internals of Dinero, and what type(s) it uses. Finally, regarding libraries, I definitely don't want to create any coupling with a third-party, nor use any as a dependency. I think Dinero should stay dependency-free, and as agnostic as possible. However, writing adaptors so you can provide entities created with third-parties could be an idea, and Dinero v2 could definitely provide an API for this.

@bugproof
Copy link

bugproof commented Nov 15, 2019

it's problematic when you have API that returns decimal values like 666.66 how can you pass it as amount to dinero? multiply by 100?

Dinero({amount: 666.66*100}

@johnnyperdomo
Copy link

Any updates on this?

@ghost
Copy link

ghost commented Jun 24, 2020

Hi , I just discovered dinero.js and I think this issue is very necessary because most of the time money is used with text-masking and the value returned from masking doesn't come in cents.

For example currently I use react text-mask addon and when I give 5000 cents Dinero object as input value I get back $50.00 as string. My workaround is to remove the currency, then parse the float, then multiply by 100 (while controlling the minimum sub unit for the currency).

I think these are necessary..I will try doing these in my fork

DineroFactory.fromFloat({ amount: 499.99, currency: 'EUR' }) 
DineroFactory.fromString({ amount: "499.99", currency: 'EUR'  })

this is 'nice to have'
DineroFactory.fromCurrencyString({ amount: "$499.99", currency: 'USD' })

Edit

I couldn't make the library work with the change in static.js similar to what @ishegg stated but in my React code I made the parsing work with masking as below.


import MaskedInput from 'react-text-mask';
import createNumberMask from 'text-mask-addons/dist/createNumberMask';
import getSymbolFromCurrency from 'currency-symbol-map';

let _currencySymbol = '';
let currencyMask = [];
_currencySymbol = getSymbolFromCurrency(business.currency);
currencyMask = createNumberMask({
  prefix: _currencySymbol,
  allowDecimal: true,
  allowNegative: true,
  includeThousandsSeparator: false,
  suffix: ''
});

const parseCurrencyString = (amount, currency)=> {
  var currencyString = amount.replace(_currencySymbol,'')
  var precision = 2 //in my case it's fixed to 2
  var amountInMinimalUnit = Math.round(Number(currencyString) * Math.pow(10, precision));
  return Dinero({
    amount: amountInMinimalUnit,
    currency: currency,
    precision: precision
  })
}

const handlePricingItemChange = (e, classname, idx = null) => {
  let pricingitems = values.pricing;
  pricingitems[idx][classname] = parseCurrencyString(e.target.value, business.currency).getAmount()
  setFieldValue('pricing', pricingitems);
};

<div style={{ width: 105 }} key={`supply_price-${row.idx}`}>
  <MaskedInput
    name={`supply_price-${row.idx}`}
    id={`supply_price-${row.idx}`}
    key={`supply_price-${row.idx}`}
    mask={currencyMask}
    className="form-control"
    value={Dinero({ amount: values.pricing[row.idx].supply_price}).toFormat('0.00') }
    onBlur={e => handlePricingItemChange(e, 'supply_price', row.idx)}
  />
</div>

@nickpalmer
Copy link

Why not allow a { unit: 499.99 } on construction that does:

  if (o.unit) {
    o.amount = Math.round(o.unit * Math.pow(10, o.precision || defaultPrecision));
    delete o.unit;
  }

Users of { unit: xxx } are going to be aware that they are coming from a representation that is imprecise, and rounding is likely to occur.

@sarahdayan
Copy link
Collaborator

Hey! I just released Dinero.js v2 in alpha. It doesn't provide decimal support out of the box, but I've written a guide on how to do it userland. The new design makes it straightforward to extend the library.

I would still recommend being cautious with floats, and make sure what you're passing is really the amount you intended.

@ScreamZ
Copy link

ScreamZ commented Jul 17, 2021

Looks like I'm coming at perfect timing ✌🏻

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests