Skip to content

Commit

Permalink
fix(compiler-cli): dom property binding check in signal extended diag…
Browse files Browse the repository at this point in the history
…nostic

The compiler now checks if a signal is properly called on dom property bindings.
The ideal solution would be for the compiler to check if dom property bindings in general are properly typed,
but this is currently not the case, and it is a bigger task to land this change.
In the meantime, the signal diagnostic is augmented to catch cases like the following:

```
<div [id]="mySignal"></div>
```
  • Loading branch information
cexbrayat committed May 16, 2024
1 parent 400911e commit d93ae67
Show file tree
Hide file tree
Showing 2 changed files with 165 additions and 5 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,15 @@
* found in the LICENSE file at https://angular.io/license
*/

import {AST, Interpolation, PropertyRead, TmplAstNode} from '@angular/compiler';
import {
AST,
ASTWithSource,
BindingType,
Interpolation,
PropertyRead,
TmplAstBoundAttribute,
TmplAstNode,
} from '@angular/compiler';
import ts from 'typescript';

import {ErrorCode, ExtendedTemplateDiagnosticName} from '../../../../diagnostics';
Expand Down Expand Up @@ -34,11 +42,28 @@ class InterpolatedSignalCheck extends TemplateCheckWithVisitor<ErrorCode.INTERPO
component: ts.ClassDeclaration,
node: TmplAstNode | AST,
): NgTemplateDiagnostic<ErrorCode.INTERPOLATED_SIGNAL_NOT_INVOKED>[] {
// interpolations like `{{ mySignal }}`
if (node instanceof Interpolation) {
return node.expressions
.filter((item): item is PropertyRead => item instanceof PropertyRead)
.flatMap((item) => buildDiagnosticForSignal(ctx, item, component));
}
// bound properties like `[prop]="mySignal"`
else if (node instanceof TmplAstBoundAttribute) {
const symbol = ctx.templateTypeChecker.getSymbolOfNode(node, component);
// we skip the check if the node is an input binding
if (symbol !== null && symbol.kind === SymbolKind.Input) {
return [];
}
// otherwise, we check if the node is a bound property like `[prop]="mySignal"`
if (
node.type === BindingType.Property &&
node.value instanceof ASTWithSource &&
node.value.ast instanceof PropertyRead
) {
return buildDiagnosticForSignal(ctx, node.value.ast, component);
}
}
return [];
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,7 @@ runInEachFileSystem(() => {
export class TestCmp {
mySignal1 = signal<number>(0);
mySignal2:Signal<number>;
mySignal2: Signal<number>;
}`,
},
]);
Expand Down Expand Up @@ -128,7 +128,7 @@ runInEachFileSystem(() => {
'TestCmp': `<div>{{ mySignal2 }}</div>`,
},
source: `
import {signal, Signal, computed} from '@angular/core';
import {signal, computed} from '@angular/core';
export class TestCmp {
mySignal1 = signal<number>(0);
Expand Down Expand Up @@ -275,6 +275,82 @@ runInEachFileSystem(() => {
expect(getSourceCodeForDiagnostic(diags[0])).toBe(`myRequiredModel`);
});

it('should not produce a warning when a signal is not invoked in a banana in box binding', () => {
const fileName = absoluteFrom('/main.ts');
const {program, templateTypeChecker} = setup([
{
fileName,
templates: {
'TestCmp': `<div [(value)]="signal">{{ myRequiredModel }}</div>`,
},
source: `
import {signal} from '@angular/core';
export class TestCmp {
mySignal = signal(0);
}`,
},
]);
const sf = getSourceFileOrError(program, fileName);
const component = getClass(sf, 'TestCmp');
const extendedTemplateChecker = new ExtendedTemplateCheckerImpl(
templateTypeChecker,
program.getTypeChecker(),
[interpolatedSignalFactory],
{} /* options */,
);
const diags = extendedTemplateChecker.getDiagnosticsForComponent(component);
expect(diags.length).toBe(0);
});

it('should not produce a warning when a signal is not invoked in an input binding as they are skipped', () => {
const fileName = absoluteFrom('/main.ts');
const {program, templateTypeChecker} = setup([
{
fileName,
templates: {
'TestCmp': `<div dir [myInput]="mySignal"></div>`,
},
source: `
import {signal, input} from '@angular/core';
export class TestDir {
myInput = input.required();
}
export class TestCmp {
mySignal = signal(0);
}`,
declarations: [
{
type: 'directive',
name: 'TestDir',
selector: '[dir]',
inputs: {
myInput: {
isSignal: true,
bindingPropertyName: 'myInput',
classPropertyName: 'myInput',
required: true,
transform: null,
},
},
},
],
},
]);
const sf = getSourceFileOrError(program, fileName);
const component = getClass(sf, 'TestCmp');
const extendedTemplateChecker = new ExtendedTemplateCheckerImpl(
templateTypeChecker,
program.getTypeChecker(),
[interpolatedSignalFactory],
{},
/* options */
);
const diags = extendedTemplateChecker.getDiagnosticsForComponent(component);
expect(diags.length).toBe(0);
});

it('should produce a warning when a signal in a nested property read is not invoked', () => {
const fileName = absoluteFrom('/main.ts');
const {program, templateTypeChecker} = setup([
Expand All @@ -284,7 +360,7 @@ runInEachFileSystem(() => {
'TestCmp': `<div>{{ obj.nested.prop.signal }}</div>`,
},
source: `
import {signal, Signal} from '@angular/core';
import {signal} from '@angular/core';
export class TestCmp {
obj = {
Expand Down Expand Up @@ -350,7 +426,7 @@ runInEachFileSystem(() => {
'TestCmp': `<div>{{ mySignal2() }}</div>`,
},
source: `
import {signal, Signal, computed} from '@angular/core';
import {signal, computed} from '@angular/core';
export class TestCmp {
mySignal1 = signal<number>(0);
Expand Down Expand Up @@ -549,6 +625,65 @@ runInEachFileSystem(() => {
expect(getSourceCodeForDiagnostic(diags[0])).toBe(`myNestedSignal`);
});

it("should produce a warning when signal isn't invoked on dom property binding", () => {
const fileName = absoluteFrom('/main.ts');
const {program, templateTypeChecker} = setup([
{
fileName,
templates: {
'TestCmp': `<div [id]="mySignal"></div>`,
},
source: `
import {signal} from '@angular/core';
export class TestCmp {
mySignal = signal<number>(0);
}`,
},
]);
const sf = getSourceFileOrError(program, fileName);
const component = getClass(sf, 'TestCmp');
const extendedTemplateChecker = new ExtendedTemplateCheckerImpl(
templateTypeChecker,
program.getTypeChecker(),
[interpolatedSignalFactory],
{} /* options */,
);
const diags = extendedTemplateChecker.getDiagnosticsForComponent(component);
expect(diags.length).toBe(1);
expect(diags[0].category).toBe(ts.DiagnosticCategory.Warning);
expect(diags[0].code).toBe(ngErrorCode(ErrorCode.INTERPOLATED_SIGNAL_NOT_INVOKED));
expect(getSourceCodeForDiagnostic(diags[0])).toBe(`mySignal`);
});

it('should not produce a warning when signal is invoked on dom property binding', () => {
const fileName = absoluteFrom('/main.ts');
const {program, templateTypeChecker} = setup([
{
fileName,
templates: {
'TestCmp': `<div [id]="mySignal()"></div>`,
},
source: `
import {signal} from '@angular/core';
export class TestCmp {
mySignal = signal<number>(0);
}`,
},
]);
const sf = getSourceFileOrError(program, fileName);
const component = getClass(sf, 'TestCmp');
const extendedTemplateChecker = new ExtendedTemplateCheckerImpl(
templateTypeChecker,
program.getTypeChecker(),
[interpolatedSignalFactory],
{} /* options */,
);
const diags = extendedTemplateChecker.getDiagnosticsForComponent(component);
expect(diags.length).toBe(0);
});

it('should not produce a warning with other Signal type', () => {
const fileName = absoluteFrom('/main.ts');
const {program, templateTypeChecker} = setup([
Expand Down

0 comments on commit d93ae67

Please sign in to comment.