fix(ivy): check [class] and [style] bindings properly (#29698)

Previously, bindings to [class] and [style] were treated like any other
property binding. That is, they would result in type-checking code that
attempted to write directly to .class or .style on the element node.

This is incorrect, however - the mapping from Angular's [class] and [style]
onto the DOM properties is non-trivial.

For now, this commit avoids the issue by only checking the expressions
themselves and not the assignment to the element properties.

Testing strategy: TCB tests included.

PR Close #29698
This commit is contained in:
Alex Rickabaugh 2019-03-26 12:18:21 -07:00 committed by Ben Lesh
parent 073d258deb
commit 410151b07f
2 changed files with 44 additions and 17 deletions

View File

@ -6,7 +6,7 @@
* found in the LICENSE file at https://angular.io/license * found in the LICENSE file at https://angular.io/license
*/ */
import {AST, BoundTarget, DYNAMIC_TYPE, ExpressionType, ExternalExpr, ImplicitReceiver, PropertyRead, TmplAstBoundAttribute, TmplAstBoundText, TmplAstElement, TmplAstNode, TmplAstReference, TmplAstTemplate, TmplAstTextAttribute, TmplAstVariable, Type} from '@angular/compiler'; import {AST, BindingType, BoundTarget, DYNAMIC_TYPE, ExpressionType, ExternalExpr, ImplicitReceiver, PropertyRead, TmplAstBoundAttribute, TmplAstBoundText, TmplAstElement, TmplAstNode, TmplAstReference, TmplAstTemplate, TmplAstTextAttribute, TmplAstVariable, Type} from '@angular/compiler';
import * as ts from 'typescript'; import * as ts from 'typescript';
import {NOOP_DEFAULT_IMPORT_RECORDER, Reference, ReferenceEmitter} from '../../imports'; import {NOOP_DEFAULT_IMPORT_RECORDER, Reference, ReferenceEmitter} from '../../imports';
@ -272,7 +272,7 @@ class TcbDirectiveOp extends TcbOp {
class TcbUnclaimedInputsOp extends TcbOp { class TcbUnclaimedInputsOp extends TcbOp {
constructor( constructor(
private tcb: Context, private scope: Scope, private element: TmplAstElement, private tcb: Context, private scope: Scope, private element: TmplAstElement,
private inputs: Set<string>) { private claimedInputs: Set<string>) {
super(); super();
} }
@ -280,14 +280,32 @@ class TcbUnclaimedInputsOp extends TcbOp {
// `this.inputs` contains only those bindings not matched by any directive. These bindings go to // `this.inputs` contains only those bindings not matched by any directive. These bindings go to
// the element itself. // the element itself.
const elId = this.scope.resolve(this.element); const elId = this.scope.resolve(this.element);
this.inputs.forEach(name => {
// TODO(alxhub): this could be more efficient. // TODO(alxhub): this could be more efficient.
const binding = this.element.inputs.find(input => input.name === name) !; for (const binding of this.element.inputs) {
if (binding.type === BindingType.Property && this.claimedInputs.has(binding.name)) {
// Skip this binding as it was claimed by a directive.
continue;
}
const expr = tcbExpression(binding.value, this.tcb, this.scope); const expr = tcbExpression(binding.value, this.tcb, this.scope);
const prop = ts.createPropertyAccess(elId, name);
const assign = ts.createBinary(prop, ts.SyntaxKind.EqualsToken, expr); if (binding.type === BindingType.Property) {
this.scope.addStatement(ts.createStatement(assign)); if (binding.name !== 'style' && binding.name !== 'class') {
}); // A direct binding to a property.
const prop = ts.createPropertyAccess(elId, binding.name);
const assign = ts.createBinary(prop, ts.SyntaxKind.EqualsToken, expr);
this.scope.addStatement(ts.createStatement(assign));
} else {
this.scope.addStatement(ts.createExpressionStatement(expr));
}
} else {
// A binding to an animation, attribute, class or style. For now, only validate the right-
// hand side of the expression.
// TODO: properly check class and style bindings.
this.scope.addStatement(ts.createExpressionStatement(expr));
}
}
return null; return null;
} }
@ -597,13 +615,13 @@ class Scope {
private appendDirectivesAndInputsOfNode(node: TmplAstElement|TmplAstTemplate): void { private appendDirectivesAndInputsOfNode(node: TmplAstElement|TmplAstTemplate): void {
// Collect all the inputs on the element. // Collect all the inputs on the element.
const elementInputs = new Set<string>(node.inputs.map(input => input.name)); const claimedInputs = new Set<string>();
const directives = this.tcb.boundTarget.getDirectivesOfNode(node); const directives = this.tcb.boundTarget.getDirectivesOfNode(node);
if (directives === null || directives.length === 0) { if (directives === null || directives.length === 0) {
// If there are no directives, then all inputs are unclaimed inputs, so queue an operation // If there are no directives, then all inputs are unclaimed inputs, so queue an operation
// to add them if needed. // to add them if needed.
if (node instanceof TmplAstElement && elementInputs.size > 0) { if (node instanceof TmplAstElement) {
this.opQueue.push(new TcbUnclaimedInputsOp(this.tcb, this, node, elementInputs)); this.opQueue.push(new TcbUnclaimedInputsOp(this.tcb, this, node, claimedInputs));
} }
return; return;
} }
@ -622,14 +640,11 @@ class Scope {
for (const dir of directives) { for (const dir of directives) {
for (const fieldName of Object.keys(dir.inputs)) { for (const fieldName of Object.keys(dir.inputs)) {
const value = dir.inputs[fieldName]; const value = dir.inputs[fieldName];
elementInputs.delete(Array.isArray(value) ? value[0] : value); claimedInputs.add(Array.isArray(value) ? value[0] : value);
} }
} }
// If any are left over, queue a `TcbUnclaimedInputsOp` to check them. this.opQueue.push(new TcbUnclaimedInputsOp(this.tcb, this, node, claimedInputs));
if (elementInputs.size > 0) {
this.opQueue.push(new TcbUnclaimedInputsOp(this.tcb, this, node, elementInputs));
}
} }
} }
} }

View File

@ -62,6 +62,18 @@ describe('type check blocks', () => {
.toContain( .toContain(
'var _t1 = i0.Dir.ngTypeCtor({}); _t1.value; var _t2 = document.createElement("div");'); 'var _t1 = i0.Dir.ngTypeCtor({}); _t1.value; var _t2 = document.createElement("div");');
}); });
it('should handle style and class bindings specially', () => {
const TEMPLATE = `
<div [style]="a" [class]="b"></div>
`;
const block = tcb(TEMPLATE);
expect(block).toContain('ctx.a; ctx.b;');
// There should be no assignments to the class or style properties.
expect(block).not.toContain('.class = ');
expect(block).not.toContain('.style = ');
});
}); });
it('should generate a circular directive reference correctly', () => { it('should generate a circular directive reference correctly', () => {