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 false positives for v-bind="obj" with v-model in vue/attributes-order rule #1771

Merged
merged 3 commits into from Jan 24, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
32 changes: 21 additions & 11 deletions lib/rules/attributes-order.js
Expand Up @@ -36,6 +36,14 @@ const ATTRS = {
function isVBind(node) {
return Boolean(node && node.directive && node.key.name.name === 'bind')
}
/**
* Check whether the given attribute is `v-model` directive.
* @param {VAttribute | VDirective | undefined | null} node
* @returns { node is VDirective }
*/
function isVModel(node) {
return Boolean(node && node.directive && node.key.name.name === 'model')
}
/**
* Check whether the given attribute is plain attribute.
* @param {VAttribute | VDirective | undefined | null} node
Expand All @@ -45,12 +53,12 @@ function isVAttribute(node) {
return Boolean(node && !node.directive)
}
/**
* Check whether the given attribute is plain attribute or `v-bind` directive.
* Check whether the given attribute is plain attribute, `v-bind` directive or `v-model` directive.
* @param {VAttribute | VDirective | undefined | null} node
* @returns { node is VAttribute }
*/
function isVAttributeOrVBind(node) {
return isVAttribute(node) || isVBind(node)
function isVAttributeOrVBindOrVModel(node) {
return isVAttribute(node) || isVBind(node) || isVModel(node)
}

/**
Expand Down Expand Up @@ -235,8 +243,8 @@ function create(context) {

if (isVBindObject(node)) {
// prev, v-bind:foo, v-bind -> v-bind:foo, v-bind, prev
isMoveUp = isVAttributeOrVBind
} else if (isVAttributeOrVBind(node)) {
isMoveUp = isVAttributeOrVBindOrVModel
} else if (isVAttributeOrVBindOrVModel(node)) {
// prev, v-bind, v-bind:foo -> v-bind, v-bind:foo, prev
isMoveUp = isVBindObject
} else {
Expand Down Expand Up @@ -298,11 +306,13 @@ function create(context) {
const attributes = node.attributes.filter((node, index, attributes) => {
if (
isVBindObject(node) &&
(isVAttributeOrVBind(attributes[index - 1]) ||
isVAttributeOrVBind(attributes[index + 1]))
(isVAttributeOrVBindOrVModel(attributes[index - 1]) ||
isVAttributeOrVBindOrVModel(attributes[index + 1]))
) {
// In Vue 3, ignore the `v-bind:foo=" ... "` and `v-bind ="object"` syntax
// as they behave differently if you change the order.
// In Vue 3, ignore `v-bind="object"`, which is
// a pair of `v-bind:foo="..."` and `v-bind="object"` and
// a pair of `v-model="..."` and `v-bind="object"`,
// because changing the order behaves differently.
return false
}
return true
Expand Down Expand Up @@ -330,14 +340,14 @@ function create(context) {
if (isVBindObject(node)) {
// node is `v-bind ="object"` syntax

// In Vue 3, if change the order of `v-bind:foo=" ... "` and `v-bind ="object"`,
// In Vue 3, if change the order of `v-bind:foo="..."`, `v-model="..."` and `v-bind="object"`,
// the behavior will be different, so adjust so that there is no change in behavior.

const len = attributes.length
for (let nextIndex = index + 1; nextIndex < len; nextIndex++) {
const next = attributes[nextIndex]

if (isVAttributeOrVBind(next) && !isVBindObject(next)) {
if (isVAttributeOrVBindOrVModel(next) && !isVBindObject(next)) {
// It is considered to be in the same order as the next bind prop node.
return getPositionFromAttrIndex(nextIndex)
}
Expand Down
80 changes: 80 additions & 0 deletions tests/lib/rules/attributes-order.js
Expand Up @@ -442,6 +442,42 @@ tester.run('attributes-order', rule, {
</template>`,
options: [{ alphabetical: true }]
},
{
filename: 'test.vue',
code: `
<template>
<div
v-if="x"
v-bind="b"
v-model="c"
v-bind:value="a">
</div>
</template>`
},
{
filename: 'test.vue',
code: `
<template>
<div
v-if="x"
v-model="c"
v-bind="b"
v-bind:value="a">
</div>
</template>`
},
{
filename: 'test.vue',
code: `
<template>
<div
v-if="x"
v-bind="b"
v-bind:id="a"
v-model="c">
</div>
</template>`
},

// omit order
{
Expand Down Expand Up @@ -1246,6 +1282,50 @@ tester.run('attributes-order', rule, {
'Attribute "v-if" should go before "v-on:click".'
]
},
{
filename: 'test.vue',
code: `
<template>
<div
v-custom-directive="x"
v-bind="b"
v-model="c"
v-bind:value="a">
</div>
</template>`,
output: `
<template>
<div
v-bind="b"
v-model="c"
v-custom-directive="x"
v-bind:value="a">
</div>
</template>`,
errors: ['Attribute "v-model" should go before "v-custom-directive".']
},
{
filename: 'test.vue',
code: `
<template>
<div
v-if="x"
v-model="c"
v-bind="b"
v-bind:id="a">
</div>
</template>`,
output: `
<template>
<div
v-if="x"
v-bind="b"
v-bind:id="a"
v-model="c">
</div>
</template>`,
errors: ['Attribute "v-bind:id" should go before "v-model".']
},
Comment on lines +1307 to +1328
Copy link
Member

Choose a reason for hiding this comment

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

Will this create false positives in existing Vue 2 code? There, the order v-if, v-model, v-bind, v-bind:id would be perfectly fine.

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think there are any false positives because the id must be before the v-model.

https://vuejs.org/v2/style-guide/#Element-attribute-order-recommended

Copy link
Member

Choose a reason for hiding this comment

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

Oh, indeed.


// omit order
{
Expand Down