From d4de3d1e4ded7486c79d4f0771bc958266d06a13 Mon Sep 17 00:00:00 2001 From: Kristiyan Kostadinov Date: Wed, 8 Nov 2023 10:31:14 +0100 Subject: [PATCH 1/4] refactor(compiler): expose utility for creating CSS selectors from AST nodes When doing directive matching in the compiler, we need to be able to create a selector from an AST node. We already have the utility, but these changes simplify the public API and expose it so it can be used in `compiler-cli`. --- 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, 34 insertions(+), 32 deletions(-) diff --git a/packages/compiler/src/compiler.ts b/packages/compiler/src/compiler.ts index 7b6c8508fe0d..215edcfec020 100644 --- a/packages/compiler/src/compiler.ts +++ b/packages/compiler/src/compiler.ts @@ -66,6 +66,7 @@ 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 1355b49bcb18..2ef5e5fe5745 100644 --- a/packages/compiler/src/render3/view/t2_binder.ts +++ b/packages/compiler/src/render3/view/t2_binder.ts @@ -11,8 +11,7 @@ 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 {createCssSelector} from './template'; -import {getAttrsForDirectiveMatching} from './util'; +import {createCssSelectorFromNode} from './util'; /** * Processes `Target`s with a given set of directives and performs a binding operation, which @@ -307,17 +306,17 @@ class DirectiveBinder implements Visitor { } visitElement(element: Element): void { - this.visitElementOrTemplate(element.name, element); + this.visitElementOrTemplate(element); } visitTemplate(template: Template): void { - this.visitElementOrTemplate('ng-template', template); + this.visitElementOrTemplate(template); } - visitElementOrTemplate(elementName: string, node: Element|Template): void { + visitElementOrTemplate(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 = createCssSelector(elementName, getAttrsForDirectiveMatching(node)); + const cssSelector = createCssSelectorFromNode(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 deb1a818c432..1a2093ae421b 100644 --- a/packages/compiler/src/render3/view/template.ts +++ b/packages/compiler/src/render3/view/template.ts @@ -2581,30 +2581,6 @@ 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 386af5048c90..d2288ad2b7be 100644 --- a/packages/compiler/src/render3/view/util.ts +++ b/packages/compiler/src/render3/view/util.ts @@ -8,8 +8,10 @@ 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'; @@ -277,6 +279,31 @@ 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. @@ -286,8 +313,7 @@ export class DefinitionMap { * object maps a property name to its (static) value. For any bindings, this map simply maps the * property name to an empty string. */ -export function getAttrsForDirectiveMatching(elOrTpl: t.Element| - t.Template): {[name: string]: string} { +function getAttrsForDirectiveMatching(elOrTpl: t.Element|t.Template): {[name: string]: string} { const attributesMap: {[name: string]: string} = {}; From a2169c31d71d4f65dfeacfec03d7f558fef787b7 Mon Sep 17 00:00:00 2001 From: Kristiyan Kostadinov Date: Wed, 8 Nov 2023 10:57:58 +0100 Subject: [PATCH 2/4] refactor(compiler-cli): expose ng-content selectors and preserveWhitespaces during template type checking These changes expose the `ngContentSelectors` and `preserveWhitespaces` metadata to the TCB so they can be used in the next commit to implement a new diagnostic. --- .../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, 76 insertions(+), 12 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 49592bccb109..402e7ffe1c0d 100644 --- a/packages/compiler-cli/src/ngtsc/annotations/component/src/handler.ts +++ b/packages/compiler-cli/src/ngtsc/annotations/component/src/handler.ts @@ -463,10 +463,7 @@ export class ComponentDecoratorHandler implements rawHostDirectives, meta: { ...metadata, - template: { - nodes: template.nodes, - ngContentSelectors: template.ngContentSelectors, - }, + template, encapsulation, changeDetection, interpolation: template.interpolationConfig ?? DEFAULT_INTERPOLATION_CONFIG, @@ -546,6 +543,8 @@ 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); @@ -614,7 +613,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.isStandalone, meta.meta.template.preserveWhitespaces ?? false); } 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 0c7310711fbd..b17630770cbf 100644 --- a/packages/compiler-cli/src/ngtsc/annotations/directive/src/handler.ts +++ b/packages/compiler-cli/src/ngtsc/annotations/directive/src/handler.ts @@ -169,7 +169,9 @@ 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 6b2f2308fdd0..b0cede22afc2 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,6 +110,8 @@ 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 e2d287758de3..2907c6291e71 100644 --- a/packages/compiler-cli/src/ngtsc/indexer/test/util.ts +++ b/packages/compiler-cli/src/ngtsc/indexer/test/util.ts @@ -56,6 +56,8 @@ 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 eb91b3bb2b84..3f3d4ce29ce6 100644 --- a/packages/compiler-cli/src/ngtsc/metadata/src/dts.ts +++ b/packages/compiler-cli/src/ngtsc/metadata/src/dts.ts @@ -96,6 +96,9 @@ 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); @@ -126,6 +129,7 @@ export class DtsMetadataReader implements MetadataReader { isPoisoned: false, isStructural, animationTriggerNames: null, + ngContentSelectors, isStandalone, isSignal, // Imports are tracked in metadata only for template type-checking purposes, @@ -136,6 +140,9 @@ 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 14f54199b34d..64aa131addb1 100644 --- a/packages/compiler-cli/src/ngtsc/scope/test/local_spec.ts +++ b/packages/compiler-cli/src/ngtsc/scope/test/local_spec.ts @@ -330,6 +330,8 @@ 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 5bdc53ef0409..11e82931d560 100644 --- a/packages/compiler-cli/src/ngtsc/typecheck/api/api.ts +++ b/packages/compiler-cli/src/ngtsc/typecheck/api/api.ts @@ -84,6 +84,11 @@ 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 747477a0dc04..7967fb007dc1 100644 --- a/packages/compiler-cli/src/ngtsc/typecheck/api/context.ts +++ b/packages/compiler-cli/src/ngtsc/typecheck/api/context.ts @@ -37,13 +37,15 @@ 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): void; + parseErrors: ParseError[]|null, isStandalone: boolean, preserveWhitespaces: 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 075e7d28c03e..c9fdd2e2fb93 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): void { + parseErrors: ParseError[]|null, isStandalone: boolean, preserveWhitespaces: boolean): void { if (!this.host.shouldCheckComponent(ref.node)) { return; } @@ -293,7 +293,8 @@ export class TypeCheckContextImpl implements TypeCheckContext { boundTarget, pipes, schemas, - isStandalone + isStandalone, + preserveWhitespaces, }; 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 dda37e07da0f..d00b33b243ca 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.isStandalone, meta.preserveWhitespaces); const scope = Scope.forNodes(tcb, null, null, tcb.boundTarget.target.template!, /* guard */ null); const ctxRawType = env.referenceType(ref); if (!ts.isTypeReferenceNode(ctxRawType)) { @@ -1488,7 +1488,8 @@ export class Context { readonly oobRecorder: OutOfBandDiagnosticRecorder, readonly id: TemplateId, readonly boundTarget: BoundTarget, private pipes: Map>>, - readonly schemas: SchemaMetadata[], readonly hostIsStandalone: boolean) {} + readonly schemas: SchemaMetadata[], readonly hostIsStandalone: boolean, + readonly hostPreserveWhitespaces: 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 51bf3d900a35..10526facbf10 100644 --- a/packages/compiler-cli/src/ngtsc/typecheck/testing/index.ts +++ b/packages/compiler-cli/src/ngtsc/typecheck/testing/index.ts @@ -249,6 +249,8 @@ export interface TestDirective extends Partial { return { directive: new Reference(resolveDeclaration(hostDecl.directive)), @@ -732,6 +737,8 @@ 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 103eb707f706..f2ca0d78ed0a 100644 --- a/packages/compiler/src/render3/view/api.ts +++ b/packages/compiler/src/render3/view/api.ts @@ -226,6 +226,11 @@ 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 f4e9662948d0..689d28cc381c 100644 --- a/packages/compiler/src/render3/view/t2_api.ts +++ b/packages/compiler/src/render3/view/t2_api.ts @@ -93,8 +93,21 @@ 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 eb682c3fd2b6..46c6a09258b4 100644 --- a/packages/compiler/test/render3/view/binding_spec.ts +++ b/packages/compiler/test/render3/view/binding_spec.ts @@ -41,6 +41,8 @@ function makeSelectorMatcher(): SelectorMatcher { isStructural: true, selector: '[ngFor][ngForOf]', animationTriggerNames: null, + ngContentSelectors: null, + preserveWhitespaces: false, }]); matcher.addSelectables(CssSelector.parse('[dir]'), [{ name: 'Dir', @@ -51,6 +53,8 @@ function makeSelectorMatcher(): SelectorMatcher { isStructural: false, selector: '[dir]', animationTriggerNames: null, + ngContentSelectors: null, + preserveWhitespaces: false, }]); matcher.addSelectables(CssSelector.parse('[hasOutput]'), [{ name: 'HasOutput', @@ -61,6 +65,8 @@ function makeSelectorMatcher(): SelectorMatcher { isStructural: false, selector: '[hasOutput]', animationTriggerNames: null, + ngContentSelectors: null, + preserveWhitespaces: false, }]); matcher.addSelectables(CssSelector.parse('[hasInput]'), [{ name: 'HasInput', @@ -71,6 +77,8 @@ function makeSelectorMatcher(): SelectorMatcher { isStructural: false, selector: '[hasInput]', animationTriggerNames: null, + ngContentSelectors: null, + preserveWhitespaces: false, }]); matcher.addSelectables(CssSelector.parse('[sameSelectorAsInput]'), [{ name: 'SameSelectorAsInput', @@ -81,6 +89,8 @@ function makeSelectorMatcher(): SelectorMatcher { isStructural: false, selector: '[sameSelectorAsInput]', animationTriggerNames: null, + ngContentSelectors: null, + preserveWhitespaces: false, }]); matcher.addSelectables(CssSelector.parse('comp'), [{ name: 'Comp', @@ -91,6 +101,8 @@ function makeSelectorMatcher(): SelectorMatcher { isStructural: false, selector: 'comp', animationTriggerNames: null, + ngContentSelectors: null, + preserveWhitespaces: false, }]); const simpleDirectives = ['a', 'b', 'c', 'd', 'e', 'f']; @@ -106,6 +118,8 @@ function makeSelectorMatcher(): SelectorMatcher { isStructural: true, selector: `[${dir}]`, animationTriggerNames: null, + ngContentSelectors: null, + preserveWhitespaces: false, }]); } @@ -155,6 +169,8 @@ 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 e6ddfb9b68781b81f86e3cc2be1e3615413582fc Mon Sep 17 00:00:00 2001 From: Kristiyan Kostadinov Date: Wed, 8 Nov 2023 12:28:23 +0100 Subject: [PATCH 3/4] test(core): add tests for control flow content projection with ng-container The control flow projection diagnostic will mention `ng-container` as a workaround for projection multiple nodes. These changes add a couple of tests to ensure that the approach works. --- .../test/acceptance/control_flow_for_spec.ts | 31 +++++++++++++++++++ .../test/acceptance/control_flow_if_spec.ts | 30 ++++++++++++++++++ 2 files changed, 61 insertions(+) diff --git a/packages/core/test/acceptance/control_flow_for_spec.ts b/packages/core/test/acceptance/control_flow_for_spec.ts index c993d2f4366e..cc03b21c2489 100644 --- a/packages/core/test/acceptance/control_flow_for_spec.ts +++ b/packages/core/test/acceptance/control_flow_for_spec.ts @@ -417,6 +417,37 @@ 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 3f63f6d9472e..969a03807ffb 100644 --- a/packages/core/test/acceptance/control_flow_if_spec.ts +++ b/packages/core/test/acceptance/control_flow_if_spec.ts @@ -317,6 +317,36 @@ 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 22334930d216ad46daf7e30e920587aedff2656b Mon Sep 17 00:00:00 2001 From: Kristiyan Kostadinov Date: Mon, 27 Nov 2023 10:29:20 +0100 Subject: [PATCH 4/4] fix(compiler-cli): add diagnostic for control flow that prevents content projection This is a follow-up to the fix from #52414. It adds a diagnostic that will tell users when a control flow is preventing its direct descendants from being projected into a specific component slot. --- 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, 465 insertions(+), 2 deletions(-) diff --git a/goldens/public-api/compiler-cli/error_code.md b/goldens/public-api/compiler-cli/error_code.md index ca3dd2a1c325..24007655bce9 100644 --- a/goldens/public-api/compiler-cli/error_code.md +++ b/goldens/public-api/compiler-cli/error_code.md @@ -25,6 +25,7 @@ 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 3f523a09eb08..f5ec27d15ba6 100644 --- a/packages/compiler-cli/src/ngtsc/diagnostics/src/error_code.ts +++ b/packages/compiler-cli/src/ngtsc/diagnostics/src/error_code.ts @@ -286,6 +286,21 @@ 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 7bbe04cceafe..8adee446fb9b 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, TmplAstInteractionDeferredTrigger, TmplAstReference, TmplAstTemplate, TmplAstVariable, TmplAstViewportDeferredTrigger} from '@angular/compiler'; +import {BindingPipe, PropertyRead, PropertyWrite, TmplAstBoundAttribute, TmplAstBoundEvent, TmplAstElement, TmplAstForLoopBlock, TmplAstHoverDeferredTrigger, TmplAstIfBlockBranch, TmplAstInteractionDeferredTrigger, TmplAstReference, TmplAstTemplate, TmplAstVariable, TmplAstViewportDeferredTrigger} from '@angular/compiler'; import ts from 'typescript'; import {ErrorCode, makeDiagnostic, makeRelatedInformation, ngErrorCode} from '../../diagnostics'; @@ -100,6 +100,14 @@ 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 { @@ -340,6 +348,34 @@ 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 d00b33b243ca..531f5a6c3b73 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, 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 {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 ts from 'typescript'; import {Reference} from '../../imports'; @@ -905,6 +905,100 @@ 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. @@ -1838,6 +1932,7 @@ 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); @@ -2034,6 +2129,22 @@ 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 10526facbf10..f72cde49c7ed 100644 --- a/packages/compiler-cli/src/ngtsc/typecheck/testing/index.ts +++ b/packages/compiler-cli/src/ngtsc/typecheck/testing/index.ts @@ -807,4 +807,5 @@ 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 8aaae84a86fe..aa4383013882 100644 --- a/packages/compiler-cli/test/ngtsc/template_typecheck_spec.ts +++ b/packages/compiler-cli/test/ngtsc/template_typecheck_spec.ts @@ -5020,5 +5020,304 @@ 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); + }); + }); }); });