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

fix(runtime-dom): prevent unnecessary DOM update from v-model #11656

114 changes: 114 additions & 0 deletions packages/runtime-dom/__tests__/directives/vModel.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -729,6 +729,120 @@ describe('vModel', () => {
expect(bar.checked).toEqual(false)
})

it('should not update DOM unnecessarily', async () => {
const component = defineComponent({
data() {
return { value: true }
},
render() {
return [
withVModel(
h('input', {
type: 'checkbox',
'onUpdate:modelValue': setValue.bind(this),
}),
this.value,
),
]
},
})
render(h(component), root)

const input = root.querySelector('input')
const data = root._vnode.component.data

const setCheckedSpy = vi.spyOn(input, 'checked', 'set')

// Trigger a change event without actually changing the value
triggerEvent('change', input)
await nextTick()
expect(data.value).toEqual(true)
expect(setCheckedSpy).not.toHaveBeenCalled()

// Change the value and trigger a change event
input.checked = false
triggerEvent('change', input)
await nextTick()
expect(data.value).toEqual(false)
expect(setCheckedSpy).toHaveBeenCalledTimes(1)

setCheckedSpy.mockClear()

data.value = false
await nextTick()
expect(input.checked).toEqual(false)
expect(setCheckedSpy).not.toHaveBeenCalled()

data.value = true
await nextTick()
expect(input.checked).toEqual(true)
expect(setCheckedSpy).toHaveBeenCalledTimes(1)
})

it('should handle array values correctly without unnecessary updates', async () => {
const component = defineComponent({
data() {
return { value: ['foo'] }
},
render() {
return [
withVModel(
h('input', {
type: 'checkbox',
value: 'foo',
'onUpdate:modelValue': setValue.bind(this),
}),
this.value,
),
withVModel(
h('input', {
type: 'checkbox',
value: 'bar',
'onUpdate:modelValue': setValue.bind(this),
}),
this.value,
),
]
},
})
render(h(component), root)

const [foo, bar] = root.querySelectorAll('input')
const data = root._vnode.component.data

const setCheckedSpyFoo = vi.spyOn(foo, 'checked', 'set')
const setCheckedSpyBar = vi.spyOn(bar, 'checked', 'set')

expect(foo.checked).toEqual(true)
expect(bar.checked).toEqual(false)

triggerEvent('change', foo)
await nextTick()
expect(data.value).toEqual(['foo'])
expect(setCheckedSpyFoo).not.toHaveBeenCalled()

bar.checked = true
triggerEvent('change', bar)
await nextTick()
expect(data.value).toEqual(['foo', 'bar'])
expect(setCheckedSpyBar).toHaveBeenCalledTimes(1)

setCheckedSpyFoo.mockClear()
setCheckedSpyBar.mockClear()

data.value = ['foo', 'bar']
await nextTick()
expect(setCheckedSpyFoo).not.toHaveBeenCalled()
expect(setCheckedSpyBar).not.toHaveBeenCalled()

data.value = ['bar']
await nextTick()
expect(setCheckedSpyFoo).toHaveBeenCalledTimes(1)
expect(setCheckedSpyBar).not.toHaveBeenCalled()
expect(foo.checked).toEqual(false)
expect(bar.checked).toEqual(true)
})

it('should work with radio', async () => {
const component = defineComponent({
data() {
Expand Down
23 changes: 15 additions & 8 deletions packages/runtime-dom/src/directives/vModel.ts
Original file line number Diff line number Diff line change
Expand Up @@ -138,13 +138,13 @@ export const vModelCheckbox: ModelDirective<HTMLInputElement> = {
assign(filtered)
}
} else if (isSet(modelValue)) {
const cloned = new Set(modelValue)
const updatedValue = new Set(modelValue)
if (checked) {
cloned.add(elementValue)
updatedValue.add(elementValue)
} else {
cloned.delete(elementValue)
updatedValue.delete(elementValue)
}
assign(cloned)
assign(new Set(updatedValue))
Copy link
Member

Choose a reason for hiding this comment

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

The change here is not necessary.
We've already cloned the set on line 141

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@edison1105
Thank you for review!
Obviously, it was unnecessary, I've fixed it!

} else {
assign(getCheckboxValue(el, checked))
}
Expand All @@ -166,12 +166,19 @@ function setChecked(
// store the v-model value on the element so it can be accessed by the
// change listener.
;(el as any)._modelValue = value
let newChecked: boolean

if (isArray(value)) {
el.checked = looseIndexOf(value, vnode.props!.value) > -1
newChecked = looseIndexOf(value, vnode.props!.value) > -1
} else if (isSet(value)) {
el.checked = value.has(vnode.props!.value)
} else if (value !== oldValue) {
el.checked = looseEqual(value, getCheckboxValue(el, true))
newChecked = value.has(vnode.props!.value)
} else {
newChecked = looseEqual(value, getCheckboxValue(el, true))
}

// Only update if the checked state has changed
if (el.checked !== newChecked) {
el.checked = newChecked
}
}

Expand Down