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

toNumber is lenient #2598

Closed
jods4 opened this issue Nov 12, 2020 · 13 comments
Closed

toNumber is lenient #2598

jods4 opened this issue Nov 12, 2020 · 13 comments
Labels
need guidance The approach/solution in the PR is unclear and requires guidance from maintainer to proceed further.

Comments

@jods4
Copy link
Contributor

jods4 commented Nov 12, 2020

Version

3.0.2

Steps to reproduce

toNumber (defined in @vue/shared) uses parseFloat, which is very lenient and will succeed if the first non-whitespace character is a digit (or sign).

So toNumber("1234abc") will succeed and return 1234.

toNumber is used by the .number modifier, so this surfaces in very observable ways to the end-user.

A simple alternative would be Number("1234abc"), which does fail if the complete string isn't a valid number. Like parseFloat it does accept spaces at the beginning and end of string.

What is expected?

<input v-model.number="x" />

If I input "12abc", given the behavior of v-model.number, I expect x to be the string 12abc, which can then be handled as a validation error in user code.

What is actually happening?

12 is assigned to x.

EDIT Another defect caused by this implementation is that a textbox is not stable and its content changes unexpectedly for the end user.
If you have a textbox <input v-model.number=x> and type abc123, then the text stays the same when the component is updated. But if you type 123abc then the content is truncated to 123 when the component updates.

@posva
Copy link
Member

posva commented Nov 12, 2020

I would say this is expected. It is consistent with Vue 2. Changing it would create trouble for users when migrating.
If you want to handle the string yourself, then it's better not to use the v-model number modifier and use a computed property instead to format the value as you wish.

You can also use custom v-model modifiers to create a behavior that better fits your specific needs.

@jods4
Copy link
Contributor Author

jods4 commented Nov 12, 2020

Migration might be a concern, yes.

Although I don't see how this could be a "feature". It's strange that typing - 2 doesn't parse but 1.2.3 does.
As it is, I'd say the modifier is only useful in contexts where you know this input will be correct and you don't care about error handling.

I am working around this by not using the modifier, as you suggest. If you need this repeatedly it's annoying.
That's where pipes shined, but they were removed from Vue 3.

Could you please tell me more about custom v-model modifiers? I am very interested into those (e.g. a .null, .date and more) but had the impression that Vue did not support them?

@vhoyer
Copy link

vhoyer commented Nov 12, 2020

I'd say this being javascript it's consistent with javascript behaviour and I agree with @posva that one should just handle the value as string manually instead 🤔

@jods4
Copy link
Contributor Author

jods4 commented Nov 12, 2020

@vhoyer saying it's consistent with JS is implying you know the underlying implementation.
Javascript has Number() ctor, which is a far better tool to parse numbers.
How is it consistent with javascript that .number returns a different result from Number()?
The modifier isn't called .parseFloat so I'd say I find it pretty inconsistent with javascript.

@vhoyer
Copy link

vhoyer commented Nov 12, 2020

well, your right about that, but looking at the parsing behavior from Number I'd say instead of return x as a string, if we end up updating this, maybe it's better to return a NaN?

@jods4
Copy link
Contributor Author

jods4 commented Nov 12, 2020

I agree with you, it's strange that .number returns a string, sometimes, and I'd also prefer NaN.
But that would be massively breaking for v2 users, much more so than just being more strict at parsing bogus inputs.

Fun fact: v-model implicitly use .number if your input is of type="number". Given that this input type is supported way back to IE10, a simpler, more efficient strategy now could be to read valueAsNumber instead, for that specific input type.

EDIT: although I guess the main reason why .number returns a string when parsing fails is so that the values can round-trip and the input text isn't cleared as soon as you type something that doesn't parse. There are other solutions to this problem but I guess that's the easiest one for a core modifier. Note that using parseFloat goes straight against that design as it trims the text that is in the textbox if you enter something that doesn't parse after a number prefix.

@skirtles-code
Copy link
Contributor

That's where pipes shined, but they were removed from Vue 3.

I assume you're referring to filters. Anything you used to do with filters you should now be able to do with methods instead. See:

https://v3.vuejs.org/guide/migration/filters.html#migration-strategy

Could you please tell me more about custom v-model modifiers? I am very interested into those (e.g. a .null, .date and more) but had the impression that Vue did not support them?

I believe the relevant docs are here:

https://v3.vuejs.org/guide/component-custom-events.html#handling-v-model-modifiers

@jods4
Copy link
Contributor Author

jods4 commented Nov 12, 2020

@skirtles-code Thanks for the pointers, much appreciated!

I assume you're referring to filters. Anything you used to do with filters you should now be able to do with methods instead.

Yes, I'm referring to filters. Sorry for the confusion, they are called pipes in Angular; Value Converters in Aurelia.

Functions are a good enough replacement for (one-way) bindings and they'll be even more so when JS gets the pipe operator |> and you could write :value="name |> uppercase".

