diff --git a/goldens/public-api/compiler-cli/error_code.md b/goldens/public-api/compiler-cli/error_code.md index ca3dd2a1c3257c..24007655bce9c7 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 3f523a09eb0876..f5ec27d15ba633 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 7bbe04cceafeb6..5b47a37fca5837 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 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 into 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.Error, + 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 78d5bca65222df..3b8bd285285767 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,105 @@ 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: (TmplAstIfBlockBranch|TmplAstForLoopBlock)[] = []; + + 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. + // Skip nodes with less than two children since it's impossible for them to run into the + // issue that we're checking for. + if (child instanceof TmplAstForLoopBlock && child.children.length > 1) { + eligibleNode = child; + } else if (child instanceof TmplAstIfBlock && child.branches[0].children.length > 1) { + eligibleNode = child.branches[0]; + } + + if (eligibleNode === null) { + continue; + } + + // Count the number of root nodes while skipping empty text where relevant. + const rootNodeCount = eligibleNode.children.reduce((count, node) => { + if (node instanceof TmplAstText) { + // 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 (this.tcb.hostPreserveWhitespaces) { + count++; + } else { + count += node.value.trim().length > 0 ? 1 : 0; + } + } else { + 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. @@ -1826,6 +1925,7 @@ class Scope { this.appendDirectivesAndInputsOfNode(node); this.appendOutputsOfNode(node); this.appendChildren(node); + this.appendContentProjectionCheckOp(node); this.checkAndAppendReferencesOfNode(node); } else if (node instanceof TmplAstTemplate) { // Template children are rendered in a child scope. @@ -2019,6 +2119,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 10526facbf1056..f72cde49c7edb6 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 00b576b753c2ef..79e5cdd2338423 100644 --- a/packages/compiler-cli/test/ngtsc/template_typecheck_spec.ts +++ b/packages/compiler-cli/test/ngtsc/template_typecheck_spec.ts @@ -4923,5 +4923,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 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 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 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); + }); + }); }); });