vuejs / core

🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.
https://vuejs.org/
MIT License
47.69k stars 8.33k forks source link

toNumber is lenient #2598

Closed jods4 closed 2 years ago

jods4 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 :thinking:

jods4 commented 4 years ago

@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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

@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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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

LinusBorg commented 4 years ago

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 commented 4 years ago

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