From 8976f82bde649d5c9274da9c872035a5e099a1f6 Mon Sep 17 00:00:00 2001 From: Herrington Darkholme Date: Fri, 25 Aug 2017 21:20:38 +0800 Subject: [PATCH 1/6] polish template v-for error report Fix #164, don't report error when nested v-for refers iterator. Do report error when nested v-for doesn't refer iterator. --- lib/rules/valid-v-for.js | 28 ++++++++++++++++++++++------ tests/lib/rules/valid-v-for.js | 9 +++++++++ 2 files changed, 31 insertions(+), 6 deletions(-) diff --git a/lib/rules/valid-v-for.js b/lib/rules/valid-v-for.js index 47d8f5029..25d343f7f 100644 --- a/lib/rules/valid-v-for.js +++ b/lib/rules/valid-v-for.js @@ -19,21 +19,36 @@ const utils = require('../utils') * Check whether the given attribute is using the variables which are defined by `v-for` directives. * @param {ASTNode} vFor The attribute node of `v-for` to check. * @param {ASTNode} vBindKey The attribute node of `v-bind:key` to check. + * @param {boolean} isChild If checks on child in template[v-for]. * @returns {boolean} `true` if the node is using the variables which are defined by `v-for` directives. */ -function isUsingIterationVar (vFor, vBindKey) { +function isUsingIterationVar (vFor, vBindKey, isChild) { if (vBindKey.value == null) { return false } const references = vBindKey.value.references const variables = vFor.parent.parent.variables - - return references.some(reference => + const used = references.some(reference => variables.some(variable => variable.id.name === reference.id.name && variable.kind === 'v-for' ) ) + + if (!isChild || used) { + return used + } + const childFor = utils.getDirective(vBindKey.parent.parent, 'for') + if (!childFor) { + return false + } + const childForRefs = childFor.value.references + return childForRefs.some(cref => + variables.some(variable => + cref.id.name === variable.id.name && + variable.kind === 'v-for' + ) + ) } /** @@ -41,12 +56,13 @@ function isUsingIterationVar (vFor, vBindKey) { * @param {RuleContext} context The rule context to report. * @param {ASTNode} vFor The attribute node of `v-for` to check. * @param {ASTNode} element The element node to check. + * @param {boolean} isChild If element is a child of template[v-for]. */ -function checkKey (context, vFor, element) { +function checkKey (context, vFor, element, isChild) { if (element.name === 'template') { for (const child of element.children) { if (child.type === 'VElement') { - checkKey(context, vFor, child) + checkKey(context, vFor, child, true) } } return @@ -61,7 +77,7 @@ function checkKey (context, vFor, element) { message: "Custom elements in iteration require 'v-bind:key' directives." }) } - if (vBindKey != null && !isUsingIterationVar(vFor, vBindKey)) { + if (vBindKey != null && !isUsingIterationVar(vFor, vBindKey, isChild)) { context.report({ node: vBindKey, loc: vBindKey.loc, diff --git a/tests/lib/rules/valid-v-for.js b/tests/lib/rules/valid-v-for.js index e3da7c5ed..4dc33ddfe 100644 --- a/tests/lib/rules/valid-v-for.js +++ b/tests/lib/rules/valid-v-for.js @@ -82,6 +82,10 @@ tester.run('valid-v-for', rule, { { filename: 'test.vue', code: '' + }, + { + filename: 'test.vue', + code: '' } ], invalid: [ @@ -179,6 +183,11 @@ tester.run('valid-v-for', rule, { filename: 'test.vue', code: '', errors: ["'v-for' directives require that attribute value."] + }, + { + filename: 'test.vue', + code: '', + errors: ["Expected 'v-bind:key' directive to use the variables which are defined by the 'v-for' directive."] } ] }) From c5ef2c4e36126c4aed46afc9c27110c56d007321 Mon Sep 17 00:00:00 2001 From: Herrington Darkholme Date: Sat, 26 Aug 2017 11:26:54 +0800 Subject: [PATCH 2/6] update jsdoc --- lib/rules/valid-v-for.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/rules/valid-v-for.js b/lib/rules/valid-v-for.js index 25d343f7f..42ba4dc36 100644 --- a/lib/rules/valid-v-for.js +++ b/lib/rules/valid-v-for.js @@ -19,7 +19,7 @@ const utils = require('../utils') * Check whether the given attribute is using the variables which are defined by `v-for` directives. * @param {ASTNode} vFor The attribute node of `v-for` to check. * @param {ASTNode} vBindKey The attribute node of `v-bind:key` to check. - * @param {boolean} isChild If checks on child in template[v-for]. + * @param {boolean} [isChild] If checks on child in template[v-for]. * @returns {boolean} `true` if the node is using the variables which are defined by `v-for` directives. */ function isUsingIterationVar (vFor, vBindKey, isChild) { @@ -56,7 +56,7 @@ function isUsingIterationVar (vFor, vBindKey, isChild) { * @param {RuleContext} context The rule context to report. * @param {ASTNode} vFor The attribute node of `v-for` to check. * @param {ASTNode} element The element node to check. - * @param {boolean} isChild If element is a child of template[v-for]. + * @param {boolean} [isChild] If element is a child of template[v-for]. */ function checkKey (context, vFor, element, isChild) { if (element.name === 'template') { From 4bffb08dab73cd28304b46f8711af838e1f214f1 Mon Sep 17 00:00:00 2001 From: Herrington Darkholme Date: Sat, 26 Aug 2017 23:06:55 +0800 Subject: [PATCH 3/6] check iterator usage in new way --- lib/rules/valid-v-for.js | 51 +++++++++++++++++++++------------- tests/lib/rules/valid-v-for.js | 11 ++++++++ 2 files changed, 42 insertions(+), 20 deletions(-) diff --git a/lib/rules/valid-v-for.js b/lib/rules/valid-v-for.js index 42ba4dc36..1f6d527a6 100644 --- a/lib/rules/valid-v-for.js +++ b/lib/rules/valid-v-for.js @@ -19,36 +19,48 @@ const utils = require('../utils') * Check whether the given attribute is using the variables which are defined by `v-for` directives. * @param {ASTNode} vFor The attribute node of `v-for` to check. * @param {ASTNode} vBindKey The attribute node of `v-bind:key` to check. - * @param {boolean} [isChild] If checks on child in template[v-for]. * @returns {boolean} `true` if the node is using the variables which are defined by `v-for` directives. */ -function isUsingIterationVar (vFor, vBindKey, isChild) { +function isUsingIterationVar (vFor, vBindKey) { if (vBindKey.value == null) { return false } const references = vBindKey.value.references const variables = vFor.parent.parent.variables - const used = references.some(reference => + return references.some(reference => variables.some(variable => variable.id.name === reference.id.name && variable.kind === 'v-for' ) ) +} - if (!isChild || used) { - return used - } - const childFor = utils.getDirective(vBindKey.parent.parent, 'for') - if (!childFor) { - return false +/** + * Check the child element in tempalte v-for about `v-bind:key` attributes. + * @param {RuleContext} context The rule context to report. + * @param {ASTNode} vFor The attribute node of `v-for` to check. + * @param {ASTNode} child The child node to check. + */ +function checkChildKey(context, vFor, child) { + let childFor = utils.getDirective(child, 'for') + // if child has v-for, check if parent iterator is used in v-for + if (childFor != null) { + const childForRefs = childFor.value.references + const variables = vFor.parent.parent.variables + const usedInFor = childForRefs.some(cref => + variables.some(variable => + cref.id.name === variable.id.name && + variable.kind === 'v-for' + ) + ) + // if parent iterator is used, skip other checks + // iterator usage will be checked later by child v-for + if (usedInFor) { + return + } } - const childForRefs = childFor.value.references - return childForRefs.some(cref => - variables.some(variable => - cref.id.name === variable.id.name && - variable.kind === 'v-for' - ) - ) + // otherwise, check if parent iterator is directly used in child's key + checkKey(context, vFor, child) } /** @@ -56,13 +68,12 @@ function isUsingIterationVar (vFor, vBindKey, isChild) { * @param {RuleContext} context The rule context to report. * @param {ASTNode} vFor The attribute node of `v-for` to check. * @param {ASTNode} element The element node to check. - * @param {boolean} [isChild] If element is a child of template[v-for]. */ -function checkKey (context, vFor, element, isChild) { +function checkKey (context, vFor, element) { if (element.name === 'template') { for (const child of element.children) { if (child.type === 'VElement') { - checkKey(context, vFor, child, true) + checkChildKey(context, vFor, child) } } return @@ -77,7 +88,7 @@ function checkKey (context, vFor, element, isChild) { message: "Custom elements in iteration require 'v-bind:key' directives." }) } - if (vBindKey != null && !isUsingIterationVar(vFor, vBindKey, isChild)) { + if (vBindKey != null && !isUsingIterationVar(vFor, vBindKey)) { context.report({ node: vBindKey, loc: vBindKey.loc, diff --git a/tests/lib/rules/valid-v-for.js b/tests/lib/rules/valid-v-for.js index 4dc33ddfe..41dabb7e4 100644 --- a/tests/lib/rules/valid-v-for.js +++ b/tests/lib/rules/valid-v-for.js @@ -86,6 +86,17 @@ tester.run('valid-v-for', rule, { { filename: 'test.vue', code: '' + }, + { + filename: 'test.vue', + code: ` +` } ], invalid: [ From c08c6bf9e38e44f6d11e7fe824639da3af94d006 Mon Sep 17 00:00:00 2001 From: Herrington Darkholme Date: Sat, 26 Aug 2017 23:22:57 +0800 Subject: [PATCH 4/6] add more test cases --- tests/lib/rules/valid-v-for.js | 48 ++++++++++++++++++++++++++++++++++ 1 file changed, 48 insertions(+) diff --git a/tests/lib/rules/valid-v-for.js b/tests/lib/rules/valid-v-for.js index 41dabb7e4..75001dbd3 100644 --- a/tests/lib/rules/valid-v-for.js +++ b/tests/lib/rules/valid-v-for.js @@ -96,6 +96,17 @@ tester.run('valid-v-for', rule, { 123 +` + }, + { + filename: 'test.vue', + code: ` +` } ], @@ -199,6 +210,43 @@ tester.run('valid-v-for', rule, { filename: 'test.vue', code: '', errors: ["Expected 'v-bind:key' directive to use the variables which are defined by the 'v-for' directive."] + }, + { + filename: 'test.vue', + errors: ["Expected 'v-bind:key' directive to use the variables which are defined by the 'v-for' directive."], + code: ` +` + }, + { + filename: 'test.vue', + errors: ["Expected 'v-bind:key' directive to use the variables which are defined by the 'v-for' directive."], + code: ` +` + }, + { + filename: 'test.vue', + errors: ["Expected 'v-bind:key' directive to use the variables which are defined by the 'v-for' directive."], + code: ` +` } + ] }) From bb880eaa6a21ee0e139985f62ae531d420cf9ddb Mon Sep 17 00:00:00 2001 From: Herrington Darkholme Date: Sat, 26 Aug 2017 23:24:53 +0800 Subject: [PATCH 5/6] fix code style --- lib/rules/valid-v-for.js | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/lib/rules/valid-v-for.js b/lib/rules/valid-v-for.js index 1f6d527a6..8e20f9e66 100644 --- a/lib/rules/valid-v-for.js +++ b/lib/rules/valid-v-for.js @@ -41,13 +41,13 @@ function isUsingIterationVar (vFor, vBindKey) { * @param {ASTNode} vFor The attribute node of `v-for` to check. * @param {ASTNode} child The child node to check. */ -function checkChildKey(context, vFor, child) { - let childFor = utils.getDirective(child, 'for') +function checkChildKey (context, vFor, child) { + const childFor = utils.getDirective(child, 'for') // if child has v-for, check if parent iterator is used in v-for if (childFor != null) { const childForRefs = childFor.value.references const variables = vFor.parent.parent.variables - const usedInFor = childForRefs.some(cref => + const usedInFor = childForRefs.some(cref => variables.some(variable => cref.id.name === variable.id.name && variable.kind === 'v-for' From a113a05d94719da2d79581b40eacb21f8dcad54c Mon Sep 17 00:00:00 2001 From: Herrington Darkholme Date: Sat, 26 Aug 2017 23:27:15 +0800 Subject: [PATCH 6/6] cosmetic indentation --- tests/lib/rules/valid-v-for.js | 70 +++++++++++++++++----------------- 1 file changed, 35 insertions(+), 35 deletions(-) diff --git a/tests/lib/rules/valid-v-for.js b/tests/lib/rules/valid-v-for.js index 75001dbd3..6fe1f0f1c 100644 --- a/tests/lib/rules/valid-v-for.js +++ b/tests/lib/rules/valid-v-for.js @@ -90,24 +90,24 @@ tester.run('valid-v-for', rule, { { filename: 'test.vue', code: ` -` + ` }, { filename: 'test.vue', code: ` -` + ` } ], invalid: [ @@ -215,37 +215,37 @@ tester.run('valid-v-for', rule, { filename: 'test.vue', errors: ["Expected 'v-bind:key' directive to use the variables which are defined by the 'v-for' directive."], code: ` -` + ` }, { filename: 'test.vue', errors: ["Expected 'v-bind:key' directive to use the variables which are defined by the 'v-for' directive."], code: ` -` + ` }, { filename: 'test.vue', errors: ["Expected 'v-bind:key' directive to use the variables which are defined by the 'v-for' directive."], code: ` -` + ` } ]