Skip to content

refactor(compiler): disallow unsupported bindings and attributes on ng-content and ng-template #71887

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
JeanMeche wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

refactor(compiler): disallow unsupported bindings and attributes on ng-content and ng-template #71887

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 15 additions & 0 deletions packages/compiler-cli/preprocessor/src/oob.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -511,6 +511,21 @@ export class OutOfBandDiagnosticRecorderImpl implements OutOfBandDiagnosticRecor
);
}

unsupportedNgContentAttribute(id: TypeCheckId, attr: TmplAstTextAttribute): void {
const lower = attr.name.toLowerCase();
const message =
lower === 'selection' || lower === 'selector'
? `'${attr.name}' is not supported on ng-content. Did you mean 'select'?`
: `'${attr.name}' is not supported on ng-content.`;
this.pushDiagnostic(
id,
message,
attr.sourceSpan,
OutOfBandDiagnosticCategory.Error,
ErrorCode.TEMPLATE_PARSE_ERROR,
);
}

/**
* Span covering only the tag name of a node, since highlighting the entire start tag can be
* noisy. Falls back to the whole start tag for nodes without a name (e.g. `<ng-template>`).
Expand Down
22 changes: 20 additions & 2 deletions packages/compiler-cli/src/ngtsc/typecheck/src/oob.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,9 +16,9 @@ import {
ParseSourceSpan,
PropertyRead,
TcbDirectiveMetadata,
TmplAstBoundaryBlock,
TmplAstBoundAttribute,
TmplAstBoundEvent,
TmplAstBoundaryBlock,
TmplAstElement,
TmplAstForLoopBlock,
TmplAstForLoopBlockEmpty,
Expand All @@ -40,8 +40,8 @@ import {ErrorCode, ngErrorCode} from '../../diagnostics';
import {TemplateDiagnostic} from '../api';
import {makeTemplateDiagnostic} from '../diagnostics';

import {TypeCheckSourceResolver} from './tcb_util';
import {DOC_PAGE_BASE_URL} from '../../diagnostics/src/error_details_base_url';
import {TypeCheckSourceResolver} from './tcb_util';

export class OutOfBandDiagnosticRecorderImpl implements OutOfBandDiagnosticRecorder<TemplateDiagnostic> {
private readonly _diagnostics: TemplateDiagnostic[] = [];
Expand Down Expand Up @@ -681,6 +681,24 @@ export class OutOfBandDiagnosticRecorderImpl implements OutOfBandDiagnosticRecor
);
}

unsupportedNgContentAttribute(id: TypeCheckId, attr: TmplAstTextAttribute): void {
const lower = attr.name.toLowerCase();
const message =
lower === 'selection' || lower === 'selector'
? `'${attr.name}' is not supported on ng-content. Did you mean 'select'?`
: `'${attr.name}' is not supported on ng-content.`;
this._diagnostics.push(
makeTemplateDiagnostic(
id,
this.resolver.getTemplateSourceMapping(id),
attr.sourceSpan,
ts.DiagnosticCategory.Error,
ngErrorCode(ErrorCode.TEMPLATE_PARSE_ERROR),
message,
),
);
}

private getTagNameSpan(node: TmplAstElement | TmplAstTemplate) {
let span: ParseSourceSpan;
const name = node instanceof TmplAstElement ? node.name : null;
Expand Down
109 changes: 109 additions & 0 deletions packages/compiler-cli/src/ngtsc/typecheck/test/diagnostics_spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1562,6 +1562,115 @@ class TestComponent {

expect(messages).toEqual([]);
});

describe('ng-content attributes', () => {
it('should report an error for unsupported attributes on ng-content', () => {
const messages = diagnose(
`<ng-content id="test" class="foo"></ng-content>`,
`export class TestComponent {}`,
);

expect(messages).toEqual([
`TestComponent.html(1, 13): 'id' is not supported on ng-content.`,
`TestComponent.html(1, 23): 'class' is not supported on ng-content.`,
]);
});

it('should report an error and suggestion when selection or selector is used on ng-content', () => {
const messages = diagnose(
`<ng-content selection="header"></ng-content><ng-content selector="footer"></ng-content>`,
`export class TestComponent {}`,
);

expect(messages).toEqual([
`TestComponent.html(1, 13): 'selection' is not supported on ng-content. Did you mean 'select'?`,
`TestComponent.html(1, 57): 'selector' is not supported on ng-content. Did you mean 'select'?`,
]);
});

it('should allow valid attributes on ng-content', () => {
const messages = diagnose(
`
<ng-content
select="header"
ngProjectAs="[header]"
ngSkipHydration
ngPreserveWhitespaces>
</ng-content>
`,
`export class TestComponent {}`,
);

expect(messages).toEqual([]);
});

it('should allow static attributes on ng-content that match a parent projection slot', () => {
const messages = diagnose(
`<proj><ng-content card-header /><ng-content class="card-body" /></proj>`,
`
export class Proj {}
export class TestComponent {}
`,
[
{
type: 'directive',
name: 'Proj',
selector: 'proj',
isComponent: true,
ngContentSelectors: ['[card-header]', '.card-body', '*'],
},
],
);

expect(messages).toEqual([]);
});

it('should report static attributes on ng-content that do not match a parent projection slot', () => {
const messages = diagnose(
`<proj><ng-content class="card-body" title="unused" /></proj>`,
`
export class Proj {}
export class TestComponent {}
`,
[
{
type: 'directive',
name: 'Proj',
selector: 'proj',
isComponent: true,
ngContentSelectors: ['[card-header]', '.card-body', '*'],
},
],
);

expect(messages).toEqual([
`TestComponent.html(1, 37): 'title' is not supported on ng-content.`,
]);
});

it('should report static attributes on ng-content projected into a catch-all slot', () => {
const messages = diagnose(
`<catch-all><ng-content class="foo" /></catch-all>`,
`
export class CatchAll {}
export class TestComponent {}
`,
[
{
type: 'directive',
name: 'CatchAll',
selector: 'catch-all',
isComponent: true,
ngContentSelectors: ['*'],
},
],
);

expect(messages).toEqual([
`TestComponent.html(1, 24): 'class' is not supported on ng-content.`,
]);
});
});
});

// https://github.com/angular/angular/issues/43970
Expand Down
1 change: 1 addition & 0 deletions packages/compiler-cli/src/ngtsc/typecheck/testing/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -852,6 +852,7 @@ export class NoopOobRecorder implements OutOfBandDiagnosticRecorder<TemplateDiag
| TmplAstViewportDeferredTrigger,
): void {}
conflictingHostDirectiveBinding(): void {}
unsupportedNgContentAttribute(): void {}
}

export function createNgCompilerForFile(fileContent: string) {
Expand Down
8 changes: 7 additions & 1 deletion packages/compiler-cli/src/ngtsc/typecheck/testing/src/ngp_typecheck_testing.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -425,7 +425,13 @@ function synthesizeTsDecoratorForDeclaration(
props.push(`hostDirectives: [${hdEntries.join(', ')}]`);
}
if (decl.isComponent) {
props.push(`template: ''`);
const templateContent =
decl.ngContentSelectors
?.map((selector) =>
selector === '*' ? '<ng-content/>' : `<ng-content select=${JSON.stringify(selector)}/>`,
)
.join('') ?? '';
props.push(`template: ${JSON.stringify(templateContent)}`);
return `@ɵNgpComponent({${props.join(', ')}})`;
}
return `@ɵNgpDirective({${props.join(', ')}})`;
Expand Down
39 changes: 33 additions & 6 deletions ...ses/r3_compiler_compliance/components_and_directives/content_projection/GOLDEN_PARTIAL.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -359,27 +359,41 @@ export declare class MyApp {
/****************************************************************************************************
* PARTIAL FILE: ng_content_with_structural_dir.js
****************************************************************************************************/
import { Component } from '@angular/core';
import { Component, Directive } from '@angular/core';
import * as i0 from "@angular/core";
export class FakeNgIf {
static ɵfac = i0.ɵɵngDeclareFactory({ minVersion: "12.0.0", version: "0.0.0-PLACEHOLDER", ngImport: i0, type: FakeNgIf, deps: [], target: i0.ɵɵFactoryTarget.Directive });
static ɵdir = i0.ɵɵngDeclareDirective({ minVersion: "14.0.0", version: "0.0.0-PLACEHOLDER", type: FakeNgIf, isStandalone: true, selector: "[ngIf]", ngImport: i0 });
}
i0.ɵɵngDeclareClassMetadata({ minVersion: "12.0.0", version: "0.0.0-PLACEHOLDER", ngImport: i0, type: FakeNgIf, decorators: [{
type: Directive,
args: [{ selector: '[ngIf]' }]
}] });
export class SimpleComponent {
showContent = true;
static ɵfac = i0.ɵɵngDeclareFactory({ minVersion: "12.0.0", version: "0.0.0-PLACEHOLDER", ngImport: i0, type: SimpleComponent, deps: [], target: i0.ɵɵFactoryTarget.Component });
static ɵcmp = i0.ɵɵngDeclareComponent({ minVersion: "14.0.0", version: "0.0.0-PLACEHOLDER", type: SimpleComponent, isStandalone: false, selector: "simple", ngImport: i0, template: '<ng-content *ngIf="showContent"></ng-content>', isInline: true });
static ɵcmp = i0.ɵɵngDeclareComponent({ minVersion: "14.0.0", version: "0.0.0-PLACEHOLDER", type: SimpleComponent, isStandalone: true, selector: "simple", ngImport: i0, template: '<ng-content *ngIf="showContent"></ng-content>', isInline: true, dependencies: [{ kind: "directive", type: FakeNgIf, selector: "[ngIf]" }] });
}
i0.ɵɵngDeclareClassMetadata({ minVersion: "12.0.0", version: "0.0.0-PLACEHOLDER", ngImport: i0, type: SimpleComponent, decorators: [{
type: Component,
args: [{
imports: [FakeNgIf],
selector: 'simple', template: '<ng-content *ngIf="showContent"></ng-content>',
standalone: false
}]
}] });

