From 410151b07f8ee94899dada72478265e97d578986 Mon Sep 17 00:00:00 2001 From: Alex Rickabaugh Date: Tue, 26 Mar 2019 12:18:21 -0700 Subject: [PATCH] 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 --- .../ngtsc/typecheck/src/type_check_block.ts | 49 ++++++++++++------- .../typecheck/test/type_check_block_spec.ts | 12 +++++ 2 files changed, 44 insertions(+), 17 deletions(-) diff --git a/packages/compiler-cli/src/ngtsc/typecheck/src/type_check_block.ts b/packages/compiler-cli/src/ngtsc/typecheck/src/type_check_block.ts index cec092b0b6..3a2e4732e6 100644 --- a/packages/compiler-cli/src/ngtsc/typecheck/src/type_check_block.ts +++ b/packages/compiler-cli/src/ngtsc/typecheck/src/type_check_block.ts @@ -6,7 +6,7 @@ * 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 {NOOP_DEFAULT_IMPORT_RECORDER, Reference, ReferenceEmitter} from '../../imports'; @@ -272,7 +272,7 @@ class TcbDirectiveOp extends TcbOp { class TcbUnclaimedInputsOp extends TcbOp { constructor( private tcb: Context, private scope: Scope, private element: TmplAstElement, - private inputs: Set) { + private claimedInputs: Set) { super(); } @@ -280,14 +280,32 @@ class TcbUnclaimedInputsOp extends TcbOp { // `this.inputs` contains only those bindings not matched by any directive. These bindings go to // the element itself. const elId = this.scope.resolve(this.element); - this.inputs.forEach(name => { - // TODO(alxhub): this could be more efficient. - const binding = this.element.inputs.find(input => input.name === name) !; + + // TODO(alxhub): this could be more efficient. + 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 prop = ts.createPropertyAccess(elId, name); - const assign = ts.createBinary(prop, ts.SyntaxKind.EqualsToken, expr); - this.scope.addStatement(ts.createStatement(assign)); - }); + + if (binding.type === BindingType.Property) { + 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; } @@ -597,13 +615,13 @@ class Scope { private appendDirectivesAndInputsOfNode(node: TmplAstElement|TmplAstTemplate): void { // Collect all the inputs on the element. - const elementInputs = new Set(node.inputs.map(input => input.name)); + const claimedInputs = new Set(); const directives = this.tcb.boundTarget.getDirectivesOfNode(node); if (directives === null || directives.length === 0) { // If there are no directives, then all inputs are unclaimed inputs, so queue an operation // to add them if needed. - if (node instanceof TmplAstElement && elementInputs.size > 0) { - this.opQueue.push(new TcbUnclaimedInputsOp(this.tcb, this, node, elementInputs)); + if (node instanceof TmplAstElement) { + this.opQueue.push(new TcbUnclaimedInputsOp(this.tcb, this, node, claimedInputs)); } return; } @@ -622,14 +640,11 @@ class Scope { for (const dir of directives) { for (const fieldName of Object.keys(dir.inputs)) { 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. - if (elementInputs.size > 0) { - this.opQueue.push(new TcbUnclaimedInputsOp(this.tcb, this, node, elementInputs)); - } + this.opQueue.push(new TcbUnclaimedInputsOp(this.tcb, this, node, claimedInputs)); } } } diff --git a/packages/compiler-cli/src/ngtsc/typecheck/test/type_check_block_spec.ts b/packages/compiler-cli/src/ngtsc/typecheck/test/type_check_block_spec.ts index 985ea2bb5f..0b31ee3ea6 100644 --- a/packages/compiler-cli/src/ngtsc/typecheck/test/type_check_block_spec.ts +++ b/packages/compiler-cli/src/ngtsc/typecheck/test/type_check_block_spec.ts @@ -62,6 +62,18 @@ describe('type check blocks', () => { .toContain( 'var _t1 = i0.Dir.ngTypeCtor({}); _t1.value; var _t2 = document.createElement("div");'); }); + + it('should handle style and class bindings specially', () => { + const TEMPLATE = ` +
+ `; + 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', () => {