Skip to content

Fix siblings and interleaving issues reported in #2342 #2348

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

Merged
Merged
146 changes: 94 additions & 52 deletions lib/rules/v-if-else-key.js
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,33 @@ const casing = require('../utils/casing')
* @property {VElement | null} else - The node associated with the 'v-else' directive, or null if there isn't one.
*/

/**
* Checks if a given node has sibling nodes of the same type that are also conditionally rendered.
* This is used to determine if multiple instances of the same component are being conditionally
* rendered within the same parent scope.
*
* @param {VElement} node - The Vue component node to check for conditional rendering siblings.
* @param {string} componentName - The name of the component to check for sibling instances.
* @returns {boolean} True if there are sibling nodes of the same type and conditionally rendered, false otherwise.
*/
const hasConditionalRenderedSiblings = (node, componentName) => {
if (node.parent && node.parent.type === 'VElement') {
const siblings = node.parent.children.filter(
(child) => child.type === 'VElement'
)

return siblings.some(
(sibling) =>
sibling !== node &&
sibling.type === 'VElement' &&
sibling.rawName === componentName &&
hasConditionalDirective(sibling)
)
}

return false
}

/**
* Checks for the presence of a 'key' attribute in the given node. If the 'key' attribute is missing
* and the node is part of a conditional family a report is generated.
Expand All @@ -44,35 +71,39 @@ const checkForKey = (
uniqueKey,
conditionalFamilies
) => {
if (node.parent && node.parent.type === 'VElement') {
if (
node.parent &&
node.parent.type === 'VElement' &&
hasConditionalRenderedSiblings(node, componentName)
) {
const conditionalFamily = conditionalFamilies.get(node.parent)

if (
conditionalFamily &&
(utils.hasDirective(node, 'bind', 'key') ||
utils.hasAttribute(node, 'key') ||
!hasConditionalDirective(node) ||
!(conditionalFamily.else || conditionalFamily.elseIf.length > 0))
) {
return
}
if (conditionalFamily && !utils.hasAttribute(node, 'key')) {
let needsKey = false

context.report({
node: node.startTag,
loc: node.startTag.loc,
messageId: 'requireKey',
data: {
componentName
},
fix(fixer) {
const afterComponentNamePosition =
node.startTag.range[0] + componentName.length + 1
return fixer.insertTextBeforeRange(
[afterComponentNamePosition, afterComponentNamePosition],
` key="${uniqueKey}"`
)
if (node === conditionalFamily.if || node === conditionalFamily.else) {
needsKey = true
} else if (conditionalFamily.elseIf.includes(node)) {
needsKey = true
}
})

if (needsKey) {
context.report({
node: node.startTag,
loc: node.startTag.loc,
messageId: 'requireKey',
data: { componentName },
fix(fixer) {
const afterComponentNamePosition =
node.startTag.range[0] + componentName.length + 1
return fixer.insertTextBeforeRange(
[afterComponentNamePosition, afterComponentNamePosition],
` key="${uniqueKey}"`
)
}
})
}
}
}
}

Expand Down Expand Up @@ -189,7 +220,7 @@ module.exports = {
if (node.parent && node.parent.type === 'VElement') {
let conditionalFamily = conditionalFamilies.get(node.parent)

if (conditionType === 'if' && !conditionalFamily) {
if (conditionType === 'if') {
conditionalFamily = createConditionalFamily(node)
conditionalFamilies.set(node.parent, conditionalFamily)
}
Expand Down Expand Up @@ -222,44 +253,55 @@ module.exports = {
const currentScope = componentUsageStack[componentUsageStack.length - 1]
const usageInfo = currentScope.get(componentName) || {
count: 0,
firstNode: null
firstNode: null,
hasIf: false
}

if (hasConditionalDirective(node)) {
// Store the first node if this is the first occurrence
if (usageInfo.count === 0) {
usageInfo.firstNode = node
}
const isIfDirective = utils.getDirective(node, 'if') !== null
const isConditional =
isIfDirective ||
utils.getDirective(node, 'else-if') !== null ||
utils.getDirective(node, 'else') !== null

if (usageInfo.count > 0) {
const uniqueKey = `${casing.kebabCase(componentName)}-${
usageInfo.count + 1
}`
checkForKey(
node,
context,
componentName,
uniqueKey,
conditionalFamilies
)

// If this is the second occurrence, also apply a fix to the first occurrence
if (usageInfo.count === 1) {
const uniqueKeyForFirstInstance = `${casing.kebabCase(
componentName
)}-1`
if (isConditional) {
if (isIfDirective && usageInfo.hasIf) {
// Reset if family already has an 'if' directive
usageInfo.count = 1
usageInfo.firstNode = node
} else {
usageInfo.hasIf = true
if (usageInfo.count === 0) {
usageInfo.firstNode = node
} else if (usageInfo.count > 0 && usageInfo.firstNode !== node) {
const uniqueKey = `${casing.kebabCase(componentName)}-${
usageInfo.count + 1
}`
checkForKey(
usageInfo.firstNode,
node,
context,
componentName,
uniqueKeyForFirstInstance,
uniqueKey,
conditionalFamilies
)

if (usageInfo.count === 1) {
const uniqueKeyForFirstInstance = `${casing.kebabCase(
componentName
)}-1`
checkForKey(
usageInfo.firstNode,
context,
componentName,
uniqueKeyForFirstInstance,
conditionalFamilies
)
}
}
usageInfo.count++
}
usageInfo.count += 1
currentScope.set(componentName, usageInfo)
}

componentUsageStack.push(new Map())
pushedNodes.add(node)
},
Expand Down
140 changes: 140 additions & 0 deletions tests/lib/rules/v-if-else-key.js
Original file line number Diff line number Diff line change
Expand Up @@ -127,6 +127,90 @@ tester.run('v-if-else-key', rule, {
}
</script>
`
},
{
filename: 'test.vue',
code: `
<template>
<div>
<div>
<ComponentA v-if="foo" />
<ComponentB v-else-if="bar" />
</div>
<div>
<div>
<ComponentA v-if="foo" />
<ComponentB v-else-if="baz" />
</div>
<div>
<ComponentA v-if="foo" />
<ComponentB v-else-if="baz" />
</div>
</div>
<div>
<ComponentA v-if="foo" />
<ComponentB v-else />
</div>
<div>
<div v-if="foo" />
<ComponentB v-else />
</div>
</div>
</template>
<script>
export default {
components: {
ComponentA,
ComponentB
}
}
</script>
`
},
{
filename: 'test.vue',
code: `
<template>
<div>
<div>
<CustomComponent v-if="foo" />
<div v-else />
</div>

<CustomComponent v-if="bar" />
</div>
</template>
<script>
export default {
components: {
CustomComponent
}
}
</script>
`
},
{
filename: 'test.vue',
code: `
<template>
<tile>
<template v-if="state === 'foo'">
<ComponentA>…</ComponentA>
<ComponentB>…</ComponentB>
</template>
<ComponentA v-else-if="state === 'bar'" key="a">…</ComponentA>
<ComponentA v-else-if="state === 'bar'" key="b">…</ComponentA>
</tile>
</template>
<script>
export default {
components: {
ComponentA,
ComponentB
}
}
</script>
`
}
],
invalid: [
Expand Down Expand Up @@ -424,6 +508,62 @@ tester.run('v-if-else-key', rule, {
line: 6
}
]
},
{
filename: 'test.vue',
code: `
<template>
<div>
<ComponentA v-if="foo" />

<ComponentA v-if="bar" />
<ComponentA v-else-if="baz" />
<ComponentA v-else />
</div>
</template>
<script>
export default {
components: {
ComponentA
}
}
</script>
`,
output: `
<template>
<div>
<ComponentA v-if="foo" />

<ComponentA key="component-a-1" v-if="bar" />
<ComponentA key="component-a-2" v-else-if="baz" />
<ComponentA key="component-a-3" v-else />
</div>
</template>
<script>
export default {
components: {
ComponentA
}
}
</script>
`,
errors: [
{
message:
"Conditionally rendered repeated component 'ComponentA' expected to have a 'key' attribute.",
line: 6
},
{
message:
"Conditionally rendered repeated component 'ComponentA' expected to have a 'key' attribute.",
line: 7
},
{
message:
"Conditionally rendered repeated component 'ComponentA' expected to have a 'key' attribute.",
line: 8
}
]
}
]
})