jonathanpmartins / v-money3

Vue3 currency input/directive mask
MIT License
103 stars 27 forks source link

Return number when mask = false #27

Closed sougiovn closed 3 years ago

sougiovn commented 3 years ago

https://github.com/jonathanpmartins/v-money3/blob/a3d0ff7014926abed16873e6519d8db7231a2b31/src/utils.js#L108

I was expecting to receive a number type when setting masked: false, but instead it returns a string.

Shouldn't it return a number?

jonathanpmartins commented 3 years ago

You are rights. Will parse it to a float in the next release.

jonathanpmartins commented 3 years ago

Fixed in the v3.14.3 version.

https://github.com/jonathanpmartins/v-money3/blob/f5731be3a79f0f67c1455e7e05b2ad61dd5a174b/src/utils.js#L133

Thanks for reporting!

AndrolGenhald commented 3 years ago

Not a big issue but I wonder if it'd be better to check for the v-model.number modifier before parsing to a float? It's possible that decimal values could have precision problems and some users may want to always keep currency in fixed precision formats.

jonathanpmartins commented 3 years ago

So this number modifier would be a boolean? True by default. If it is false, it will return the fixed string, otherwise the parsed number.

AndrolGenhald commented 3 years ago

I was referring to the v-model modifier: https://v3.vuejs.org/guide/forms.html#number I haven't tested it, but it actually looks like it should work without having to do anything in the component itself: https://github.com/vuejs/vue-next/issues/2326 I think this would be the best way to handle it so it's more consistent with the way normal number inputs work.

@gigioSouza Do you think it's reasonable to expect users to use v-model.number="" to get a Number, with the default being a string?

sougiovn commented 3 years ago

@AndrolGenhald actually I myself would rather use Number by default and create a .masked modifier for when I need want the value to be masked.

I want the component to present my numeric the data with a mask, so I want to deal with numbers by default and opt-in if I want to handle the masked value.

I'll fork and propose a Pull Request about it.

jonathanpmartins commented 3 years ago

I will reopen this until we ship a new release about the changes discussed until now.

jonathanpmartins commented 3 years ago

I think the main goal here is to define what would be the default return type when masked is false. String or Number?

I like to follow what Vue has to offer. This is how Vue deals with the problem and solving it with the number modifier will make sense. Besides, it makes the API look pretty! Default is a fixed string, use the modifier is you need a number.

On the other hand, I think that the default return type should be a number because this is a money mask. The prime focus is to deal with numbers. So it makes sense too, to have the default return type to be a number.

We could even use a property to solve this, but something like returnString or returnNumber will make the API look ugly. hehehhe

sougiovn commented 3 years ago

@jonathanpmartins I was testing the second approach.

So the config masked isn't needed anymore, and instead of it, users should use the .masked modifier:

price is a number by default

<money3 v-model="price" :config="config" />

price is a masked string due to the .masked modifier

<money3 v-model.masked="price" :config="config" /> 
jonathanpmartins commented 3 years ago

The masked property is needed for cases where you want the complete masked value to be returned. Changing its behavior could brake user applications. So should the unmasked number, when masked is false, by default, be a float or a fixed string?

If the implementation changes in favor of the number modifier the default behavior will return a typeof string: '1234.56'

<money3 v-model="price" /> 

With the number modifier it will return a typeof number: 1234.56

<money3 v-model.number="price" /> 

If you really need a float after this change, the annoying refactoring would be change all your v-model= to v-model.number=.

sougiovn commented 3 years ago

Currency data are used for calculation, it bothered me having to parse the data back to numbers.

I think it doesn't make sense to return a string with fixed floating number. I see the proposed change would break users applications, therefore it should be a new major version release.

jonathanpmartins commented 3 years ago

Perhaps we should introduce a new property called parsed. It is a boolean, true by default, and will only take effect when masked is false.

This will not create a break change and will introduce the possibility to have a fixed string returned. What do you think about it?

AndrolGenhald commented 3 years ago

I think the main goal here is to define what would be the default return type when masked is false. String or Number?

Correct, eg whether to return 1234 or "1234.00", not whether to return 1234 or $1,234.00.

Currency data are used for calculation, it bothered me having to parse the data back to numbers.

I mostly agree, but the reason I bring it up is because many applications that deal with currency want fixed precision, and having to convert from a float back to a string is also annoying, with the additional annoyance that in some cases the floating point format could cause a loss of precision. It's unlikely to affect most use cases since JavaScript uses a 64 bit floating point format, but it's still possible.

I see the proposed change would break users applications, therefore it should be a new major version release.

Probably, but the same could be said of the previous change to return a number instead of a string.

I like to follow what Vue has to offer. This is how Vue deals with the problem and solving it with the number modifier will make sense. Besides, it makes the API look pretty! Default is a fixed string, use the modifier is you need a number.

This is the other reason I lean more towards wanting it to return a string, it's the same way other inputs work. It's the same way a type="number" input works.

Perhaps we should introduce a new property called parsed.

That seems reasonable. Using v-model.number seems more elegant to me, but if you prefer this to avoid breaking changes I'm ok with that.

jonathanpmartins commented 3 years ago

I'm much more inclined to use the number modifier. It's about elegance and about following Vue patterns.

I don't think that this refactoring (v-model= to v-model.number=) will do much damage. I will try to document it fairly.

sougiovn commented 3 years ago

I mostly agree, but the reason I bring it up is because many applications that deal with currency want fixed precision, and having to convert from a float back to a string is also annoying, with the additional annoyance that in some cases the floating point format could cause a loss of precision. It's unlikely to affect most use cases since JavaScript uses a 64 bit floating point format, but it's still possible.

The only scenario I can think of losing precision is: Data receives a number with higher precision than the one configured in the mask, then the mask corrupts the data.

AndrolGenhald commented 3 years ago

The only scenario I can think of losing precision is: Data receives a number with higher precision than the one configured in the mask, then the mask corrupts the data.

64-bit floating point values can store 15-17 decimal digits before losing precision. If you want to have tens of trillions, that's 14 digits, add in cents and it's 16, which could start to lose precision. Numbers this high don't show up too often in US dollars, but you can get values like that when talking about national budgets. I'd guess in other countries with lower valued currencies it could be worse.

jonathanpmartins commented 3 years ago

I see the proposed change would break users applications, therefore it should be a new major version release.

I will not release a major 4.0 because I don't think it will ever be one. The idea is to follow Vue on their major release numbers. If Vue decides to release a major 4.0 version tomorrow I will probably create a new repository/project called v-money4.

This is what I was planing to do until now. That said, now that I think more about it, I feel that I need to plan a project rename to accomplish modern standards. Will be much better to keep everything in one repository for so many reasons....

Or we can forever stay v-money3 and decouple the number 3 from the version releases. But a 4.0 or 5.0 release of v-money3 will be strange.

For the time being I will release a 3.16.0 version with the changes discussed above.