They are not usable in (two-way) v-model, though and that's really where their power was lost.

I believe the relevant [modifiers] docs are here:

Thank you, I thought custom modifiers weren't a thing!
I'll have good uses for them.

It's no silver bullet, though. It only works on components and is defined per-component.
That can make sense but it turns them into poor substitute for a custom .number (or any generic filter for that matter), as they wouldn't work on <input> for example.

Another limitation is that they don't take arguments, they're just booleans. This is also pretty limiting compared to filters, that may for example accept a date format, a default value, a rounding factor.

It's amusing that the recommended way to manipulate a value updated by a v-model is to use a property setter. From what I read, the main argument against filters is that they may create instable loops when a value changes between reading and writing it. But that same thing can exactly happen if you set up a property setter, there's no difference.

On the other hand, you have to use a cumbersome syntax (that won't play nice with <script setup> if you don't rely on a computed) and most of all it's not easily composable/reusable.

A basic example that has been mentioned before is wanting to convert empty strings to null in your model. Why would .trim be something that's supported everywhere and there's no way to achieve something similar for a .null (or a filter syntax)? What makes trim so different?
A good design should enable users to create .trim or .number without relying on Vue Core.

It looks like the focus of that issue has shifted from a .number implementation detail to filters 😆

@leopiccionia
Copy link
Contributor

leopiccionia commented Nov 13, 2020

They are not usable in (two-way) v-model, though and that's really where their power was lost.

In fact, Vue 2 supports filters only inside mustache expressions ({{ ... }}).

The last Vue version that supports filters inside v-bind, v-model, v-for, etc. was v1. As per migration guide, the most suitable replacement for filters is computed getters/setters.


I particularly like to use computed properties factories for reusing these logics, although typing them may be tricky in some cases.

In Option API:

function trim (key) {
  return {
    get () {
      const val = this[key]
      return typeof val === 'string' ? val.trim() : ''
    },
    set (val) {
      this[key] = val.trim()
    }
  }
}

In Composition API:

function useTrim (originalRef) {
  return computed({
    get: () => {
      const val = originalRef.value
      return typeof val === 'string' ? val.trim() : ''
    },
    set: (val) => {
      originalRef.value = val.trim()
    }
  })
}

@jods4
Copy link
Contributor Author

jods4 commented Nov 13, 2020

I wasn't aware Vue 2 only had one-way filters. Yeah, it's just functions then.
Interesting it never had two-way filters as that's a nice useful primitive that everyone else (who does 2-way bindings) has: e.g. WPF (binding converters), Angular (pipes), Aurelia (value converters).

ref factories is ok-ish. Although if you start stacking them it's crazy to have 3 computeds nested on top of a ref for something like v-model="name | trim | upper | null" (bonus points if you build a slightly more complex abstraction, like a ref factory that takes a list of transforms).

Also declaring them as global template resources make them super-easy to use in any place (such as the previous example), whereas you'd need to import them in JS.
But yeah, they'll do the trick for now.

@stmtk1
Copy link

stmtk1 commented Nov 14, 2020

This behavior causes bug. But compatibility is important. So I suggest adding this feature request as a option.

@LinusBorg
Copy link
Member

From what I read, the main argument against filters is that they may create instable loops when a value changes between reading and writing it.

@jods4 That's not really the reason I am aware of. What little value the one-way filters that we had them in Vue 2 added did not justify the compiler-problems that this custom non-JS compatible Syntax added, when compared to simple functions that can do the same in a way that any JS parser is fine with.

@jods4
Copy link
Contributor Author

jods4 commented Nov 14, 2020

@LinusBorg 100% agree with that.
It's only if you support them in 2-way scenarios that they bring something new to the table.

@HcySunYang HcySunYang added the need guidance The approach/solution in the PR is unclear and requires guidance from maintainer to proceed further. label Mar 31, 2021
chrislone pushed a commit to chrislone/core that referenced this issue Feb 4, 2023
close vuejs#4946
close vuejs#2598
close vuejs#2604

This commit also refactors internal usage of previous loose
implementation of `toNumber` to the stricter version where applicable.
Use of `looseToNumber` is preserved for `v-model.number` modifier to
ensure backwards compatibility and consistency with Vue 2 behavior.
zhangzhonghe pushed a commit to zhangzhonghe/core that referenced this issue Apr 12, 2023
close vuejs#4946
close vuejs#2598
close vuejs#2604

This commit also refactors internal usage of previous loose
implementation of `toNumber` to the stricter version where applicable.
Use of `looseToNumber` is preserved for `v-model.number` modifier to
ensure backwards compatibility and consistency with Vue 2 behavior.
@github-actions github-actions bot locked and limited conversation to collaborators Sep 18, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
need guidance The approach/solution in the PR is unclear and requires guidance from maintainer to proceed further.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants