Skip to content

Commit 66a0ec6

Browse files
crisbetokirjs
crisbeto
authored and
kirjs
committed
fix(compiler): move defer trigger assertions out of parser (#61747)
When defer blocks have a reference-based trigger without a parameter, we infer it from the placeholder block. This requires some validations like ensuring that there's only one element in the placeholder. The validations are currently implemented at the parser level which can affect tools like linters that need to pass `preserveWhitespaces: true` in order to get accurate source mappings. These changes move the validations into the template type checker so that they still get flagged, but much later in the process. Moving them over involves a bit more work, because the template type checker also sets `preserveWhitespaces: true`. Fixes #61725. PR Close #61747
1 parent bc72d7a commit 66a0ec6

File tree

11 files changed

+309
-88
lines changed

11 files changed

+309
-88
lines changed

‎goldens/public-api/compiler-cli/error_code.api.md‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -37,6 +37,8 @@ export enum ErrorCode {
3737
DECORATOR_NOT_CALLED = 1003,
3838
// (undocumented)
3939
DECORATOR_UNEXPECTED = 1005,
40+
DEFER_IMPLICIT_TRIGGER_INVALID_PLACEHOLDER = 8020,
41+
DEFER_IMPLICIT_TRIGGER_MISSING_PLACEHOLDER = 8019,
4042
DEFERRED_DEPENDENCY_IMPORTED_EAGERLY = 8014,
4143
DEFERRED_DIRECTIVE_USED_EAGERLY = 8013,
4244
DEFERRED_PIPE_USED_EAGERLY = 8012,

‎packages/compiler-cli/src/ngtsc/diagnostics/src/error_code.ts‎

Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -391,6 +391,32 @@ export enum ErrorCode {
391391
*/
392392
UNCLAIMED_DIRECTIVE_BINDING = 8018,
393393

394+
/**
395+
* An `@defer` block with an implicit trigger does not have a placeholder, for example:
396+
*
397+
* ```
398+
* @defer(on viewport) {
399+
* Hello
400+
* }
401+
* ```
402+
*/
403+
DEFER_IMPLICIT_TRIGGER_MISSING_PLACEHOLDER = 8019,
404+
405+
/**
406+
* The `@placeholder` for an implicit `@defer` trigger is not set up correctly, for example:
407+
*
408+
* ```
409+
* @defer(on viewport) {
410+
* Hello
411+
* } @placeholder {
412+
* <!-- Multiple root nodes. -->
413+
* <button></button>
414+
* <div></div>
415+
* }
416+
* ```
417+
*/
418+
DEFER_IMPLICIT_TRIGGER_INVALID_PLACEHOLDER = 8020,
419+
394420
/**
395421
* A two way binding in a template has an incorrect syntax,
396422
* parentheses outside brackets. For example:

‎packages/compiler-cli/src/ngtsc/typecheck/src/oob.ts‎

Lines changed: 62 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -216,6 +216,29 @@ export interface OutOfBandDiagnosticRecorder {
216216
directive: TmplAstDirective,
217217
node: TmplAstBoundAttribute | TmplAstTextAttribute | TmplAstBoundEvent,
218218
): void;
219+
220+
/**
221+
* Reports that an implicit deferred trigger is set on a block that does not have a placeholder.
222+
*/
223+
deferImplicitTriggerMissingPlaceholder(
224+
id: TypeCheckId,
225+
trigger:
226+
| TmplAstHoverDeferredTrigger
227+
| TmplAstInteractionDeferredTrigger
228+
| TmplAstViewportDeferredTrigger,
229+
): void;
230+
231+
/**
232+
* Reports that an implicit deferred trigger is set on a block whose placeholder is not set up
233+
* correctly (e.g. more than one root node).
234+
*/
235+
deferImplicitTriggerInvalidPlaceholder(
236+
id: TypeCheckId,
237+
trigger:
238+
| TmplAstHoverDeferredTrigger
239+
| TmplAstInteractionDeferredTrigger
240+
| TmplAstViewportDeferredTrigger,
241+
): void;
219242
}
220243

221244
export class OutOfBandDiagnosticRecorderImpl implements OutOfBandDiagnosticRecorder {
@@ -739,6 +762,45 @@ export class OutOfBandDiagnosticRecorderImpl implements OutOfBandDiagnosticRecor
739762
),
740763
);
741764
}
765+
766+
deferImplicitTriggerMissingPlaceholder(
767+
id: TypeCheckId,
768+
trigger:
769+
| TmplAstHoverDeferredTrigger
770+
| TmplAstInteractionDeferredTrigger
771+
| TmplAstViewportDeferredTrigger,
772+
): void {
773+
this._diagnostics.push(
774+
makeTemplateDiagnostic(
775+
id,
776+
this.resolver.getTemplateSourceMapping(id),
777+
trigger.sourceSpan,
778+
ts.DiagnosticCategory.Error,
779+
ngErrorCode(ErrorCode.DEFER_IMPLICIT_TRIGGER_MISSING_PLACEHOLDER),
780+
'Trigger with no parameters can only be placed on an @defer that has a @placeholder block',
781+
),
782+
);
783+
}
784+
785+
deferImplicitTriggerInvalidPlaceholder(
786+
id: TypeCheckId,
787+
trigger:
788+
| TmplAstHoverDeferredTrigger
789+
| TmplAstInteractionDeferredTrigger
790+
| TmplAstViewportDeferredTrigger,
791+
): void {
792+
this._diagnostics.push(
793+
makeTemplateDiagnostic(
794+
id,
795+
this.resolver.getTemplateSourceMapping(id),
796+
trigger.sourceSpan,
797+
ts.DiagnosticCategory.Error,
798+
ngErrorCode(ErrorCode.DEFER_IMPLICIT_TRIGGER_INVALID_PLACEHOLDER),
799+
'Trigger with no parameters can only be placed on an @defer that has a ' +
800+
'@placeholder block with exactly one root element node',
801+
),
802+
);
803+
}
742804
}
743805

