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

Return number when mask = false #27

Closed
sougiovn opened this issue Jul 31, 2021 · 17 comments
Closed

Return number when mask = false #27

sougiovn opened this issue Jul 31, 2021 · 17 comments
Labels
good first issue Good for newcomers

Comments

@sougiovn
Copy link
Contributor

sougiovn commented Jul 31, 2021

return Number(currency).toFixed(fixed(opt.precision));

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
Copy link
Owner

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

@jonathanpmartins
Copy link
Owner

Fixed in the v3.14.3 version.

return parseFloat(output);

Thanks for reporting!

@jonathanpmartins jonathanpmartins added the good first issue Good for newcomers label Aug 2, 2021
@AndrolGenhald
Copy link
Contributor

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
Copy link
Owner

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
Copy link
Contributor

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: vuejs/core#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
Copy link
Contributor Author

sougiovn commented Aug 9, 2021

@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
Copy link
Owner

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

@jonathanpmartins
Copy link
Owner

jonathanpmartins commented Aug 9, 2021

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
Copy link
Contributor Author

sougiovn commented Aug 9, 2021

@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
Copy link
Owner

jonathanpmartins commented Aug 9, 2021

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
Copy link
Contributor Author

sougiovn commented Aug 9, 2021

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
Copy link
Owner

jonathanpmartins commented Aug 9, 2021

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.

  • When its set to true it will return a parsed float number.
  • When its set to false it will return a fixed string.

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
Copy link
Contributor

AndrolGenhald commented Aug 9, 2021

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
Copy link
Owner

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
Copy link
Contributor Author

sougiovn commented Aug 9, 2021

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
Copy link
Contributor

AndrolGenhald commented Aug 10, 2021

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 added a commit that referenced this issue Aug 10, 2021
jonathanpmartins added a commit that referenced this issue Aug 10, 2021
jonathanpmartins added a commit that referenced this issue Aug 10, 2021
…oat number even if masked property is true
jonathanpmartins added a commit that referenced this issue Aug 10, 2021
@jonathanpmartins
Copy link
Owner

jonathanpmartins commented Aug 10, 2021

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers
Projects
None yet
Development

No branches or pull requests

3 participants