Skip to content

Commit

Permalink
feat(runtime-dom): allow native Set as v-model checkbox source (#1957)
Browse files Browse the repository at this point in the history
  • Loading branch information
@Picknight
Picknight committed Sep 14, 2020
1 parent 542680e commit cf1b6c6
Show file tree
Hide file tree
Showing 4 changed files with 104 additions and 4 deletions.
70 changes: 70 additions & 0 deletions packages/runtime-dom/__tests__/directives/vModel.spec.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -433,6 +433,76 @@ describe('vModel', () => {
expect(bar.checked).toEqual(false)
})

it(`should support Set as a checkbox model`, async () => {
const component = defineComponent({
data() {
return { value: new Set() }
},
render() {
return [
withVModel(
h('input', {
type: 'checkbox',
class: 'foo',
value: 'foo',
'onUpdate:modelValue': setValue.bind(this)
}),
this.value
),
withVModel(
h('input', {
type: 'checkbox',
class: 'bar',
value: 'bar',
'onUpdate:modelValue': setValue.bind(this)
}),
this.value
)
]
}
})
render(h(component), root)

const foo = root.querySelector('.foo')
const bar = root.querySelector('.bar')
const data = root._vnode.component.data

foo.checked = true
triggerEvent('change', foo)
await nextTick()
expect(data.value).toMatchObject(new Set(['foo']))

bar.checked = true
triggerEvent('change', bar)
await nextTick()
expect(data.value).toMatchObject(new Set(['foo', 'bar']))

bar.checked = false
triggerEvent('change', bar)
await nextTick()
expect(data.value).toMatchObject(new Set(['foo']))

foo.checked = false
triggerEvent('change', foo)
await nextTick()
expect(data.value).toMatchObject(new Set())

data.value = new Set(['foo'])
await nextTick()
expect(bar.checked).toEqual(false)
expect(foo.checked).toEqual(true)

data.value = new Set(['bar'])
await nextTick()
expect(foo.checked).toEqual(false)
expect(bar.checked).toEqual(true)

data.value = new Set()
await nextTick()
expect(foo.checked).toEqual(false)
expect(bar.checked).toEqual(false)
})

it('should work with radio', async () => {
const component = defineComponent({
data() {
Expand Down
28 changes: 24 additions & 4 deletions packages/runtime-dom/src/directives/vModel.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,9 @@ import {
looseEqual,
looseIndexOf,
invokeArrayFns,
toNumber
toNumber,
isSet,
looseHas
} from '@vue/shared'

type AssignerFn = (value: any) => void
Expand Down Expand Up @@ -111,6 +113,14 @@ export const vModelCheckbox: ModelDirective<HTMLInputElement> = {
filtered.splice(index, 1)
assign(filtered)
}
} else if (isSet(modelValue)) {
const found = modelValue.has(elementValue)
if (checked && !found) {
assign(modelValue.add(elementValue))

This comment has been minimized.

Sign in to view

Copy link
@jods4

jods4 Sep 16, 2020 •
edited
Loading

Contributor

Great feature!
Why call assign here?

Unlike arrays, which are replaced by a new instance, this code mutates the Set, so modelValue has not changed, so assign seems unneeded to me.

Once assign is removed, the Set::has check is meaningless as Set::add does check if the element is already in there or not. Same for Set:delete. So code can be reduced to:

} else if (isSet(modelValue)) {
  if (checked)
    modelValue.add(elementValue)
  else
    modelValue.delete(elementValue)
}

This comment has been minimized.

Sign in to view

Copy link
@jods4

jods4 Sep 16, 2020 •
edited
Loading

Contributor

Reading the rest of the PR, there's a bug here.

When updating the checkbox, Vue uses looseHas, which considers more things equal, such two random objects with the same shape.

When updating the model, Vue uses Set::has, which is a strict reference equality.

Hence, we could end up with multiple objects loosely equal in the Set.
Equality must be consistent between reading and writing.

I'm gonna say that I lean toward using the JS semantics here, not looseEqual.
One key reason to use a Set over an array is its different perf characteristic. With many elements, it's much faster and has O(1) access instead of O(n).

Using looseHas when binding Sets to checkboxes makes the Set behave like an array and is killing the perf expected from a Set in the first place.
Even moreso since the iteration might be repeated on many checkboxes; and looseEqual can be fairly costly depending on the shape of the objects it compares.

EDIT: one more argument here.
It is very unlikely that the user code manipulating the Set uses looseEqual or equivalent.
This then means that Vue manipulates the Set with slightly different semantic than user code, which is not a good thing.

This comment has been minimized.

Sign in to view

Copy link
@Picknight

Picknight Sep 24, 2020

Author Contributor

Sry, I just noticed the comments here today. @jods4

Why call assign here?

The purpose here is to call update:modelValue function.

the Set::has check is meaningless.

Agreed. So code can be reduced to:

} else if (isSet(modelValue)) {
  if (checked)
    assign(modelValue.add(elementValue))
  else
    assign(modelValue.delete(elementValue))
}

This comment has been minimized.

Sign in to view

Copy link
@jods4

jods4 Sep 24, 2020

Contributor

The purpose here is to call update:modelValue function.

My point here is that modelValue has not changed (it has only mutated), so there's no use emitting update:modelValue.

This comment has been minimized.

Sign in to view

Copy link
@Picknight

Picknight Sep 25, 2020

Author Contributor

Yeah you're right.

This comment has been minimized.

Sign in to view

Copy link
@Picknight

Picknight Sep 25, 2020

Author Contributor

About looseHas, if we discard looseHas when modelValue is Set, this will cause the behavior of Set to be inconsistent with Array.

This comment has been minimized.

Sign in to view

Copy link
@jods4

jods4 Sep 25, 2020

Contributor

But if you keep looseHas:

  • its behavior is inconsistent with user-code, which is very unintuitive and bug-prone;
  • you kill the perf characteristics of Set (and there will be no way to efficiently bind a large number of checkboxes against a collection of many values).

IMHO if someone wants the behaviour of arrays (loose equality + O(N) access)... they can use an array?

Note that if you keep looseHas this PR is inconsistent in its handling.
} else if (!checked && found) {
modelValue.delete(elementValue)
assign(modelValue)
}
} else {
assign(getCheckboxValue(el, checked))
}
Expand All @@ -132,6 +142,8 @@ function setChecked(
;(el as any)._modelValue = value
if (isArray(value)) {
el.checked = looseIndexOf(value, vnode.props!.value) > -1
} else if (isSet(value)) {
el.checked = looseHas(value, vnode.props!.value)
} else if (value !== oldValue) {
el.checked = looseEqual(value, getCheckboxValue(el, true))
}
Expand Down Expand Up @@ -178,10 +190,10 @@ export const vModelSelect: ModelDirective<HTMLSelectElement> = {

function setSelected(el: HTMLSelectElement, value: any) {
const isMultiple = el.multiple
if (isMultiple && !isArray(value)) {
if (isMultiple && !isArray(value) && !isSet(value)) {
__DEV__ &&
warn(
`<select multiple v-model> expects an Array value for its binding, ` +
`<select multiple v-model> expects an Array or Set value for its binding, ` +
`but got ${Object.prototype.toString.call(value).slice(8, -1)}.`
)
return
Expand All @@ -190,7 +202,11 @@ function setSelected(el: HTMLSelectElement, value: any) {
const option = el.options[i]
const optionValue = getValue(option)
if (isMultiple) {
option.selected = looseIndexOf(value, optionValue) > -1
if (isArray(value)) {
option.selected = looseIndexOf(value, optionValue) > -1
} else {
option.selected = looseHas(value, optionValue)
}
} else {
if (looseEqual(getValue(option), value)) {
el.selectedIndex = i
Expand Down Expand Up @@ -280,6 +296,10 @@ if (__NODE_JS__) {
if (vnode.props && looseIndexOf(value, vnode.props.value) > -1) {
return { checked: true }
}
} else if (isSet(value)) {
if (vnode.props && looseHas(value, vnode.props.value)) {
return { checked: true }
}
} else if (value) {
return { checked: true }
}
Expand Down
3 changes: 3 additions & 0 deletions packages/shared/src/index.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,9 @@ export const hasOwn = (
): key is keyof typeof val => hasOwnProperty.call(val, key)

export const isArray = Array.isArray
export const isSet = (val: any): boolean => {
return toRawType(val) === 'Set'

This comment has been minimized.

Sign in to view

Copy link
@jods4

jods4 Sep 16, 2020 •
edited
Loading

Contributor

EDIT: Forget that comment, I see that Evan changed the code in a more recent commit.

Why not val instanceof Set?
It works on proxies and it's the code found in isDate.
I would be surprised that calling to toString on the proxy, slicing, and comparing strings ends up being faster, is it?
}
export const isDate = (val: unknown): val is Date => val instanceof Date
export const isFunction = (val: unknown): val is Function =>
typeof val === 'function'
Expand Down
7 changes: 7 additions & 0 deletions packages/shared/src/looseEqual.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -51,3 +51,10 @@ export function looseEqual(a: any, b: any): boolean {
export function looseIndexOf(arr: any[], val: any): number {
return arr.findIndex(item => looseEqual(item, val))
}

export function looseHas(set: Set<any>, val: any): boolean {

This comment has been minimized.

Sign in to view

Copy link
@jods4

jods4 Sep 16, 2020

Contributor

See my comment above
cf1b6c6#commitcomment-42355917

looseHas is killing the reason why you'd use a Set: its O(1) access time, making it behave like an array :(
for (let item of set) {
if (looseEqual(item, val)) return true
}
return false
}

0 comments on commit cf1b6c6

Please sign in to comment.