Skip to content

Commit

Permalink
fix(v-model): ensure initial value is set after other attributes
Browse files Browse the repository at this point in the history 
fix #2325
  • Loading branch information
@yyx990803
yyx990803 committed Oct 8, 2020
1 parent d1a6ce6 commit 54ed759
Showing 1 changed file with 5 additions and 2 deletions.
7 changes: 5 additions & 2 deletions packages/runtime-dom/src/directives/vModel.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -48,8 +48,7 @@ type ModelDirective<T> = ObjectDirective<T & { _assign: AssignerFn }>
export const vModelText: ModelDirective<
HTMLInputElement | HTMLTextAreaElement
> = {
created(el, { value, modifiers: { lazy, trim, number } }, vnode) {
el.value = value == null ? '' : value
created(el, { modifiers: { lazy, trim, number } }, vnode) {
el._assign = getModelAssigner(vnode)
const castToNumber = number || el.type === 'number'
addEventListener(el, lazy ? 'change' : 'input', e => {
Expand Down Expand Up @@ -77,6 +76,10 @@ export const vModelText: ModelDirective<
addEventListener(el, 'change', onCompositionEnd)
}
},
// set value on mounted so it's after min/max for type="range"
mounted(el, { value }) {
el.value = value == null ? '' : value
},
beforeUpdate(el, { value, modifiers: { trim, number } }, vnode) {
el._assign = getModelAssigner(vnode)
// avoid clearing unresolved text. #2302
Expand Down

3 comments on commit 54ed759

@jods4
Copy link
Contributor

@jods4 jods4 commented on 54ed759 Oct 9, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The purpose here is clear but I'm afraid of the unintended side-effects at larger scale.
I may not be 100% correct here, so please tell me if I'm wrong:

Before this change Vue users can assume that the lifecycle mounted event was a good time to do stuff after all bindings were set on target.

I'm not affected by this but for example:
If I wanted to do a textbox that would scale its width to its contents I could have created a directive v-autowidth that would measure the size of the input content on mounted.

It would have worked fine before, after this change there's a race condition on which directive runs first.

This commit fundamentally change at which time in the lifecycle data is set and that could be troublesome.

I have no ready-made solution but wouldn't it be better to have a way to prioritize some values over other during the binding stage (i.e. have a way to ensure min, max, step happen before v-model)?

@yyx990803
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Directive appearance order is respected so you can simply put your directive after v-model. Special casing certain props is very hard to be comprehensive.

@jods4
Copy link
Contributor

@jods4 jods4 commented on 54ed759 Dec 5, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the answer, so <input v-model v-autosize> would work but <input v-autosize v-model> would not?
Is this ordering guarantee actually documented? I don't remember seeing it.
I saw other issues where relying on the order of HTML attributes was discussed and participants were not very keen on taking ordering as granted (understandable).

EDIT: how does your answer extend to code other than directives that expect to be able to read a textbox value in mounted? E.g. what are the ordering guarantees if a generic composable function calls onMounted() to similar effect?

I don't see any easy fix for the problem that prompted this change but:

  • it doesn't feel great that just swapping the 2 attributes turn working code into a broken one;
  • esp. since the ordering might be influenced by other factors (e.g. vue eslint wants you to put attributes in a specific order -- although it's fine here as it wants v-model first);
  • the mental model of "at mounted everything is bound" is still broken;
  • the behavior is inconsistent / unpredictable. Not all v-model set their value at the same time as this change only applies to <input type=text> and <textarea>.

A mitigation could be to narrow down the scope of this change.
I believe of all input types, only range will coerce the value to its minimum (other will just be :invalid), so instead of applying it to widely used text inputs, you could create a vModelRange for this fix?

If we're going with "attribute order matters", wouldn't it be more "intuitive" (not the right word I'm afraid) to express the min / v-model dependency that way?
<input min v-model> should work and everything is set when mounted runs?
That makes the rule only matter for the narrow set of attribute that are actually inter-dependent (same behavior occurs in raw JS if you set value before min) rather than modifying the common behavior of all text inputs. Also keeps the mounted contract intact.

I think it's useful to think of a larger-scale recommendation because this may well not be the only time inter-dependent attributes exist.
A custom web element could well expose similar behavior as range / min / v-model and the "fix" in this commit doesn't generalize.

Please sign in to comment.