Skip to content

Commit

Permalink
refactor(compiler-core): emit error on v-if key usage
Browse files Browse the repository at this point in the history
  • Loading branch information
@yyx990803
yyx990803 committed Jul 28, 2020
1 parent 355c052 commit 58b4a38
Show file tree
Hide file tree
Showing 4 changed files with 23 additions and 32 deletions.
14 changes: 0 additions & 14 deletions packages/compiler-core/__tests__/transforms/__snapshots__/vIf.spec.ts.snap
View file
Original file line number Diff line number Diff line change
Expand Up @@ -145,17 +145,3 @@ return function render(_ctx, _cache) {
}
}"
`;

exports[`compiler: v-if codegen v-if with key 1`] = `
"const _Vue = Vue
return function render(_ctx, _cache) {
with (_ctx) {
const { createVNode: _createVNode, openBlock: _openBlock, createBlock: _createBlock, createCommentVNode: _createCommentVNode } = _Vue
return ok
? (_openBlock(), _createBlock(\\"div\\", { key: \\"some-key\\" }))
: _createCommentVNode(\\"v-if\\", true)
}
}"
`;
22 changes: 10 additions & 12 deletions packages/compiler-core/__tests__/transforms/vIf.spec.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -282,6 +282,16 @@ describe('compiler: v-if', () => {
}
])
})

test('error on user key', () => {
const onError = jest.fn()
parseWithIfTransform(`<div v-if="ok" :key="1" />`, { onError })
expect(onError.mock.calls[0]).toMatchObject([
{
code: ErrorCodes.X_V_IF_KEY
}
])
})
})

describe('codegen', () => {
Expand Down Expand Up @@ -581,18 +591,6 @@ describe('compiler: v-if', () => {
expect(branch1.props).toMatchObject(createObjectMatcher({ key: `[0]` }))
})

test('v-if with key', () => {
const {
root,
node: { codegenNode }
} = parseWithIfTransform(`<div v-if="ok" key="some-key"/>`)
expect(codegenNode.consequent).toMatchObject({
tag: `"div"`,
props: createObjectMatcher({ key: 'some-key' })
})
expect(generate(root).code).toMatchSnapshot()
})

test('with comments', () => {
const { node } = parseWithIfTransform(`
<template v-if="ok">
Expand Down
2 changes: 2 additions & 0 deletions packages/compiler-core/src/errors.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,7 @@ export const enum ErrorCodes {

// transform errors
X_V_IF_NO_EXPRESSION,
X_V_IF_KEY,
X_V_ELSE_NO_ADJACENT_IF,
X_V_FOR_NO_EXPRESSION,
X_V_FOR_MALFORMED_EXPRESSION,
Expand Down Expand Up @@ -135,6 +136,7 @@ export const errorMessages: { [code: number]: string } = {

// transform errors
[ErrorCodes.X_V_IF_NO_EXPRESSION]: `v-if/v-else-if is missing expression.`,
[ErrorCodes.X_V_IF_KEY]: `v-if branches must use compiler generated keys.`,
[ErrorCodes.X_V_ELSE_NO_ADJACENT_IF]: `v-else/v-else-if has no adjacent v-if.`,
[ErrorCodes.X_V_FOR_NO_EXPRESSION]: `v-for is missing expression.`,
[ErrorCodes.X_V_FOR_MALFORMED_EXPRESSION]: `v-for has invalid expression.`,
Expand Down
17 changes: 11 additions & 6 deletions packages/compiler-core/src/transforms/vIf.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ import {
OPEN_BLOCK,
TELEPORT
} from '../runtimeHelpers'
import { injectProp, findDir } from '../utils'
import { injectProp, findDir, findProp } from '../utils'
import { PatchFlags, PatchFlagNames } from '@vue/shared'

export const transformIf = createStructuralDirectiveTransform(
Expand Down Expand Up @@ -111,6 +111,11 @@ export function processIf(
validateBrowserExpression(dir.exp as SimpleExpressionNode, context)
}

const userKey = /*#__PURE__*/ findProp(node, 'key')
if (userKey) {
context.onError(createCompilerError(ErrorCodes.X_V_IF_KEY, userKey.loc))
}

if (dir.name === 'if') {
const branch = createIfBranch(node, dir)
const ifNode: IfNode = {
Expand Down Expand Up @@ -175,13 +180,13 @@ function createIfBranch(node: ElementNode, dir: DirectiveNode): IfBranchNode {

function createCodegenNodeForBranch(
branch: IfBranchNode,
index: number,
keyIndex: number,
context: TransformContext
): IfConditionalExpression | BlockCodegenNode {
if (branch.condition) {
return createConditionalExpression(
branch.condition,
createChildrenCodegenNode(branch, index, context),
createChildrenCodegenNode(branch, keyIndex, context),
// make sure to pass in asBlock: true so that the comment node call
// closes the current block.
createCallExpression(context.helper(CREATE_COMMENT), [
Expand All @@ -190,19 +195,19 @@ function createCodegenNodeForBranch(
])
) as IfConditionalExpression
} else {
return createChildrenCodegenNode(branch, index, context)
return createChildrenCodegenNode(branch, keyIndex, context)
}
}

function createChildrenCodegenNode(
branch: IfBranchNode,
index: number,
keyIndex: number,
context: TransformContext
): BlockCodegenNode {
const { helper } = context
const keyProperty = createObjectProperty(
`key`,
createSimpleExpression(index + '', false)
createSimpleExpression(`${keyIndex}`, false)
)
const { children } = branch
const firstChild = children[0]
Expand Down

6 comments on commit 58b4a38

@jods4
Copy link
Contributor

@jods4 jods4 commented on 58b4a38 Jul 29, 2020

Choose a reason for hiding this comment

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

This is a breaking change for me.

I have the following use-case:

I'm rendering details of an item.
When the item changes, I don't want the DOM to be updated in-place, rather I want the DOM to be fully removed and replaced by a fresh new DOM.
I achieve this by changing the key of the rendered item.
On top of that, I want to render nothing when no item is selected, so there's a if on the same node.

Something like that:

<comp v-if="data != null" :key="data.id" />

Out of curiosity, why is the key important to v-if?

@posva
Copy link
Member

@posva posva commented on 58b4a38 Jul 29, 2020

Choose a reason for hiding this comment

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

This should be relevant for you #1712 (comment)

@jods4
Copy link
Contributor

@jods4 jods4 commented on 58b4a38 Jul 29, 2020

Choose a reason for hiding this comment

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

@posva thanks it provides some context.

From that comment I get the impression that the issue is having 2 elements with the same key?
Shouldn't that be specifically targeted by the error?

Preventing key on v-if restricts its usefulness and doesn't prevent other silly cases such as <div :key="1" /> <div :key="1" /> and others, possibly less silly and more unfortunate.

In my specific case, is the recommended approach to wrap things in additional elements?

<template v-if="data != null">
  <comp :key="data.id" />
</template>

(Assuming it works, I haven't tested it)

@thecrypticace
Copy link
Contributor

Choose a reason for hiding this comment

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

I also use key with v-if quite a bit with Vue 2. Specially I use it in conjunction with <transition> when the root is not a component so I can be sure the entire element is animated when some state changes and not just patched in-place w/o a transition. What would be the suggestion in that case or does Vue 3 handle that seamlessly already?

@yyx990803
Copy link
Member Author

Choose a reason for hiding this comment

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

See de0c8a7 - it's been fixed to only error on same key for different branches.

@jods4
Copy link
Contributor

@jods4 jods4 commented on 58b4a38 Aug 5, 2020

Choose a reason for hiding this comment

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

@yyx990803 great, thank you!

Please sign in to comment.