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.

Sign up for GitHub

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(compiler-core): warn for empty vOn expression + improve performan… #1719

Closed
wants to merge 2 commits into from
Closed

fix(compiler-core): warn for empty vOn expression + improve performan… #1719

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
5898508
fix(compiler-core): warn for empty vOn expression + improve performan…
underfin Jul 27, 2020
0c4ebb8
refactor: take context arguments first
underfin Jul 27, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 23 additions & 8 deletions packages/compiler-core/src/transforms/hoistStatic.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,7 @@ function walk(
let staticType
if (
!doNotHoistNode &&
(staticType = getStaticType(child, resultCache)) > 0
(staticType = getStaticType(context, child, resultCache)) > 0
) {
if (staticType === StaticType.HAS_RUNTIME_CONSTANT) {
hasRuntimeConstant = true
Expand All @@ -94,7 +94,7 @@ function walk(
flag === PatchFlags.NEED_PATCH ||
flag === PatchFlags.TEXT) &&
!hasDynamicKeyOrRef(child) &&
!hasCachedProps(child)
!hasCachedProps(context, child)
) {
const props = getNodeProps(child)
if (props) {
Expand All @@ -104,7 +104,7 @@ function walk(
}
}
} else if (child.type === NodeTypes.TEXT_CALL) {
const staticType = getStaticType(child.content, resultCache)
const staticType = getStaticType(context, child.content, resultCache)
if (staticType > 0) {
if (staticType === StaticType.HAS_RUNTIME_CONSTANT) {
hasRuntimeConstant = true
Expand Down Expand Up @@ -139,6 +139,7 @@ function walk(
}

export function getStaticType(
context: TransformContext,
node: TemplateChildNode | SimpleExpressionNode,
resultCache: Map<TemplateChildNode, StaticType> = new Map()
): StaticType {
Expand All @@ -156,11 +157,19 @@ export function getStaticType(
return StaticType.NOT_STATIC
}
const flag = getPatchFlag(codegenNode)
if (!flag && !hasDynamicKeyOrRef(node) && !hasCachedProps(node)) {
if (
!flag &&
!hasDynamicKeyOrRef(node) &&
!hasCachedProps(context, node)
) {
// element self is static. check its children.
let returnType = StaticType.FULL_STATIC
for (let i = 0; i < node.children.length; i++) {
const childType = getStaticType(node.children[i], resultCache)
const childType = getStaticType(
context,
node.children[i],
resultCache
)
if (childType === StaticType.NOT_STATIC) {
resultCache.set(node, StaticType.NOT_STATIC)
return StaticType.NOT_STATIC
Expand Down Expand Up @@ -207,7 +216,7 @@ export function getStaticType(
return StaticType.NOT_STATIC
case NodeTypes.INTERPOLATION:
case NodeTypes.TEXT_CALL:
return getStaticType(node.content, resultCache)
return getStaticType(context, node.content, resultCache)
case NodeTypes.SIMPLE_EXPRESSION:
return node.isConstant
? node.isRuntimeConstant
Expand All @@ -221,7 +230,7 @@ export function getStaticType(
if (isString(child) || isSymbol(child)) {
continue
}
const childType = getStaticType(child, resultCache)
const childType = getStaticType(context, child, resultCache)
if (childType === StaticType.NOT_STATIC) {
return StaticType.NOT_STATIC
} else if (childType === StaticType.HAS_RUNTIME_CONSTANT) {
Expand All @@ -242,10 +251,16 @@ function hasDynamicKeyOrRef(node: ElementNode): boolean {
return !!(findProp(node, 'key', true) || findProp(node, 'ref', true))
}

function hasCachedProps(node: PlainElementNode): boolean {
function hasCachedProps(
context: TransformContext,
node: PlainElementNode
): boolean {
if (__BROWSER__) {
return false
}
if (!context.cacheHandlers) {
Copy link
Member

Choose a reason for hiding this comment

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

This check should be performed in all cases because cacheHandlers is not the only mechanism that uses the cache, another example is v-once, and we may add more in the future.

return false
}
const props = getNodeProps(node)
if (props && props.type === NodeTypes.JS_OBJECT_EXPRESSION) {
const { properties } = props
Expand Down
4 changes: 2 additions & 2 deletions packages/compiler-core/src/transforms/transformElement.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -157,7 +157,7 @@ export const transformElement: NodeTransform = (node, context) => {
const hasDynamicTextChild =
type === NodeTypes.INTERPOLATION ||
type === NodeTypes.COMPOUND_EXPRESSION
if (hasDynamicTextChild && !getStaticType(child)) {
if (hasDynamicTextChild && !getStaticType(context, child)) {
patchFlag |= PatchFlags.TEXT
}
// pass directly if the only child is a text node
Expand Down Expand Up @@ -293,7 +293,7 @@ export function buildProps(
value.type === NodeTypes.JS_CACHE_EXPRESSION ||
((value.type === NodeTypes.SIMPLE_EXPRESSION ||
value.type === NodeTypes.COMPOUND_EXPRESSION) &&
getStaticType(value) > 0)
getStaticType(context, value) > 0)
) {
// skip if the prop is a cached handler or has constant value
return
Expand Down
16 changes: 8 additions & 8 deletions packages/compiler-core/src/transforms/vOn.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,13 @@ export const transformOn: DirectiveTransform = (
augmentor
) => {
const { loc, modifiers, arg } = dir as VOnDirectiveNode
if (!dir.exp && !modifiers.length) {
let exp: ExpressionNode | undefined = dir.exp as
| SimpleExpressionNode
| undefined
if (exp && !exp.content.trim()) {
exp = undefined
}
if (!exp && !modifiers.length) {
Copy link
Member

Choose a reason for hiding this comment

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

This check should be performed before setting empty exp to undefined because empty v-on with modifiers are allowed, e.g. @click.prevent

context.onError(createCompilerError(ErrorCodes.X_V_ON_NO_EXPRESSION, loc))
}
let eventName: ExpressionNode
Expand All @@ -62,13 +68,7 @@ export const transformOn: DirectiveTransform = (
}

// handler processing
let exp: ExpressionNode | undefined = dir.exp as
| SimpleExpressionNode
| undefined
if (exp && !exp.content.trim()) {
exp = undefined
}
let isCacheable: boolean = !exp
let isCacheable: boolean = __BROWSER__ ? false : context.cacheHandlers && !exp
if (exp) {
const isMemberExp = isMemberExpression(exp.content)
const isInlineStatement = !(isMemberExp || fnExpRE.test(exp.content))
Expand Down