/****************************************************************************************************
* PARTIAL FILE: ng_content_with_structural_dir.d.ts
****************************************************************************************************/
import * as i0 from "@angular/core";
export declare class FakeNgIf {
static ɵfac: i0.ɵɵFactoryDeclaration<FakeNgIf, never>;
static ɵdir: i0.ɵɵDirectiveDeclaration<FakeNgIf, "[ngIf]", never, {}, {}, never, never, true, never>;
}
export declare class SimpleComponent {
showContent: boolean;
static ɵfac: i0.ɵɵFactoryDeclaration<SimpleComponent, never>;
static ɵcmp: i0.ɵɵComponentDeclaration<SimpleComponent, "simple", never, {}, {}, never, ["*"], false, never>;
static ɵcmp: i0.ɵɵComponentDeclaration<SimpleComponent, "simple", never, {}, {}, never, ["*"], true, never>;
}

/****************************************************************************************************
Expand Down Expand Up @@ -464,8 +478,16 @@ export {};
/****************************************************************************************************
* PARTIAL FILE: ng_content_fallback.js
****************************************************************************************************/
import { Component } from '@angular/core';
import { Component, Directive } from '@angular/core';
import * as i0 from "@angular/core";
export class FakeNgIf {
static ɵfac = i0.ɵɵngDeclareFactory({ minVersion: "12.0.0", version: "0.0.0-PLACEHOLDER", ngImport: i0, type: FakeNgIf, deps: [], target: i0.ɵɵFactoryTarget.Directive });
static ɵdir = i0.ɵɵngDeclareDirective({ minVersion: "14.0.0", version: "0.0.0-PLACEHOLDER", type: FakeNgIf, isStandalone: true, selector: "[ngIf]", ngImport: i0 });
}
i0.ɵɵngDeclareClassMetadata({ minVersion: "12.0.0", version: "0.0.0-PLACEHOLDER", ngImport: i0, type: FakeNgIf, decorators: [{
type: Directive,
args: [{ selector: '[ngIf]' }]
}] });
export class TestComponent {
type = 'complex';
hasFooter = false;
Expand All @@ -489,12 +511,13 @@ export class TestComponent {
<ng-content select="structural" *ngIf="hasStructural">
<h2>With a structural directive</h2>
</ng-content>
`, isInline: true });
`, isInline: true, dependencies: [{ kind: "directive", type: FakeNgIf, selector: "[ngIf]" }] });
}
i0.ɵɵngDeclareClassMetadata({ minVersion: "12.0.0", version: "0.0.0-PLACEHOLDER", ngImport: i0, type: TestComponent, decorators: [{
type: Component,
args: [{
selector: 'test',
imports: [FakeNgIf],
template: `
<ng-content select="basic">Basic fallback</ng-content>

Expand All @@ -521,6 +544,10 @@ i0.ɵɵngDeclareClassMetadata({ minVersion: "12.0.0", version: "0.0.0-PLACEHOLDE
* PARTIAL FILE: ng_content_fallback.d.ts
****************************************************************************************************/
import * as i0 from "@angular/core";
export declare class FakeNgIf {
static ɵfac: i0.ɵɵFactoryDeclaration<FakeNgIf, never>;
static ɵdir: i0.ɵɵDirectiveDeclaration<FakeNgIf, "[ngIf]", never, {}, {}, never, never, true, never>;
}
export declare class TestComponent {
type: string;
hasFooter: boolean;
Expand Down
Loading
Loading