faker-js / faker

Generate massive amounts of fake data in the browser and node.js
https://fakerjs.dev
Other
12.98k stars 919 forks source link

Commerce price does not return fractional currency prices #350

Closed ST-DDT closed 8 months ago

ST-DDT commented 2 years ago

Describe the bug

faker.commerce.price() always returns a value with full price. => 245.00 , 842.00

It would be nice if it could also generate prices with fractional currency prices => 245.71 , 842.45

Also if invalid prices ranges are given, then the returned prices does not respect the decimal places paramter.

Reproduction

faker.commerce.price() => 245.00 faker.commerce.price(-1) => 0

Additional Info

No response

ST-DDT commented 2 years ago

@Shinigami92 Would you consider this a bug or feature request?

Shinigami92 commented 2 years ago

I think the precision could be a bug, but maybe it was considered as wanted cause if you have e.g. an app that shows some prices, the prices are just xx.00

What I think would be a better idea is to directly create a whole new module for currency :thinking: Also we should take stuff into account like the symbol could be placed on the right or left side: $12.50 or 12,50 €


I think we could do 2-3 PRs targeting different versions, on the other side we should ask some folks if the .00 is expected currently :thinking:

Shinigami92 commented 2 years ago

We could also try to play around with https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Intl/NumberFormat#using_options :eyes:

damienwebdev commented 2 years ago

A comment here on pricing, it's not safe to assume that all prices are formatted to two decimals.

E.g. in the US, prices for gasoline are $2.999/gallon.

Even within a singular currency, there are inconsistencies.

import-brain commented 2 years ago

I think the precision could be a bug, but maybe it was considered as wanted cause if you have e.g. an app that shows some prices, the prices are just xx.00

What I think would be a better idea is to directly create a whole new module for currency 🤔 Also we should take stuff into account like the symbol could be placed on the right or left side: $12.50 or 12,50 €

I think we could do 2-3 PRs targeting different versions, on the other side we should ask some folks if the .00 is expected currently 🤔

Yes, I think a new module would be the best approach here, too.

import-brain commented 2 years ago

@Shinigami92 should this be put under v6.2 or v7? thanks

Shinigami92 commented 2 years ago

To get some more prio to other features, I think we can put this into v7 for now

pkuczynski commented 2 years ago

I would suggest dropping this function as we have datatype.float, which takes the precision parameter. WDYT?

ST-DDT commented 2 years ago

IMO we should localize the output some more.

E.g. de -> 1,99€ en_us -> $1.99

pkuczynski commented 2 years ago

I think currency should not come with a price. I might want to stay on pl-PL locale and still generate prices in €... Should I switch faker to de just to get the symbol? Don't think so. Currency symbols should not be based on locale. $ is the same in any locale. We can add faker.money.currency or faker.commerce.currency. We should also support short and long. However I think for adding currency users should use intl module (https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Intl/NumberFormat/NumberFormat)

import-brain commented 2 years ago

I would suggest dropping this function as we have datatype.float, which takes the precision parameter. WDYT?

I'm not sure, I could honestly go either way on this. We could have this function return values with currency symbols, which would be nice, but datatype.float still pretty much does the same thing, and users could use intl to add the symbols.

ST-DDT commented 2 years ago

I haven't worked with that api yet, but it looks straight forward. Is it well known in the JS community? If yes, then we can drop price. If not, then we should consider how we can guide our users to that function/API (Maybe retaining the method as alias to float, but with extended Jsdocs.

Shinigami92 commented 2 years ago

https://caniuse.com/?search=intl%20currency

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Intl/NumberFormat#browser_compatibility

pkuczynski commented 2 years ago

@Shinigami92 what would be your suggestion then? price returns the string and fills with 0 if not enough precision is generated. Additionally, it offers to add a currency sign.

https://fakerjs.dev/api/commerce.html#price

Shinigami92 commented 2 years ago

@Shinigami92 what would be your suggestion then? price returns the string and fills with 0 if not enough precision is generated. Additionally, it offers to add a currency sign.

fakerjs.dev/api/commerce.html#price

I must say, I don't know I currently don't have strong opinions of how to design this or what would be good UX/DX Just wanted to link on which platforms Intl can be used and it looks like we are safe with Intl because it runs in Node and all ever green browsers. Maybe we could wrap it and use it in Faker itself? Maybe we move the headaches to the user itself 🤷😅 I can assume that the commerce.price functionality is and was a good usable function to directly get what you asked for and show e.g. some fake prices in your mocked web-store. I don't and never did in the past worked with a store, so I don't have any experience on that section.

Shinigami92 commented 2 years ago

Team decision

We want to change the implementation of the price method to use Intl directly/internally

Minozzzi commented 2 years ago

I would like to try to solve this task. Could assign to me?

ST-DDT commented 9 months ago

FFR: We decided (some time ago) that we tackle the issue as is in #2458 and rethink the implementation later in #2579