744806
function makeInlineDiagnostic(

‎packages/compiler-cli/src/ngtsc/typecheck/src/type_check_block.ts‎

Lines changed: 39 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -2960,25 +2960,60 @@ class Scope {
29602960
}
29612961

29622962
if (triggers.hover !== undefined) {
2963-
this.appendReferenceBasedDeferredTrigger(block, triggers.hover);
2963+
this.validateReferenceBasedDeferredTrigger(block, triggers.hover);
29642964
}
29652965

29662966
if (triggers.interaction !== undefined) {
2967-
this.appendReferenceBasedDeferredTrigger(block, triggers.interaction);
2967+
this.validateReferenceBasedDeferredTrigger(block, triggers.interaction);
29682968
}
29692969

29702970
if (triggers.viewport !== undefined) {
2971-
this.appendReferenceBasedDeferredTrigger(block, triggers.viewport);
2971+
this.validateReferenceBasedDeferredTrigger(block, triggers.viewport);
29722972
}
29732973
}
29742974

2975-
private appendReferenceBasedDeferredTrigger(
2975+
private validateReferenceBasedDeferredTrigger(
29762976
block: TmplAstDeferredBlock,
29772977
trigger:
29782978
| TmplAstHoverDeferredTrigger
29792979
| TmplAstInteractionDeferredTrigger
29802980
| TmplAstViewportDeferredTrigger,
29812981
): void {
2982+
if (trigger.reference === null) {
2983+
if (block.placeholder === null) {
2984+
this.tcb.oobRecorder.deferImplicitTriggerMissingPlaceholder(this.tcb.id, trigger);
2985+
return;
2986+
}
2987+
2988+
let rootNode: TmplAstNode | null = null;
2989+
2990+
for (const child of block.placeholder.children) {
2991+
// Skip over empty text nodes if the host doesn't preserve whitespaces.
2992+
if (
2993+
!this.tcb.hostPreserveWhitespaces &&
2994+
child instanceof TmplAstText &&
2995+
child.value.trim().length === 0
2996+
) {
2997+
continue;
2998+
}
2999+
3000+
// Capture the first root node.
3001+
if (rootNode === null) {
3002+
rootNode = child;
3003+
} else {
3004+
// More than one root node is invalid. Reset it and break
3005+
// the loop so the assertion below can flag it.
3006+
rootNode = null;
3007+
break;
3008+
}
3009+
}
3010+
3011+
if (rootNode === null || !(rootNode instanceof TmplAstElement)) {
3012+
this.tcb.oobRecorder.deferImplicitTriggerInvalidPlaceholder(this.tcb.id, trigger);
3013+
}
3014+
return;
3015+
}
3016+
29823017
if (this.tcb.boundTarget.getDeferredTriggerTarget(block, trigger) === null) {
29833018
this.tcb.oobRecorder.inaccessibleDeferredTriggerElement(this.tcb.id, trigger);
29843019
}

‎packages/compiler-cli/src/ngtsc/typecheck/testing/index.ts‎

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -22,8 +22,11 @@ import {
2222
TmplAstComponent,
2323
TmplAstDirective,
2424
TmplAstElement,
25+
TmplAstHoverDeferredTrigger,
26+
TmplAstInteractionDeferredTrigger,
2527
TmplAstLetDeclaration,
2628
TmplAstTextAttribute,
29+
TmplAstViewportDeferredTrigger,
2730
} from '@angular/compiler';
2831
import {readFileSync} from 'fs';
2932
import path from 'path';
@@ -1042,6 +1045,20 @@ export class NoopOobRecorder implements OutOfBandDiagnosticRecorder {
10421045
id: TypeCheckId,
10431046
node: TmplAstComponent | TmplAstDirective,
10441047
): void {}
1048+
deferImplicitTriggerMissingPlaceholder(
1049+
id: TypeCheckId,
1050+
trigger:
1051+
| TmplAstHoverDeferredTrigger
1052+
| TmplAstInteractionDeferredTrigger
1053+
| TmplAstViewportDeferredTrigger,
1054+
): void {}
1055+
deferImplicitTriggerInvalidPlaceholder(
1056+
id: TypeCheckId,
1057+
trigger:
1058+
| TmplAstHoverDeferredTrigger
1059+
| TmplAstInteractionDeferredTrigger
1060+
| TmplAstViewportDeferredTrigger,
1061+
): void {}
10451062
}
10461063

10471064
export function createNgCompilerForFile(fileContent: string) {

‎packages/compiler-cli/test/ngtsc/defer_spec.ts‎

Lines changed: 149 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1570,5 +1570,154 @@ runInEachFileSystem(() => {
15701570
'CmpA => { i0.ɵsetClassMetadata(TestCmp',
15711571
);
15721572
});
1573+
1574+
describe('trigger validation', () => {
1575+
it('should report if reference-based trigger has no reference and there is no placeholder block but a hydrate trigger exists', () => {
1576+
env.write(
1577+
'/test.ts',
1578+
`
1579+
import {Component} from '@angular/core';
1580+
1581+
@Component({template: '@defer (on viewport; hydrate on immediate) {hello}'})
1582+
export class TestCmp {}
1583+
`,
1584+
);
1585+
1586+
const diags = env.driveDiagnostics();
1587+
expect(diags.length).toBe(1);
1588+
expect(diags[0].messageText).toBe(
1589+
'Trigger with no parameters can only be placed on an @defer that has a @placeholder block',
1590+
);
1591+
});
1592+
1593+
it('should report if reference-based trigger has no reference and there is no placeholder block but a hydrate trigger exists and it is also viewport', () => {
1594+
env.write(
1595+
'/test.ts',
1596+
`
1597+
import {Component} from '@angular/core';
1598+
1599+
@Component({template: '@defer (on viewport; hydrate on viewport) {hello}'})
1600+
export class TestCmp {}
1601+
`,
1602+
);
1603+
1604+
const diags = env.driveDiagnostics();
1605+
expect(diags.length).toBe(1);
1606+
expect(diags[0].messageText).toBe(
1607+
'Trigger with no parameters can only be placed on an @defer that has a @placeholder block',
1608+
);
1609+
});
1610+
1611+
it('should report if reference-based trigger has no reference and the placeholder is empty', () => {
1612+
env.write(
1613+
'/test.ts',
1614+
`
1615+
import {Component} from '@angular/core';
1616+
1617+
@Component({template: '@defer (on viewport) {hello} @placeholder {}'})
1618+
export class TestCmp {}
1619+
`,
1620+
);
1621+
1622+
const diags = env.driveDiagnostics();
1623+
expect(diags.length).toBe(1);
1624+
expect(diags[0].messageText).toBe(
1625+
'Trigger with no parameters can only be placed on an @defer that has a @placeholder block with exactly one root element node',
1626+
);
1627+
});
1628+
1629+
it('should report if reference-based trigger has no reference and the placeholder with text at the root', () => {
1630+
env.write(
1631+
'/test.ts',
1632+
`
1633+
import {Component} from '@angular/core';
1634+
1635+
@Component({template: '@defer (on viewport) {hello} @placeholder {placeholder}'})
1636+
export class TestCmp {}
1637+
`,
1638+
);
1639+
1640+
const diags = env.driveDiagnostics();
1641+
expect(diags.length).toBe(1);
1642+
expect(diags[0].messageText).toBe(
1643+
'Trigger with no parameters can only be placed on an @defer that has a @placeholder block with exactly one root element node',
1644+
);
1645+
});
1646+
1647+
it('should report if reference-based trigger has no reference and there is no placeholder block', () => {
1648+
env.write(
1649+
'/test.ts',
1650+
`
1651+
import {Component} from '@angular/core';
1652+
1653+
@Component({template: '@defer (on viewport) {hello}'})
1654+
export class TestCmp {}
1655+
`,
1656+
);
1657+
1658+
const diags = env.driveDiagnostics();
1659+
expect(diags.length).toBe(1);
1660+
expect(diags[0].messageText).toBe(
1661+
'Trigger with no parameters can only be placed on an @defer that has a @placeholder block',
1662+
);
1663+
});
1664+
1665+
it('should report if reference-based trigger has no reference and the placeholder has multiple root elements', () => {
1666+
env.write(
1667+
'/test.ts',
1668+
`
1669+
import {Component} from '@angular/core';
1670+
1671+
@Component({template: '@defer (on viewport) {hello} @placeholder {<div></div><span></span>}'})
1672+
export class TestCmp {}
1673+
`,
1674+
);
1675+
1676+
const diags = env.driveDiagnostics();
1677+
expect(diags.length).toBe(1);
1678+
expect(diags[0].messageText).toBe(
1679+
'Trigger with no parameters can only be placed on an @defer that has a @placeholder block with exactly one root element node',
1680+
);
1681+
});
1682+
1683+
it('should report if reference-based trigger has no reference and the placeholder has one root element and some text', () => {
1684+
env.write(
1685+
'/test.ts',
1686+
`
1687+
import {Component} from '@angular/core';
1688+
1689+
@Component({template: '@defer (on viewport) {hello} @placeholder {<div></div> hi}'})
1690+
export class TestCmp {}
1691+
`,
1692+
);
1693+
1694+
const diags = env.driveDiagnostics();
1695+
expect(diags.length).toBe(1);
1696+
expect(diags[0].messageText).toBe(
1697+
'Trigger with no parameters can only be placed on an @defer that has a @placeholder block with exactly one root element node',
1698+
);
1699+
});
1700+
1701+
it('should count whitespace as a root node when preserveWhitespaces is enabled', () => {
1702+
env.write(
1703+
'/test.ts',
1704+
`
1705+
import {Component} from '@angular/core';
1706+
1707+
@Component({
1708+
preserveWhitespaces: true,
1709+
template: '@defer (on viewport) {hello} @placeholder {<div></div> }'
1710+
})
1711+
export class TestCmp {}
1712+
`,
1713+
);
1714+
1715+
const diags = env.driveDiagnostics();
1716+
expect(diags.length).toBe(1);
1717+
expect(diags[0].messageText).toBe(
1718+
'Trigger with no parameters can only be placed on an @defer that has a @placeholder block with exactly one root element node',
1719+
);
1720+
});
1721+
});
15731722
});
15741723
});

0 commit comments

Comments
 (0)