From 7ef5d9deefce2ed22fb8a970d02346a33bb96cf0 Mon Sep 17 00:00:00 2001 From: Dylan Hunn Date: Thu, 14 Dec 2023 18:04:35 -0800 Subject: [PATCH] refactor(compiler): Support class and non-class attributes with the same name (#53574) Some elements may have multiple bindings with the same name. We should accept and emit them all, as long as they have different kinds. Co-authored-by: Miles Malerba PR Close #53574 --- .../class_bindings/GOLDEN_PARTIAL.js | 47 +++++++++++++++++++ .../class_bindings/TEST_CASES.json | 18 +++++++ .../class_bindings/shared_name_with_consts.ts | 18 +++++++ .../shared_name_with_consts_template.js | 30 ++++++++++++ ...ared_name_with_consts_template.pipeline.js | 32 +++++++++++++ .../pipeline/src/phases/const_collection.ts | 16 +++++-- .../src/phases/parse_extracted_styles.ts | 22 +++++++++ 7 files changed, 180 insertions(+), 3 deletions(-) create mode 100644 packages/compiler-cli/test/compliance/test_cases/r3_view_compiler_styling/class_bindings/shared_name_with_consts.ts create mode 100644 packages/compiler-cli/test/compliance/test_cases/r3_view_compiler_styling/class_bindings/shared_name_with_consts_template.js create mode 100644 packages/compiler-cli/test/compliance/test_cases/r3_view_compiler_styling/class_bindings/shared_name_with_consts_template.pipeline.js diff --git a/packages/compiler-cli/test/compliance/test_cases/r3_view_compiler_styling/class_bindings/GOLDEN_PARTIAL.js b/packages/compiler-cli/test/compliance/test_cases/r3_view_compiler_styling/class_bindings/GOLDEN_PARTIAL.js index 52f0edaa644..650c62c9e00 100644 --- a/packages/compiler-cli/test/compliance/test_cases/r3_view_compiler_styling/class_bindings/GOLDEN_PARTIAL.js +++ b/packages/compiler-cli/test/compliance/test_cases/r3_view_compiler_styling/class_bindings/GOLDEN_PARTIAL.js @@ -219,3 +219,50 @@ export declare class MyComponent { static ɵcmp: i0.ɵɵComponentDeclaration; } +/**************************************************************************************************** + * PARTIAL FILE: shared_name_with_consts.js + ****************************************************************************************************/ +import { Component } from '@angular/core'; +import * as i0 from "@angular/core"; +export class MyComponent { + constructor() { + this.tabIndex = 0; + } +} +MyComponent.ɵfac = i0.ɵɵngDeclareFactory({ minVersion: "12.0.0", version: "0.0.0-PLACEHOLDER", ngImport: i0, type: MyComponent, deps: [], target: i0.ɵɵFactoryTarget.Component }); +MyComponent.ɵcmp = i0.ɵɵngDeclareComponent({ minVersion: "14.0.0", version: "0.0.0-PLACEHOLDER", type: MyComponent, isStandalone: true, selector: "my-component", ngImport: i0, template: ` +
+
+
+
+
+
+
+`, isInline: true }); +i0.ɵɵngDeclareClassMetadata({ minVersion: "12.0.0", version: "0.0.0-PLACEHOLDER", ngImport: i0, type: MyComponent, decorators: [{ + type: Component, + args: [{ + selector: 'my-component', + standalone: true, + template: ` +
+
+
+
+
+
+
+` + }] + }] }); + +/**************************************************************************************************** + * PARTIAL FILE: shared_name_with_consts.d.ts + ****************************************************************************************************/ +import * as i0 from "@angular/core"; +export declare class MyComponent { + tabIndex: number; + static ɵfac: i0.ɵɵFactoryDeclaration; + static ɵcmp: i0.ɵɵComponentDeclaration; +} + diff --git a/packages/compiler-cli/test/compliance/test_cases/r3_view_compiler_styling/class_bindings/TEST_CASES.json b/packages/compiler-cli/test/compliance/test_cases/r3_view_compiler_styling/class_bindings/TEST_CASES.json index adb4ec394c3..ca8b443f229 100644 --- a/packages/compiler-cli/test/compliance/test_cases/r3_view_compiler_styling/class_bindings/TEST_CASES.json +++ b/packages/compiler-cli/test/compliance/test_cases/r3_view_compiler_styling/class_bindings/TEST_CASES.json @@ -70,6 +70,24 @@ ] } ] + }, + { + "description": "should handle class that shares its name with other const array items", + "inputFiles": [ + "shared_name_with_consts.ts" + ], + "expectations": [ + { + "files": [ + { + "templatePipelineExpected": "shared_name_with_consts_template.pipeline.js", + "expected": "shared_name_with_consts_template.js", + "generated": "shared_name_with_consts.js" + } + ], + "failureMessage": "Incorrect template" + } + ] } ] } diff --git a/packages/compiler-cli/test/compliance/test_cases/r3_view_compiler_styling/class_bindings/shared_name_with_consts.ts b/packages/compiler-cli/test/compliance/test_cases/r3_view_compiler_styling/class_bindings/shared_name_with_consts.ts new file mode 100644 index 00000000000..49a467e6138 --- /dev/null +++ b/packages/compiler-cli/test/compliance/test_cases/r3_view_compiler_styling/class_bindings/shared_name_with_consts.ts @@ -0,0 +1,18 @@ +import {Component} from '@angular/core'; + +@Component({ + selector: 'my-component', + standalone: true, + template: ` +
+
+
+
+
+
+
+` +}) +export class MyComponent { + tabIndex = 0; +} diff --git a/packages/compiler-cli/test/compliance/test_cases/r3_view_compiler_styling/class_bindings/shared_name_with_consts_template.js b/packages/compiler-cli/test/compliance/test_cases/r3_view_compiler_styling/class_bindings/shared_name_with_consts_template.js new file mode 100644 index 00000000000..090b286ed2e --- /dev/null +++ b/packages/compiler-cli/test/compliance/test_cases/r3_view_compiler_styling/class_bindings/shared_name_with_consts_template.js @@ -0,0 +1,30 @@ +consts: () => { + __i18nMsg__('label', [], {}, {}) + return [ + ["attr", "", __AttributeMarker.Classes__, "attr"], + ["ngProjectAs", "selector", __AttributeMarker.ProjectAs__, ["selector"], __AttributeMarker.Classes__, "selector"], + [__AttributeMarker.Classes__, "width", __AttributeMarker.Styles__, "width", "0px"], + [__AttributeMarker.Classes__, "tabindex", __AttributeMarker.Bindings__, "tabindex"], + ["class", "ngIf", __AttributeMarker.Template__, "ngIf"], + ["aria-label", i18n_0, __AttributeMarker.Classes__, "aria-label"], + ["all", "", "ngProjectAs", "all", "style", "all:all", "class", "all", __AttributeMarker.ProjectAs__, ["all"], __AttributeMarker.Bindings__, "all", __AttributeMarker.Template__], + [__AttributeMarker.Classes__, "ngIf"], + ["all", "", "ngProjectAs", "all", __AttributeMarker.ProjectAs__, ["all"], __AttributeMarker.Classes__, "all", __AttributeMarker.Styles__, "all", "all", __AttributeMarker.Bindings__, "all"] + ]; +}, +template: function MyComponent_Template(rf, ctx) { + if (rf & 1) { + i0.ɵɵelement(0, "div", 0)(1, "div", 1)(2, "div", 2)(3, "div", 3); + i0.ɵɵtemplate(4, MyComponent_div_4_Template, 1, 0, "div", 4); + i0.ɵɵelement(5, "div", 5); + i0.ɵɵtemplate(6, MyComponent_div_6_Template, 1, 1, "div", 6); + } + if (rf & 2) { + i0.ɵɵadvance(3); + i0.ɵɵproperty("tabindex", ctx.tabIndex); + i0.ɵɵadvance(1); + i0.ɵɵproperty("ngIf", ctx.cond); + i0.ɵɵadvance(2); + i0.ɵɵproperty("all", ctx.all); + } +} diff --git a/packages/compiler-cli/test/compliance/test_cases/r3_view_compiler_styling/class_bindings/shared_name_with_consts_template.pipeline.js b/packages/compiler-cli/test/compliance/test_cases/r3_view_compiler_styling/class_bindings/shared_name_with_consts_template.pipeline.js new file mode 100644 index 00000000000..876fe77eac4 --- /dev/null +++ b/packages/compiler-cli/test/compliance/test_cases/r3_view_compiler_styling/class_bindings/shared_name_with_consts_template.pipeline.js @@ -0,0 +1,32 @@ +consts: () => { + __i18nMsg__('label', [], {}, {}) + return [ + ["attr", "", __AttributeMarker.Classes__, "attr"], + ["ngProjectAs", "selector", __AttributeMarker.ProjectAs__, ["selector"], __AttributeMarker.Classes__, "selector"], + [__AttributeMarker.Classes__, "width", __AttributeMarker.Styles__, "width", "0px"], + [__AttributeMarker.Classes__, "tabindex", __AttributeMarker.Bindings__, "tabindex"], + ["class", "ngIf", __AttributeMarker.Template__, "ngIf"], + ["aria-label", i18n_0, __AttributeMarker.Classes__, "aria-label"], + // NOTE: We tolerate a slight difference -- we emit `all` as a Template binding + ["all", "", "ngProjectAs", "all", "style", "all:all", "class", "all", __AttributeMarker.ProjectAs__, ["all"], __AttributeMarker.Bindings__, "all", __AttributeMarker.Template__, "all"], + [__AttributeMarker.Classes__, "ngIf"], + ["all", "", "ngProjectAs", "all", __AttributeMarker.ProjectAs__, ["all"], __AttributeMarker.Classes__, "all", __AttributeMarker.Styles__, "all", "all", __AttributeMarker.Bindings__, "all"] + ]; + }, + template: function MyComponent_Template(rf, ctx) { + if (rf & 1) { + i0.ɵɵelement(0, "div", 0)(1, "div", 1)(2, "div", 2)(3, "div", 3); + i0.ɵɵtemplate(4, MyComponent_div_4_Template, 1, 0, "div", 4); + i0.ɵɵelement(5, "div", 5); + i0.ɵɵtemplate(6, MyComponent_div_6_Template, 1, 1, "div", 6); + } + if (rf & 2) { + i0.ɵɵadvance(3); + i0.ɵɵproperty("tabindex", ctx.tabIndex); + i0.ɵɵadvance(1); + i0.ɵɵproperty("ngIf", ctx.cond); + i0.ɵɵadvance(2); + i0.ɵɵproperty("all", ctx.all); + } + } + \ No newline at end of file diff --git a/packages/compiler/src/template/pipeline/src/phases/const_collection.ts b/packages/compiler/src/template/pipeline/src/phases/const_collection.ts index 0e82c6a2b67..b6f700f0d58 100644 --- a/packages/compiler/src/template/pipeline/src/phases/const_collection.ts +++ b/packages/compiler/src/template/pipeline/src/phases/const_collection.ts @@ -73,7 +73,7 @@ const FLYWEIGHT_ARRAY: ReadonlyArray = Object.freeze(); + private known = new Map>(); private byKind = new Map; projectAs: string|null = null; @@ -102,12 +102,22 @@ class ElementAttributes { return this.byKind.get(ir.BindingKind.I18n) ?? FLYWEIGHT_ARRAY; } + isKnown(kind: ir.BindingKind, name: string, value: o.Expression|null) { + const nameToValue = this.known.get(kind) ?? new Set(); + this.known.set(kind, nameToValue); + if (nameToValue.has(name)) { + return true; + } + nameToValue.add(name); + return false; + } + add(kind: ir.BindingKind, name: string, value: o.Expression|null, trustedValueFn: o.Expression|null): void { - if (this.known.has(name)) { + if (this.isKnown(kind, name, value)) { return; } - this.known.add(name); + // TODO: Can this be its own phase if (name === 'ngProjectAs') { if (value === null || !(value instanceof o.LiteralExpr) || (value.value == null) || diff --git a/packages/compiler/src/template/pipeline/src/phases/parse_extracted_styles.ts b/packages/compiler/src/template/pipeline/src/phases/parse_extracted_styles.ts index 6e385faea87..4695046e0d4 100644 --- a/packages/compiler/src/template/pipeline/src/phases/parse_extracted_styles.ts +++ b/packages/compiler/src/template/pipeline/src/phases/parse_extracted_styles.ts @@ -18,10 +18,32 @@ import type {CompilationJob} from '../compilation'; * class property. */ export function parseExtractedStyles(job: CompilationJob) { + const elements = new Map(); + + for (const unit of job.units) { + for (const op of unit.create) { + if (ir.isElementOrContainerOp(op)) { + elements.set(op.xref, op); + } + } + } + for (const unit of job.units) { for (const op of unit.create) { if (op.kind === ir.OpKind.ExtractedAttribute && op.bindingKind === ir.BindingKind.Attribute && ir.isStringLiteral(op.expression!)) { + const target = elements.get(op.target)!; + + if (target !== undefined && target.kind === ir.OpKind.Template && + target.templateKind === ir.TemplateKind.Structural) { + // TemplateDefinitionBuilder will not apply class and style bindings to structural + // directives; instead, it will leave them as attributes. + // (It's not clear what that would mean, anyway -- classes and styles on a structural + // element should probably be a parse error.) + // TODO: We may be able to remove this once Template Pipeline is the default. + continue; + } + if (op.name === 'style') { const parsedStyles = parseStyle(op.expression.value); for (let i = 0; i < parsedStyles.length - 1; i += 2) {