From 543361bc4a14e2088533e418e03025e0187704f3 Mon Sep 17 00:00:00 2001 From: Yosuke Ota Date: Sun, 27 Dec 2020 11:56:22 +0900 Subject: [PATCH] Fixed false positives for `v-bind="object"` syntax in `vue/attributes-order` rule (#1391) --- docs/rules/attributes-order.md | 28 ++- lib/rules/attributes-order.js | 198 ++++++++++++++------ tests/lib/rules/attributes-order.js | 270 ++++++++++++++++++++++++++++ 3 files changed, 440 insertions(+), 56 deletions(-) diff --git a/docs/rules/attributes-order.md b/docs/rules/attributes-order.md index e5c8a91f1..8bcb8b8ea 100644 --- a/docs/rules/attributes-order.md +++ b/docs/rules/attributes-order.md @@ -91,7 +91,33 @@ This rule aims to enforce ordering of component attributes. The default order is +Note that `v-bind="object"` syntax is considered to be the same as the next or previous attribute categories. + + + +```vue + +``` + + + ## :wrench: Options + ```json { "vue/attributes-order": ["error", { @@ -113,7 +139,7 @@ This rule aims to enforce ordering of component attributes. The default order is } ``` -### `"alphabetical": true` +### `"alphabetical": true` diff --git a/lib/rules/attributes-order.js b/lib/rules/attributes-order.js index 34a9d7d32..2441ee497 100644 --- a/lib/rules/attributes-order.js +++ b/lib/rules/attributes-order.js @@ -8,6 +8,11 @@ const utils = require('../utils') // ------------------------------------------------------------------------------ // Rule Definition // ------------------------------------------------------------------------------ + +/** + * @typedef { VDirective & { key: VDirectiveKey & { name: VIdentifier & { name: 'bind' } } } } VBindDirective + */ + const ATTRS = { DEFINITION: 'DEFINITION', LIST_RENDERING: 'LIST_RENDERING', @@ -22,13 +27,47 @@ const ATTRS = { CONTENT: 'CONTENT' } +/** + * Check whether the given attribute is `v-bind` directive. + * @param {VAttribute | VDirective | undefined | null} node + * @returns { node is VBindDirective } + */ +function isVBind(node) { + return Boolean(node && node.directive && node.key.name.name === 'bind') +} +/** + * Check whether the given attribute is plain attribute. + * @param {VAttribute | VDirective | undefined | null} node + * @returns { node is VAttribute } + */ +function isVAttribute(node) { + return Boolean(node && !node.directive) +} +/** + * Check whether the given attribute is plain attribute or `v-bind` directive. + * @param {VAttribute | VDirective | undefined | null} node + * @returns { node is VAttribute } + */ +function isVAttributeOrVBind(node) { + return isVAttribute(node) || isVBind(node) +} + +/** + * Check whether the given attribute is `v-bind="..."` directive. + * @param {VAttribute | VDirective | undefined | null} node + * @returns { node is VBindDirective } + */ +function isVBindObject(node) { + return isVBind(node) && node.key.argument == null +} + /** * @param {VAttribute | VDirective} attribute * @param {SourceCode} sourceCode */ function getAttributeName(attribute, sourceCode) { if (attribute.directive) { - if (attribute.key.name.name === 'bind') { + if (isVBind(attribute)) { return attribute.key.argument ? sourceCode.getText(attribute.key.argument) : '' @@ -62,7 +101,7 @@ function getDirectiveKeyName(directiveKey, sourceCode) { function getAttributeType(attribute, sourceCode) { let propName if (attribute.directive) { - if (attribute.key.name.name !== 'bind') { + if (!isVBind(attribute)) { const name = attribute.key.name.name if (name === 'for') { return ATTRS.LIST_RENDERING @@ -130,24 +169,14 @@ function getPosition(attribute, attributePosition, sourceCode) { * @param {SourceCode} sourceCode */ function isAlphabetical(prevNode, currNode, sourceCode) { - const isSameType = - getAttributeType(prevNode, sourceCode) === - getAttributeType(currNode, sourceCode) - if (isSameType) { - const prevName = getAttributeName(prevNode, sourceCode) - const currName = getAttributeName(currNode, sourceCode) - if (prevName === currName) { - const prevIsBind = Boolean( - prevNode.directive && prevNode.key.name.name === 'bind' - ) - const currIsBind = Boolean( - currNode.directive && currNode.key.name.name === 'bind' - ) - return prevIsBind <= currIsBind - } - return prevName < currName + const prevName = getAttributeName(prevNode, sourceCode) + const currName = getAttributeName(currNode, sourceCode) + if (prevName === currName) { + const prevIsBind = isVBind(prevNode) + const currIsBind = isVBind(currNode) + return prevIsBind <= currIsBind } - return true + return prevName < currName } /** @@ -186,16 +215,6 @@ function create(context) { } else attributePosition[item] = i }) - /** - * @typedef {object} State - * @property {number} currentPosition - * @property {VAttribute | VDirective} previousNode - */ - /** - * @type {State | null} - */ - let state - /** * @param {VAttribute | VDirective} node * @param {VAttribute | VDirective} previousNode @@ -213,43 +232,112 @@ function create(context) { fix(fixer) { const attributes = node.parent.attributes - const shiftAttrs = attributes.slice( + + /** @type { (node: VAttribute | VDirective | undefined) => boolean } */ + let isMoveUp + + if (isVBindObject(node)) { + // prev, v-bind:foo, v-bind -> v-bind:foo, v-bind, prev + isMoveUp = isVAttributeOrVBind + } else if (isVAttributeOrVBind(node)) { + // prev, v-bind, v-bind:foo -> v-bind, v-bind:foo, prev + isMoveUp = isVBindObject + } else { + isMoveUp = () => false + } + + const previousNodes = attributes.slice( attributes.indexOf(previousNode), - attributes.indexOf(node) + 1 + attributes.indexOf(node) ) + const moveUpNodes = [node] + const moveDownNodes = [] + let index = 0 + while (previousNodes[index]) { + const node = previousNodes[index++] + if (isMoveUp(node)) { + moveUpNodes.unshift(node) + } else { + moveDownNodes.push(node) + } + } + const moveNodes = [...moveUpNodes, ...moveDownNodes] - return shiftAttrs.map((attr, i) => { - const text = - attr === previousNode - ? sourceCode.getText(node) - : sourceCode.getText(shiftAttrs[i - 1]) - return fixer.replaceText(attr, text) + return moveNodes.map((moveNode, index) => { + const text = sourceCode.getText(moveNode) + return fixer.replaceText(previousNodes[index] || node, text) }) } }) } return utils.defineTemplateBodyVisitor(context, { - VStartTag() { - state = null - }, - VAttribute(node) { - let inAlphaOrder = true - if (state && alphabetical) { - inAlphaOrder = isAlphabetical(state.previousNode, node, sourceCode) + VStartTag(node) { + const attributes = node.attributes.filter((node, index, attributes) => { + if ( + isVBindObject(node) && + (isVAttributeOrVBind(attributes[index - 1]) || + isVAttributeOrVBind(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. + return false + } + return true + }) + if (attributes.length <= 1) { + return } - if ( - !state || - (state.currentPosition <= - getPosition(node, attributePosition, sourceCode) && - inAlphaOrder) - ) { - state = { - currentPosition: getPosition(node, attributePosition, sourceCode), - previousNode: node + + let previousNode = attributes[0] + let previousPosition = getPositionFromAttrIndex(0) + for (let index = 1; index < attributes.length; index++) { + const node = attributes[index] + const position = getPositionFromAttrIndex(index) + + let valid = previousPosition <= position + if (valid && alphabetical && previousPosition === position) { + valid = isAlphabetical(previousNode, node, sourceCode) } - } else { - reportIssue(node, state.previousNode) + if (valid) { + previousNode = node + previousPosition = position + } else { + reportIssue(node, previousNode) + } + } + + /** + * @param {number} index + * @returns {number} + */ + function getPositionFromAttrIndex(index) { + const node = attributes[index] + if (isVBindObject(node)) { + // node is `v-bind ="object"` syntax + + // In Vue 3, if change the order of `v-bind:foo=" ... "` 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)) { + // It is considered to be in the same order as the next bind prop node. + return getPositionFromAttrIndex(nextIndex) + } + } + for (let prevIndex = index - 1; prevIndex >= 0; prevIndex--) { + const prev = attributes[prevIndex] + + if (isVAttributeOrVBind(prev) && !isVBindObject(prev)) { + // It is considered to be in the same order as the prev bind prop node. + return getPositionFromAttrIndex(prevIndex) + } + } + } + return getPosition(node, attributePosition, sourceCode) } } }) diff --git a/tests/lib/rules/attributes-order.js b/tests/lib/rules/attributes-order.js index 87e72b56e..399a859b4 100644 --- a/tests/lib/rules/attributes-order.js +++ b/tests/lib/rules/attributes-order.js @@ -373,6 +373,54 @@ tester.run('attributes-order', rule, { `, options: [{ alphabetical: true }] + }, + + // v-bind="..." + { + filename: 'test.vue', + code: ` + `, + options: [{ alphabetical: true }] + }, + { + filename: 'test.vue', + code: ` + `, + options: [{ alphabetical: true }] + }, + { + filename: 'test.vue', + code: ` + `, + options: [{ alphabetical: true }] + }, + { + filename: 'test.vue', + code: ` + `, + options: [{ alphabetical: true }] } ], @@ -943,6 +991,228 @@ tester.run('attributes-order', rule, { message: 'Attribute "v-is" should go before "v-cloak".' } ] + }, + + // v-bind="..." + { + filename: 'test.vue', + code: ` + `, + output: ` + `, + errors: ['Attribute "v-if" should go before "v-bind:id".'] + }, + { + filename: 'test.vue', + code: ` + `, + output: ` + `, + errors: ['Attribute "v-if" should go before "v-bind:id".'] + }, + { + filename: 'test.vue', + code: ` + `, + output: ` + `, + errors: ['Attribute "v-if" should go before "v-bind:id".'] + }, + { + filename: 'test.vue', + code: ` + `, + options: [{ alphabetical: true }], + output: ` + `, + errors: ['Attribute "v-bind:id" should go before "v-on:click".'] + }, + { + filename: 'test.vue', + code: ` + `, + options: [{ alphabetical: true }], + output: ` + `, + errors: ['Attribute "v-bind" should go before "v-on:click".'] + }, + { + filename: 'test.vue', + code: ` + `, + options: [{ alphabetical: true }], + output: ` + `, + errors: ['Attribute "v-bind:id" should go before "v-on:click".'] + }, + { + filename: 'test.vue', + code: ` + `, + options: [{ alphabetical: true }], + output: ` + `, + errors: [ + 'Attribute "v-bind:a" should go before "v-on:click".', + 'Attribute "v-if" should go before "v-on:click".' + ] + }, + { + filename: 'test.vue', + code: ` + `, + options: [{ alphabetical: true }], + output: ` + `, + errors: [ + 'Attribute "v-bind" should go before "v-on:click".', + 'Attribute "v-if" should go before "v-on:click".' + ] + }, + { + filename: 'test.vue', + code: ` + `, + options: [{ alphabetical: true }], + output: ` + `, + errors: ['Attribute "v-if" should go before "a".'] + }, + { + filename: 'test.vue', + code: ` + `, + options: [{ alphabetical: true }], + output: ` + `, + errors: [ + 'Attribute "v-bind" should go before "v-on:click".', + 'Attribute "v-if" should go before "v-on:click".' + ] } ] })