From 1f0bfbc6d5a3d5b163e67743807cd0e0f4708d40 Mon Sep 17 00:00:00 2001 From: Kristiyan Kostadinov Date: Fri, 17 Nov 2023 20:34:08 +0100 Subject: [PATCH 1/4] Revert "refactor(compiler): expose utility for creating CSS selectors from AST nodes (#52726)" This reverts commit adbea7befbc372c93b17c7d978356d1e2dce99bc. --- packages/compiler/src/compiler.ts | 1 - .../compiler/src/render3/view/t2_binder.ts | 11 +++---- .../compiler/src/render3/view/template.ts | 24 +++++++++++++++ packages/compiler/src/render3/view/util.ts | 30 ++----------------- 4 files changed, 32 insertions(+), 34 deletions(-) diff --git a/packages/compiler/src/compiler.ts b/packages/compiler/src/compiler.ts index f806a01b4b3e..3b315da5f7d9 100644 --- a/packages/compiler/src/compiler.ts +++ b/packages/compiler/src/compiler.ts @@ -67,7 +67,6 @@ export * from './render3/view/api'; export {BoundAttribute as TmplAstBoundAttribute, BoundEvent as TmplAstBoundEvent, BoundText as TmplAstBoundText, Content as TmplAstContent, Element as TmplAstElement, Icu as TmplAstIcu, Node as TmplAstNode, RecursiveVisitor as TmplAstRecursiveVisitor, Reference as TmplAstReference, Template as TmplAstTemplate, Text as TmplAstText, TextAttribute as TmplAstTextAttribute, Variable as TmplAstVariable, DeferredBlock as TmplAstDeferredBlock, DeferredBlockPlaceholder as TmplAstDeferredBlockPlaceholder, DeferredBlockLoading as TmplAstDeferredBlockLoading, DeferredBlockError as TmplAstDeferredBlockError, DeferredTrigger as TmplAstDeferredTrigger, BoundDeferredTrigger as TmplAstBoundDeferredTrigger, IdleDeferredTrigger as TmplAstIdleDeferredTrigger, ImmediateDeferredTrigger as TmplAstImmediateDeferredTrigger, HoverDeferredTrigger as TmplAstHoverDeferredTrigger, TimerDeferredTrigger as TmplAstTimerDeferredTrigger, InteractionDeferredTrigger as TmplAstInteractionDeferredTrigger, ViewportDeferredTrigger as TmplAstViewportDeferredTrigger, SwitchBlock as TmplAstSwitchBlock, SwitchBlockCase as TmplAstSwitchBlockCase, ForLoopBlock as TmplAstForLoopBlock, ForLoopBlockEmpty as TmplAstForLoopBlockEmpty, IfBlock as TmplAstIfBlock, IfBlockBranch as TmplAstIfBlockBranch, DeferredBlockTriggers as TmplAstDeferredBlockTriggers, UnknownBlock as TmplAstUnknownBlock} from './render3/r3_ast'; export * from './render3/view/t2_api'; export * from './render3/view/t2_binder'; -export {createCssSelectorFromNode} from './render3/view/util'; export {Identifiers as R3Identifiers} from './render3/r3_identifiers'; export {R3ClassMetadata, CompileClassMetadataFn, compileClassMetadata, compileComponentClassMetadata} from './render3/r3_class_metadata_compiler'; export {compileClassDebugInfo, R3ClassDebugInfo} from './render3/r3_class_debug_info_compiler'; diff --git a/packages/compiler/src/render3/view/t2_binder.ts b/packages/compiler/src/render3/view/t2_binder.ts index 2ef5e5fe5745..1355b49bcb18 100644 --- a/packages/compiler/src/render3/view/t2_binder.ts +++ b/packages/compiler/src/render3/view/t2_binder.ts @@ -11,7 +11,8 @@ import {SelectorMatcher} from '../../selector'; import {BoundAttribute, BoundEvent, BoundText, Comment, Content, DeferredBlock, DeferredBlockError, DeferredBlockLoading, DeferredBlockPlaceholder, DeferredTrigger, Element, ForLoopBlock, ForLoopBlockEmpty, HoverDeferredTrigger, Icu, IfBlock, IfBlockBranch, InteractionDeferredTrigger, Node, Reference, SwitchBlock, SwitchBlockCase, Template, Text, TextAttribute, UnknownBlock, Variable, ViewportDeferredTrigger, Visitor} from '../r3_ast'; import {BoundTarget, DirectiveMeta, ReferenceTarget, ScopedNode, Target, TargetBinder} from './t2_api'; -import {createCssSelectorFromNode} from './util'; +import {createCssSelector} from './template'; +import {getAttrsForDirectiveMatching} from './util'; /** * Processes `Target`s with a given set of directives and performs a binding operation, which @@ -306,17 +307,17 @@ class DirectiveBinder implements Visitor { } visitElement(element: Element): void { - this.visitElementOrTemplate(element); + this.visitElementOrTemplate(element.name, element); } visitTemplate(template: Template): void { - this.visitElementOrTemplate(template); + this.visitElementOrTemplate('ng-template', template); } - visitElementOrTemplate(node: Element|Template): void { + visitElementOrTemplate(elementName: string, node: Element|Template): void { // First, determine the HTML shape of the node for the purpose of directive matching. // Do this by building up a `CssSelector` for the node. - const cssSelector = createCssSelectorFromNode(node); + const cssSelector = createCssSelector(elementName, getAttrsForDirectiveMatching(node)); // Next, use the `SelectorMatcher` to get the list of directives on the node. const directives: DirectiveT[] = []; diff --git a/packages/compiler/src/render3/view/template.ts b/packages/compiler/src/render3/view/template.ts index 1f33e720de40..3854cadb8329 100644 --- a/packages/compiler/src/render3/view/template.ts +++ b/packages/compiler/src/render3/view/template.ts @@ -2570,6 +2570,30 @@ class TrackByBindingScope extends BindingScope { } } +/** + * Creates a `CssSelector` given a tag name and a map of attributes + */ +export function createCssSelector( + elementName: string, attributes: {[name: string]: string}): CssSelector { + const cssSelector = new CssSelector(); + const elementNameNoNs = splitNsName(elementName)[1]; + + cssSelector.setElement(elementNameNoNs); + + Object.getOwnPropertyNames(attributes).forEach((name) => { + const nameNoNs = splitNsName(name)[1]; + const value = attributes[name]; + + cssSelector.addAttribute(nameNoNs, value); + if (name.toLowerCase() === 'class') { + const classes = value.trim().split(/\s+/); + classes.forEach(className => cssSelector.addClassName(className)); + } + }); + + return cssSelector; +} + /** * Creates an array of expressions out of an `ngProjectAs` attributes * which can be added to the instruction parameters. diff --git a/packages/compiler/src/render3/view/util.ts b/packages/compiler/src/render3/view/util.ts index d2288ad2b7be..386af5048c90 100644 --- a/packages/compiler/src/render3/view/util.ts +++ b/packages/compiler/src/render3/view/util.ts @@ -8,10 +8,8 @@ import {ConstantPool} from '../../constant_pool'; import {BindingType, Interpolation} from '../../expression_parser/ast'; -import {splitNsName} from '../../ml_parser/tags'; import * as o from '../../output/output_ast'; import {ParseSourceSpan} from '../../parse_util'; -import {CssSelector} from '../../selector'; import * as t from '../r3_ast'; import {Identifiers as R3} from '../r3_identifiers'; import {ForwardRefHandling} from '../util'; @@ -279,31 +277,6 @@ export class DefinitionMap { } } -/** - * Creates a `CssSelector` from an AST node. - */ -export function createCssSelectorFromNode(node: t.Element|t.Template): CssSelector { - const elementName = node instanceof t.Element ? node.name : 'ng-template'; - const attributes = getAttrsForDirectiveMatching(node); - const cssSelector = new CssSelector(); - const elementNameNoNs = splitNsName(elementName)[1]; - - cssSelector.setElement(elementNameNoNs); - - Object.getOwnPropertyNames(attributes).forEach((name) => { - const nameNoNs = splitNsName(name)[1]; - const value = attributes[name]; - - cssSelector.addAttribute(nameNoNs, value); - if (name.toLowerCase() === 'class') { - const classes = value.trim().split(/\s+/); - classes.forEach(className => cssSelector.addClassName(className)); - } - }); - - return cssSelector; -} - /** * Extract a map of properties to values for a given element or template node, which can be used * by the directive matching machinery. @@ -313,7 +286,8 @@ export function createCssSelectorFromNode(node: t.Element|t.Template): CssSelect * object maps a property name to its (static) value. For any bindings, this map simply maps the * property name to an empty string. */ -function getAttrsForDirectiveMatching(elOrTpl: t.Element|t.Template): {[name: string]: string} { +export function getAttrsForDirectiveMatching(elOrTpl: t.Element| + t.Template): {[name: string]: string} { const attributesMap: {[name: string]: string} = {}; From 0f3111248b5928256e0e8879d0b71993dff7f5c9 Mon Sep 17 00:00:00 2001 From: Kristiyan Kostadinov Date: Fri, 17 Nov 2023 20:34:19 +0100 Subject: [PATCH 2/4] Revert "refactor(compiler-cli): expose ng-content selectors and preserveWhitespaces during template type checking (#52726)" This reverts commit 4550a81bdc130b2ea6bd365e9d5547afbf8850b2. --- .../ngtsc/annotations/component/src/handler.ts | 9 +++++---- .../ngtsc/annotations/directive/src/handler.ts | 2 -- .../annotations/directive/test/directive_spec.ts | 2 -- .../compiler-cli/src/ngtsc/indexer/test/util.ts | 2 -- .../compiler-cli/src/ngtsc/metadata/src/dts.ts | 7 ------- .../src/ngtsc/scope/test/local_spec.ts | 2 -- .../compiler-cli/src/ngtsc/typecheck/api/api.ts | 5 ----- .../src/ngtsc/typecheck/api/context.ts | 4 +--- .../src/ngtsc/typecheck/src/context.ts | 5 ++--- .../src/ngtsc/typecheck/src/type_check_block.ts | 5 ++--- .../src/ngtsc/typecheck/testing/index.ts | 11 ++--------- packages/compiler/src/render3/view/api.ts | 5 ----- packages/compiler/src/render3/view/t2_api.ts | 13 ------------- .../compiler/test/render3/view/binding_spec.ts | 16 ---------------- 14 files changed, 12 insertions(+), 76 deletions(-) diff --git a/packages/compiler-cli/src/ngtsc/annotations/component/src/handler.ts b/packages/compiler-cli/src/ngtsc/annotations/component/src/handler.ts index 402e7ffe1c0d..49592bccb109 100644 --- a/packages/compiler-cli/src/ngtsc/annotations/component/src/handler.ts +++ b/packages/compiler-cli/src/ngtsc/annotations/component/src/handler.ts @@ -463,7 +463,10 @@ export class ComponentDecoratorHandler implements rawHostDirectives, meta: { ...metadata, - template, + template: { + nodes: template.nodes, + ngContentSelectors: template.ngContentSelectors, + }, encapsulation, changeDetection, interpolation: template.interpolationConfig ?? DEFAULT_INTERPOLATION_CONFIG, @@ -543,8 +546,6 @@ export class ComponentDecoratorHandler implements schemas: analysis.schemas, decorator: analysis.decorator, assumedToExportProviders: false, - ngContentSelectors: analysis.template.ngContentSelectors, - preserveWhitespaces: analysis.template.preserveWhitespaces ?? false, }); this.resourceRegistry.registerResources(analysis.resources, node); @@ -613,7 +614,7 @@ export class ComponentDecoratorHandler implements ctx.addTemplate( new Reference(node), binder, meta.template.diagNodes, scope.pipes, scope.schemas, meta.template.sourceMapping, meta.template.file, meta.template.errors, - meta.meta.isStandalone, meta.meta.template.preserveWhitespaces ?? false); + meta.meta.isStandalone); } extendedTemplateCheck( diff --git a/packages/compiler-cli/src/ngtsc/annotations/directive/src/handler.ts b/packages/compiler-cli/src/ngtsc/annotations/directive/src/handler.ts index b17630770cbf..0c7310711fbd 100644 --- a/packages/compiler-cli/src/ngtsc/annotations/directive/src/handler.ts +++ b/packages/compiler-cli/src/ngtsc/annotations/directive/src/handler.ts @@ -169,9 +169,7 @@ export class DirectiveDecoratorHandler implements isSignal: analysis.meta.isSignal, imports: null, schemas: null, - ngContentSelectors: null, decorator: analysis.decorator, - preserveWhitespaces: false, // Directives analyzed within our own compilation are not _assumed_ to export providers. // Instead, we statically analyze their imports to make a direct determination. assumedToExportProviders: false, diff --git a/packages/compiler-cli/src/ngtsc/annotations/directive/test/directive_spec.ts b/packages/compiler-cli/src/ngtsc/annotations/directive/test/directive_spec.ts index b0cede22afc2..6b2f2308fdd0 100644 --- a/packages/compiler-cli/src/ngtsc/annotations/directive/test/directive_spec.ts +++ b/packages/compiler-cli/src/ngtsc/annotations/directive/test/directive_spec.ts @@ -110,8 +110,6 @@ runInEachFileSystem(() => { selector: '[dir]', isStructural: false, animationTriggerNames: null, - ngContentSelectors: null, - preserveWhitespaces: false, }; matcher.addSelectables(CssSelector.parse('[dir]'), [dirMeta]); diff --git a/packages/compiler-cli/src/ngtsc/indexer/test/util.ts b/packages/compiler-cli/src/ngtsc/indexer/test/util.ts index 2907c6291e71..e2d287758de3 100644 --- a/packages/compiler-cli/src/ngtsc/indexer/test/util.ts +++ b/packages/compiler-cli/src/ngtsc/indexer/test/util.ts @@ -56,8 +56,6 @@ export function getBoundTemplate( exportAs: null, isStructural: false, animationTriggerNames: null, - ngContentSelectors: null, - preserveWhitespaces: false, }]); }); const binder = new R3TargetBinder(matcher); diff --git a/packages/compiler-cli/src/ngtsc/metadata/src/dts.ts b/packages/compiler-cli/src/ngtsc/metadata/src/dts.ts index 3f3d4ce29ce6..eb91b3bb2b84 100644 --- a/packages/compiler-cli/src/ngtsc/metadata/src/dts.ts +++ b/packages/compiler-cli/src/ngtsc/metadata/src/dts.ts @@ -96,9 +96,6 @@ export class DtsMetadataReader implements MetadataReader { param.typeValueReference.importedName === 'TemplateRef'; }); - const ngContentSelectors = - def.type.typeArguments.length > 6 ? readStringArrayType(def.type.typeArguments[6]) : null; - const isStandalone = def.type.typeArguments.length > 7 && (readBooleanType(def.type.typeArguments[7]) ?? false); @@ -129,7 +126,6 @@ export class DtsMetadataReader implements MetadataReader { isPoisoned: false, isStructural, animationTriggerNames: null, - ngContentSelectors, isStandalone, isSignal, // Imports are tracked in metadata only for template type-checking purposes, @@ -140,9 +136,6 @@ export class DtsMetadataReader implements MetadataReader { decorator: null, // Assume that standalone components from .d.ts files may export providers. assumedToExportProviders: isComponent && isStandalone, - // `preserveWhitespaces` isn't encoded in the .d.ts and is only - // used to increase the accuracy of a diagnostic. - preserveWhitespaces: false, }; } diff --git a/packages/compiler-cli/src/ngtsc/scope/test/local_spec.ts b/packages/compiler-cli/src/ngtsc/scope/test/local_spec.ts index 64aa131addb1..14f54199b34d 100644 --- a/packages/compiler-cli/src/ngtsc/scope/test/local_spec.ts +++ b/packages/compiler-cli/src/ngtsc/scope/test/local_spec.ts @@ -330,8 +330,6 @@ function fakeDirective(ref: Reference): DirectiveMeta { decorator: null, hostDirectives: null, assumedToExportProviders: false, - ngContentSelectors: null, - preserveWhitespaces: false, }; } diff --git a/packages/compiler-cli/src/ngtsc/typecheck/api/api.ts b/packages/compiler-cli/src/ngtsc/typecheck/api/api.ts index 11e82931d560..5bdc53ef0409 100644 --- a/packages/compiler-cli/src/ngtsc/typecheck/api/api.ts +++ b/packages/compiler-cli/src/ngtsc/typecheck/api/api.ts @@ -84,11 +84,6 @@ export interface TypeCheckBlockMetadata { * A boolean indicating whether the component is standalone. */ isStandalone: boolean; - - /** - * A boolean indicating whether the component preserves whitespaces in its template. - */ - preserveWhitespaces: boolean; } export interface TypeCtorMetadata { diff --git a/packages/compiler-cli/src/ngtsc/typecheck/api/context.ts b/packages/compiler-cli/src/ngtsc/typecheck/api/context.ts index 7967fb007dc1..747477a0dc04 100644 --- a/packages/compiler-cli/src/ngtsc/typecheck/api/context.ts +++ b/packages/compiler-cli/src/ngtsc/typecheck/api/context.ts @@ -37,15 +37,13 @@ export interface TypeCheckContext { * @param file the `ParseSourceFile` associated with the template. * @param parseErrors the `ParseError`'s associated with the template. * @param isStandalone a boolean indicating whether the component is standalone. - * @param preserveWhitespaces a boolean indicating whether the component's template preserves - * whitespaces. */ addTemplate( ref: Reference>, binder: R3TargetBinder, template: TmplAstNode[], pipes: Map>>, schemas: SchemaMetadata[], sourceMapping: TemplateSourceMapping, file: ParseSourceFile, - parseErrors: ParseError[]|null, isStandalone: boolean, preserveWhitespaces: boolean): void; + parseErrors: ParseError[]|null, isStandalone: boolean): void; } /** diff --git a/packages/compiler-cli/src/ngtsc/typecheck/src/context.ts b/packages/compiler-cli/src/ngtsc/typecheck/src/context.ts index c9fdd2e2fb93..075e7d28c03e 100644 --- a/packages/compiler-cli/src/ngtsc/typecheck/src/context.ts +++ b/packages/compiler-cli/src/ngtsc/typecheck/src/context.ts @@ -210,7 +210,7 @@ export class TypeCheckContextImpl implements TypeCheckContext { binder: R3TargetBinder, template: TmplAstNode[], pipes: Map>>, schemas: SchemaMetadata[], sourceMapping: TemplateSourceMapping, file: ParseSourceFile, - parseErrors: ParseError[]|null, isStandalone: boolean, preserveWhitespaces: boolean): void { + parseErrors: ParseError[]|null, isStandalone: boolean): void { if (!this.host.shouldCheckComponent(ref.node)) { return; } @@ -293,8 +293,7 @@ export class TypeCheckContextImpl implements TypeCheckContext { boundTarget, pipes, schemas, - isStandalone, - preserveWhitespaces, + isStandalone }; this.perf.eventCount(PerfEvent.GenerateTcb); if (inliningRequirement !== TcbInliningRequirement.None && 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 531f5a6c3b73..c56e0bebece8 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 @@ -84,7 +84,7 @@ export function generateTypeCheckBlock( genericContextBehavior: TcbGenericContextBehavior): ts.FunctionDeclaration { const tcb = new Context( env, domSchemaChecker, oobRecorder, meta.id, meta.boundTarget, meta.pipes, meta.schemas, - meta.isStandalone, meta.preserveWhitespaces); + meta.isStandalone); const scope = Scope.forNodes(tcb, null, null, tcb.boundTarget.target.template!, /* guard */ null); const ctxRawType = env.referenceType(ref); if (!ts.isTypeReferenceNode(ctxRawType)) { @@ -1582,8 +1582,7 @@ export class Context { readonly oobRecorder: OutOfBandDiagnosticRecorder, readonly id: TemplateId, readonly boundTarget: BoundTarget, private pipes: Map>>, - readonly schemas: SchemaMetadata[], readonly hostIsStandalone: boolean, - readonly hostPreserveWhitespaces: boolean) {} + readonly schemas: SchemaMetadata[], readonly hostIsStandalone: boolean) {} /** * Allocate a new variable name for use within the `Context`. diff --git a/packages/compiler-cli/src/ngtsc/typecheck/testing/index.ts b/packages/compiler-cli/src/ngtsc/typecheck/testing/index.ts index f72cde49c7ed..46ba48449631 100644 --- a/packages/compiler-cli/src/ngtsc/typecheck/testing/index.ts +++ b/packages/compiler-cli/src/ngtsc/typecheck/testing/index.ts @@ -249,8 +249,6 @@ export interface TestDirective extends Partial { return { directive: new Reference(resolveDeclaration(hostDecl.directive)), @@ -737,8 +732,6 @@ function makeScope(program: ts.Program, sf: ts.SourceFile, decls: TestDeclaratio schemas: null, decorator: null, assumedToExportProviders: false, - ngContentSelectors: decl.ngContentSelectors || null, - preserveWhitespaces: decl.preserveWhitespaces ?? false, hostDirectives: decl.hostDirectives === undefined ? null : decl.hostDirectives.map(hostDecl => { return { diff --git a/packages/compiler/src/render3/view/api.ts b/packages/compiler/src/render3/view/api.ts index bee4ce5b0128..74363df18e8d 100644 --- a/packages/compiler/src/render3/view/api.ts +++ b/packages/compiler/src/render3/view/api.ts @@ -226,11 +226,6 @@ export interface R3ComponentMetadata * element without selector is present. */ ngContentSelectors: string[]; - - /** - * Whether the template preserves whitespaces from the user's code. - */ - preserveWhitespaces?: boolean; }; declarations: DeclarationT[]; diff --git a/packages/compiler/src/render3/view/t2_api.ts b/packages/compiler/src/render3/view/t2_api.ts index 689d28cc381c..f4e9662948d0 100644 --- a/packages/compiler/src/render3/view/t2_api.ts +++ b/packages/compiler/src/render3/view/t2_api.ts @@ -93,21 +93,8 @@ export interface DirectiveMeta { */ exportAs: string[]|null; - /** - * Whether the directive is a structural directive (e.g. `
`). - */ isStructural: boolean; - /** - * If the directive is a component, includes the selectors of its `ng-content` elements. - */ - ngContentSelectors: string[]|null; - - /** - * Whether the template of the component preserves whitespaces. - */ - preserveWhitespaces: boolean; - /** * The name of animations that the user defines in the component. * Only includes the animation names. diff --git a/packages/compiler/test/render3/view/binding_spec.ts b/packages/compiler/test/render3/view/binding_spec.ts index 46c6a09258b4..eb682c3fd2b6 100644 --- a/packages/compiler/test/render3/view/binding_spec.ts +++ b/packages/compiler/test/render3/view/binding_spec.ts @@ -41,8 +41,6 @@ function makeSelectorMatcher(): SelectorMatcher { isStructural: true, selector: '[ngFor][ngForOf]', animationTriggerNames: null, - ngContentSelectors: null, - preserveWhitespaces: false, }]); matcher.addSelectables(CssSelector.parse('[dir]'), [{ name: 'Dir', @@ -53,8 +51,6 @@ function makeSelectorMatcher(): SelectorMatcher { isStructural: false, selector: '[dir]', animationTriggerNames: null, - ngContentSelectors: null, - preserveWhitespaces: false, }]); matcher.addSelectables(CssSelector.parse('[hasOutput]'), [{ name: 'HasOutput', @@ -65,8 +61,6 @@ function makeSelectorMatcher(): SelectorMatcher { isStructural: false, selector: '[hasOutput]', animationTriggerNames: null, - ngContentSelectors: null, - preserveWhitespaces: false, }]); matcher.addSelectables(CssSelector.parse('[hasInput]'), [{ name: 'HasInput', @@ -77,8 +71,6 @@ function makeSelectorMatcher(): SelectorMatcher { isStructural: false, selector: '[hasInput]', animationTriggerNames: null, - ngContentSelectors: null, - preserveWhitespaces: false, }]); matcher.addSelectables(CssSelector.parse('[sameSelectorAsInput]'), [{ name: 'SameSelectorAsInput', @@ -89,8 +81,6 @@ function makeSelectorMatcher(): SelectorMatcher { isStructural: false, selector: '[sameSelectorAsInput]', animationTriggerNames: null, - ngContentSelectors: null, - preserveWhitespaces: false, }]); matcher.addSelectables(CssSelector.parse('comp'), [{ name: 'Comp', @@ -101,8 +91,6 @@ function makeSelectorMatcher(): SelectorMatcher { isStructural: false, selector: 'comp', animationTriggerNames: null, - ngContentSelectors: null, - preserveWhitespaces: false, }]); const simpleDirectives = ['a', 'b', 'c', 'd', 'e', 'f']; @@ -118,8 +106,6 @@ function makeSelectorMatcher(): SelectorMatcher { isStructural: true, selector: `[${dir}]`, animationTriggerNames: null, - ngContentSelectors: null, - preserveWhitespaces: false, }]); } @@ -169,8 +155,6 @@ describe('t2 binding', () => { isStructural: false, selector: 'text[dir]', animationTriggerNames: null, - ngContentSelectors: null, - preserveWhitespaces: false, }]); const binder = new R3TargetBinder(matcher); const res = binder.bind({template: template.nodes}); From f66c503ae9971ee9a802d560e08ac13b5aefd66e Mon Sep 17 00:00:00 2001 From: Kristiyan Kostadinov Date: Fri, 17 Nov 2023 20:34:30 +0100 Subject: [PATCH 3/4] Revert "test(core): add tests for control flow content projection with ng-container (#52726)" This reverts commit 181f8e4b6c5303254b575e9c468d91b9c3fcb2ad. --- .../test/acceptance/control_flow_for_spec.ts | 31 ------------------- .../test/acceptance/control_flow_if_spec.ts | 30 ------------------ 2 files changed, 61 deletions(-) diff --git a/packages/core/test/acceptance/control_flow_for_spec.ts b/packages/core/test/acceptance/control_flow_for_spec.ts index cc03b21c2489..c993d2f4366e 100644 --- a/packages/core/test/acceptance/control_flow_for_spec.ts +++ b/packages/core/test/acceptance/control_flow_for_spec.ts @@ -417,37 +417,6 @@ describe('control flow - for', () => { expect(fixture.nativeElement.textContent).toBe('Main: Before one1two1one2two2 After Slot: '); }); - it('should project an @for with an ng-container root node', () => { - @Component({ - standalone: true, - selector: 'test', - template: 'Main: Slot: ', - }) - class TestComponent { - } - - @Component({ - standalone: true, - imports: [TestComponent], - template: ` - Before @for (item of items; track $index) { - - {{item}} - | - - } After - ` - }) - class App { - items = [1, 2, 3]; - } - - const fixture = TestBed.createComponent(App); - fixture.detectChanges(); - - expect(fixture.nativeElement.textContent).toBe('Main: Before After Slot: 1|2|3|'); - }); - // Right now the template compiler doesn't collect comment nodes. // This test is to ensure that we don't regress if it happens in the future. it('should project an @for with single root node and comments into the root node slot', () => { diff --git a/packages/core/test/acceptance/control_flow_if_spec.ts b/packages/core/test/acceptance/control_flow_if_spec.ts index 969a03807ffb..3f63f6d9472e 100644 --- a/packages/core/test/acceptance/control_flow_if_spec.ts +++ b/packages/core/test/acceptance/control_flow_if_spec.ts @@ -317,36 +317,6 @@ describe('control flow - if', () => { expect(fixture.nativeElement.textContent).toBe('Main: Before onetwo After Slot: '); }); - it('should project an @if with an ng-container root node', () => { - @Component({ - standalone: true, - selector: 'test', - template: 'Main: Slot: ', - }) - class TestComponent { - } - - @Component({ - standalone: true, - imports: [TestComponent], - template: ` - Before @if (true) { - - foo - bar - - } After - ` - }) - class App { - } - - const fixture = TestBed.createComponent(App); - fixture.detectChanges(); - - expect(fixture.nativeElement.textContent).toBe('Main: Before After Slot: foobar'); - }); - // Right now the template compiler doesn't collect comment nodes. // This test is to ensure that we don't regress if it happens in the future. it('should project an @if with a single root node and comments into the root node slot', () => { From 66d1522302a32963f712e0e4c8a216b8c43bd412 Mon Sep 17 00:00:00 2001 From: Kristiyan Kostadinov Date: Fri, 17 Nov 2023 20:34:40 +0100 Subject: [PATCH 4/4] Revert "fix(compiler-cli): add diagnostic for control flow that prevents content projection (#52726)" This reverts commit b4d022e230ca141b12437949d11dc384bfe5c082. --- goldens/public-api/compiler-cli/error_code.md | 1 - .../src/ngtsc/diagnostics/src/error_code.ts | 15 - .../src/ngtsc/typecheck/src/oob.ts | 38 +-- .../ngtsc/typecheck/src/type_check_block.ts | 113 +------ .../src/ngtsc/typecheck/testing/index.ts | 1 - .../test/ngtsc/template_typecheck_spec.ts | 299 ------------------ 6 files changed, 2 insertions(+), 465 deletions(-) diff --git a/goldens/public-api/compiler-cli/error_code.md b/goldens/public-api/compiler-cli/error_code.md index 24007655bce9..ca3dd2a1c325 100644 --- a/goldens/public-api/compiler-cli/error_code.md +++ b/goldens/public-api/compiler-cli/error_code.md @@ -25,7 +25,6 @@ export enum ErrorCode { // (undocumented) CONFIG_STRICT_TEMPLATES_IMPLIES_FULL_TEMPLATE_TYPECHECK = 4002, CONFLICTING_INPUT_TRANSFORM = 2020, - CONTROL_FLOW_PREVENTING_CONTENT_PROJECTION = 8011, // (undocumented) DECORATOR_ARG_NOT_LITERAL = 1001, // (undocumented) diff --git a/packages/compiler-cli/src/ngtsc/diagnostics/src/error_code.ts b/packages/compiler-cli/src/ngtsc/diagnostics/src/error_code.ts index f5ec27d15ba6..3f523a09eb08 100644 --- a/packages/compiler-cli/src/ngtsc/diagnostics/src/error_code.ts +++ b/packages/compiler-cli/src/ngtsc/diagnostics/src/error_code.ts @@ -286,21 +286,6 @@ export enum ErrorCode { */ INACCESSIBLE_DEFERRED_TRIGGER_ELEMENT = 8010, - /** - * A control flow node is projected at the root of a component and is preventing its direct - * descendants from being projected, because it has more than one root node. - * - * ``` - * - * @if (expr) { - *
- * Text preventing the div from being projected - * } - * - * ``` - */ - CONTROL_FLOW_PREVENTING_CONTENT_PROJECTION = 8011, - /** * A two way binding in a template has an incorrect syntax, * parentheses outside brackets. For example: diff --git a/packages/compiler-cli/src/ngtsc/typecheck/src/oob.ts b/packages/compiler-cli/src/ngtsc/typecheck/src/oob.ts index 8adee446fb9b..7bbe04cceafe 100644 --- a/packages/compiler-cli/src/ngtsc/typecheck/src/oob.ts +++ b/packages/compiler-cli/src/ngtsc/typecheck/src/oob.ts @@ -6,7 +6,7 @@ * found in the LICENSE file at https://angular.io/license */ -import {BindingPipe, PropertyRead, PropertyWrite, TmplAstBoundAttribute, TmplAstBoundEvent, TmplAstElement, TmplAstForLoopBlock, TmplAstHoverDeferredTrigger, TmplAstIfBlockBranch, TmplAstInteractionDeferredTrigger, TmplAstReference, TmplAstTemplate, TmplAstVariable, TmplAstViewportDeferredTrigger} from '@angular/compiler'; +import {BindingPipe, PropertyRead, PropertyWrite, TmplAstBoundAttribute, TmplAstBoundEvent, TmplAstElement, TmplAstForLoopBlock, TmplAstHoverDeferredTrigger, TmplAstInteractionDeferredTrigger, TmplAstReference, TmplAstTemplate, TmplAstVariable, TmplAstViewportDeferredTrigger} from '@angular/compiler'; import ts from 'typescript'; import {ErrorCode, makeDiagnostic, makeRelatedInformation, ngErrorCode} from '../../diagnostics'; @@ -100,14 +100,6 @@ export interface OutOfBandDiagnosticRecorder { templateId: TemplateId, trigger: TmplAstHoverDeferredTrigger|TmplAstInteractionDeferredTrigger| TmplAstViewportDeferredTrigger): void; - - /** - * Reports cases where control flow nodes prevent content projection. - */ - controlFlowPreventingContentProjection( - templateId: TemplateId, projectionNode: TmplAstElement|TmplAstTemplate, componentName: string, - slotSelector: string, controlFlowNode: TmplAstIfBlockBranch|TmplAstForLoopBlock, - preservesWhitespaces: boolean): void; } export class OutOfBandDiagnosticRecorderImpl implements OutOfBandDiagnosticRecorder { @@ -348,34 +340,6 @@ export class OutOfBandDiagnosticRecorderImpl implements OutOfBandDiagnosticRecor ts.DiagnosticCategory.Error, ngErrorCode(ErrorCode.INACCESSIBLE_DEFERRED_TRIGGER_ELEMENT), message)); } - - controlFlowPreventingContentProjection( - templateId: TemplateId, projectionNode: TmplAstElement|TmplAstTemplate, componentName: string, - slotSelector: string, controlFlowNode: TmplAstIfBlockBranch|TmplAstForLoopBlock, - preservesWhitespaces: boolean): void { - const blockName = controlFlowNode instanceof TmplAstIfBlockBranch ? '@if' : '@for'; - const lines = [ - `Node matches the "${slotSelector}" slot of the "${ - componentName}" component, but will not be projected into the specific slot because the surrounding ${ - blockName} has more than one node at its root. To project the node in the right slot, you can:\n`, - `1. Wrap the content of the ${blockName} block in an that matches the "${ - slotSelector}" selector.`, - `2. Split the content of the ${blockName} block across multiple ${ - blockName} blocks such that each one only has a single projectable node at its root.`, - `3. Remove all content from the ${blockName} block, except for the node being projected.` - ]; - - if (preservesWhitespaces) { - lines.push( - `Note: the host component has \`preserveWhitespaces: true\` which may ` + - `cause whitespace to affect content projection.`); - } - - this._diagnostics.push(makeTemplateDiagnostic( - templateId, this.resolver.getSourceMapping(templateId), projectionNode.startSourceSpan, - ts.DiagnosticCategory.Warning, - ngErrorCode(ErrorCode.CONTROL_FLOW_PREVENTING_CONTENT_PROJECTION), lines.join('\n'))); - } } function makeInlineDiagnostic( 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 c56e0bebece8..dda37e07da0f 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, BindingPipe, BindingType, BoundTarget, Call, createCssSelectorFromNode, CssSelector, DYNAMIC_TYPE, ImplicitReceiver, ParsedEventType, ParseSourceSpan, PropertyRead, PropertyWrite, SafeCall, SafePropertyRead, SchemaMetadata, SelectorMatcher, ThisReceiver, TmplAstBoundAttribute, TmplAstBoundEvent, TmplAstBoundText, TmplAstDeferredBlock, TmplAstDeferredBlockTriggers, TmplAstElement, TmplAstForLoopBlock, TmplAstHoverDeferredTrigger, TmplAstIcu, TmplAstIfBlock, TmplAstIfBlockBranch, TmplAstInteractionDeferredTrigger, TmplAstNode, TmplAstReference, TmplAstSwitchBlock, TmplAstSwitchBlockCase, TmplAstTemplate, TmplAstText, TmplAstTextAttribute, TmplAstVariable, TmplAstViewportDeferredTrigger, TransplantedType} from '@angular/compiler'; +import {AST, BindingPipe, BindingType, BoundTarget, Call, DYNAMIC_TYPE, ImplicitReceiver, ParsedEventType, ParseSourceSpan, PropertyRead, PropertyWrite, SafeCall, SafePropertyRead, SchemaMetadata, ThisReceiver, TmplAstBoundAttribute, TmplAstBoundEvent, TmplAstBoundText, TmplAstDeferredBlock, TmplAstDeferredBlockTriggers, TmplAstElement, TmplAstForLoopBlock, TmplAstHoverDeferredTrigger, TmplAstIcu, TmplAstIfBlock, TmplAstIfBlockBranch, TmplAstInteractionDeferredTrigger, TmplAstNode, TmplAstReference, TmplAstSwitchBlock, TmplAstSwitchBlockCase, TmplAstTemplate, TmplAstTextAttribute, TmplAstVariable, TmplAstViewportDeferredTrigger, TransplantedType} from '@angular/compiler'; import ts from 'typescript'; import {Reference} from '../../imports'; @@ -905,100 +905,6 @@ class TcbDomSchemaCheckerOp extends TcbOp { } -/** - * A `TcbOp` that finds and flags control flow nodes that interfere with content projection. - * - * Context: - * `@if` and `@for` try to emulate the content projection behavior of `*ngIf` and `*ngFor` - * in order to reduce breakages when moving from one syntax to the other (see #52414), however the - * approach only works if there's only one element at the root of the control flow expression. - * This means that a stray sibling node (e.g. text) can prevent an element from being projected - * into the right slot. The purpose of the `TcbOp` is to find any places where a node at the root - * of a control flow expression *would have been projected* into a specific slot, if the control - * flow node didn't exist. - */ -class TcbControlFlowContentProjectionOp extends TcbOp { - constructor( - private tcb: Context, private element: TmplAstElement, private ngContentSelectors: string[], - private componentName: string) { - super(); - } - - override readonly optional = false; - - override execute(): null { - const controlFlowToCheck = this.findPotentialControlFlowNodes(); - - if (controlFlowToCheck.length > 0) { - const matcher = new SelectorMatcher(); - - for (const selector of this.ngContentSelectors) { - // `*` is a special selector for the catch-all slot. - if (selector !== '*') { - matcher.addSelectables(CssSelector.parse(selector), selector); - } - } - - for (const root of controlFlowToCheck) { - for (const child of root.children) { - if (child instanceof TmplAstElement || child instanceof TmplAstTemplate) { - matcher.match(createCssSelectorFromNode(child), (_, originalSelector) => { - this.tcb.oobRecorder.controlFlowPreventingContentProjection( - this.tcb.id, child, this.componentName, originalSelector, root, - this.tcb.hostPreserveWhitespaces); - }); - } - } - } - } - - return null; - } - - private findPotentialControlFlowNodes() { - const result: Array = []; - - for (const child of this.element.children) { - let eligibleNode: TmplAstForLoopBlock|TmplAstIfBlockBranch|null = null; - - // Only `@for` blocks and the first branch of `@if` blocks participate in content projection. - if (child instanceof TmplAstForLoopBlock) { - eligibleNode = child; - } else if (child instanceof TmplAstIfBlock) { - eligibleNode = child.branches[0]; // @if blocks are guaranteed to have at least one branch. - } - - // Skip nodes with less than two children since it's impossible - // for them to run into the issue that we're checking for. - if (eligibleNode === null || eligibleNode.children.length < 2) { - continue; - } - - // Count the number of root nodes while skipping empty text where relevant. - const rootNodeCount = eligibleNode.children.reduce((count, node) => { - // Normally `preserveWhitspaces` would have been accounted for during parsing, however - // in `ngtsc/annotations/component/src/resources.ts#parseExtractedTemplate` we enable - // `preserveWhitespaces` to preserve the accuracy of source maps diagnostics. This means - // that we have to account for it here since the presence of text nodes affects the - // content projection behavior. - if (!(node instanceof TmplAstText) || this.tcb.hostPreserveWhitespaces || - node.value.trim().length > 0) { - count++; - } - - return count; - }, 0); - - // Content projection can only be affected if there is more than one root node. - if (rootNodeCount > 1) { - result.push(eligibleNode); - } - } - - return result; - } -} - /** * Mapping between attributes names that don't correspond to their element property names. * Note: this mapping has to be kept in sync with the equally named mapping in the runtime. @@ -1931,7 +1837,6 @@ class Scope { if (node instanceof TmplAstElement) { const opIndex = this.opQueue.push(new TcbElementOp(this.tcb, this, node)) - 1; this.elementOpMap.set(node, opIndex); - this.appendContentProjectionCheckOp(node); this.appendDirectivesAndInputsOfNode(node); this.appendOutputsOfNode(node); this.appendChildren(node); @@ -2128,22 +2033,6 @@ class Scope { } } - private appendContentProjectionCheckOp(root: TmplAstElement): void { - const meta = - this.tcb.boundTarget.getDirectivesOfNode(root)?.find(meta => meta.isComponent) || null; - - if (meta !== null && meta.ngContentSelectors !== null && meta.ngContentSelectors.length > 0) { - const selectors = meta.ngContentSelectors; - - // We don't need to generate anything for components that don't have projection - // slots, or they only have one catch-all slot (represented by `*`). - if (selectors.length > 1 || (selectors.length === 1 && selectors[0] !== '*')) { - this.opQueue.push( - new TcbControlFlowContentProjectionOp(this.tcb, root, selectors, meta.name)); - } - } - } - private appendDeferredBlock(block: TmplAstDeferredBlock): void { this.appendDeferredTriggers(block, block.triggers); this.appendDeferredTriggers(block, block.prefetchTriggers); diff --git a/packages/compiler-cli/src/ngtsc/typecheck/testing/index.ts b/packages/compiler-cli/src/ngtsc/typecheck/testing/index.ts index 46ba48449631..51bf3d900a35 100644 --- a/packages/compiler-cli/src/ngtsc/typecheck/testing/index.ts +++ b/packages/compiler-cli/src/ngtsc/typecheck/testing/index.ts @@ -800,5 +800,4 @@ export class NoopOobRecorder implements OutOfBandDiagnosticRecorder { missingRequiredInputs(): void {} illegalForLoopTrackAccess(): void {} inaccessibleDeferredTriggerElement(): void {} - controlFlowPreventingContentProjection(): void {} } diff --git a/packages/compiler-cli/test/ngtsc/template_typecheck_spec.ts b/packages/compiler-cli/test/ngtsc/template_typecheck_spec.ts index 799f8921992d..e3a9291589be 100644 --- a/packages/compiler-cli/test/ngtsc/template_typecheck_spec.ts +++ b/packages/compiler-cli/test/ngtsc/template_typecheck_spec.ts @@ -4923,304 +4923,5 @@ suppress ]); }); }); - - describe('control flow content projection diagnostics', () => { - it('should report when an @if block prevents an element from being projected', () => { - env.write('test.ts', ` - import {Component} from '@angular/core'; - - @Component({ - selector: 'comp', - template: ' ', - standalone: true, - }) - class Comp {} - - @Component({ - standalone: true, - imports: [Comp], - template: \` - - @if (true) { -
- breaks projection - } - - \`, - }) - class TestCmp {} - `); - - const diags = - env.driveDiagnostics().map(d => ts.flattenDiagnosticMessageText(d.messageText, '')); - expect(diags.length).toBe(1); - expect(diags[0]).toContain( - `Node matches the "bar, [foo]" slot of the "Comp" component, but will ` + - `not be projected into the specific slot because the surrounding @if has more than one node at its root.`); - }); - - it('should report when an @if block prevents a template from being projected', () => { - env.write('test.ts', ` - import {Component} from '@angular/core'; - - @Component({ - selector: 'comp', - template: ' ', - standalone: true, - }) - class Comp {} - - @Component({ - standalone: true, - imports: [Comp], - template: \` - - @if (true) { - - breaks projection - } - - \`, - }) - class TestCmp {} - `); - - const diags = - env.driveDiagnostics().map(d => ts.flattenDiagnosticMessageText(d.messageText, '')); - expect(diags.length).toBe(1); - expect(diags[0]).toContain( - `Node matches the "bar, [foo]" slot of the "Comp" component, but will ` + - `not be projected into the specific slot because the surrounding @if has more than one node at its root.`); - }); - - it('should report when an @for block prevents content from being projected', () => { - env.write('test.ts', ` - import {Component} from '@angular/core'; - - @Component({ - selector: 'comp', - template: ' ', - standalone: true, - }) - class Comp {} - - @Component({ - standalone: true, - imports: [Comp], - template: \` - - @for (i of [1, 2, 3]; track i) { -
- breaks projection - } - - \`, - }) - class TestCmp {} - `); - - const diags = - env.driveDiagnostics().map(d => ts.flattenDiagnosticMessageText(d.messageText, '')); - expect(diags.length).toBe(1); - expect(diags[0]).toContain( - `Node matches the "bar, [foo]" slot of the "Comp" component, but will ` + - `not be projected into the specific slot because the surrounding @for has more than one node at its root.`); - }); - - it('should report nodes that are targeting different slots but cannot be projected', () => { - env.write('test.ts', ` - import {Component} from '@angular/core'; - - @Component({ - selector: 'comp', - template: ' ', - standalone: true, - }) - class Comp {} - - @Component({ - standalone: true, - imports: [Comp], - template: \` - - @if (true) { -
-
- } - - \`, - }) - class TestCmp {} - `); - - const diags = - env.driveDiagnostics().map(d => ts.flattenDiagnosticMessageText(d.messageText, '')); - expect(diags.length).toBe(2); - expect(diags[0]).toContain( - `Node matches the "[foo]" slot of the "Comp" component, but will not be projected`); - expect(diags[1]).toContain( - `Node matches the "[bar]" slot of the "Comp" component, but will not be projected`); - }); - - it('should report nodes that are targeting the same slot but cannot be projected', () => { - env.write('test.ts', ` - import {Component} from '@angular/core'; - - @Component({ - selector: 'comp', - template: '', - standalone: true, - }) - class Comp {} - - @Component({ - standalone: true, - imports: [Comp], - template: \` - - @if (true) { -
- - } - - \`, - }) - class TestCmp {} - `); - - const diags = - env.driveDiagnostics().map(d => ts.flattenDiagnosticMessageText(d.messageText, '')); - expect(diags.length).toBe(2); - expect(diags[0]).toContain( - `Node matches the "[foo]" slot of the "Comp" component, but will not be projected`); - expect(diags[1]).toContain( - `Node matches the "[foo]" slot of the "Comp" component, but will not be projected`); - }); - - it('should report when preserveWhitespaces may affect content projection', () => { - env.write('test.ts', ` - import {Component} from '@angular/core'; - - @Component({ - selector: 'comp', - template: '', - standalone: true, - }) - class Comp {} - - @Component({ - standalone: true, - imports: [Comp], - preserveWhitespaces: true, - template: \` - - @if (true) { -
- } - - \`, - }) - class TestCmp {} - `); - - const diags = - env.driveDiagnostics().map(d => ts.flattenDiagnosticMessageText(d.messageText, '')); - expect(diags.length).toBe(1); - expect(diags[0]).toContain(`Node matches the "[foo]" slot of the "Comp" component`); - expect(diags[0]).toContain( - `Note: the host component has \`preserveWhitespaces: true\` which may cause ` + - `whitespace to affect content projection.`); - }); - - it('should not report when there is only one root node', () => { - env.write('test.ts', ` - import {Component} from '@angular/core'; - - @Component({ - selector: 'comp', - template: '', - standalone: true, - }) - class Comp {} - - @Component({ - standalone: true, - imports: [Comp], - template: \` - - @if (true) { -
- } - - \`, - }) - class TestCmp {} - `); - - const diags = env.driveDiagnostics(); - expect(diags.length).toBe(0); - }); - - it('should not report when there are comments at the root of the control flow node', () => { - env.write('test.ts', ` - import {Component} from '@angular/core'; - - @Component({ - selector: 'comp', - template: '', - standalone: true, - }) - class Comp {} - - @Component({ - standalone: true, - imports: [Comp], - template: \` - - @if (true) { - -
- - } - - \`, - }) - class TestCmp {} - `); - - const diags = env.driveDiagnostics(); - expect(diags.length).toBe(0); - }); - - it('should not report when the component only has a catch-all slot', () => { - env.write('test.ts', ` - import {Component} from '@angular/core'; - - @Component({ - selector: 'comp', - template: '', - standalone: true, - }) - class Comp {} - - @Component({ - standalone: true, - imports: [Comp], - template: \` - - @if (true) { -
- breaks projection - } - - \`, - }) - class TestCmp {} - `); - - const diags = env.driveDiagnostics(); - expect(diags.length).toBe(0); - }); - }); }); });