Skip to content
This repository has been archived by the owner on Sep 16, 2022. It is now read-only.

Commit

Permalink
fix(Compiler): Issue an error on improper @deferred usage.
Browse files Browse the repository at this point in the history
Closes #1538.

PiperOrigin-RevId: 207997994
  • Loading branch information
matanlurey authored and leonsenft committed Aug 9, 2018
1 parent 3db5ab8 commit a1b4b60
Show file tree
Hide file tree
Showing 5 changed files with 122 additions and 4 deletions.
54 changes: 54 additions & 0 deletions _tests/test/regression/1538_deferred_template_test.dart
Original file line number Diff line number Diff line change
@@ -0,0 +1,54 @@
@TestOn('vm')
import 'package:_tests/compiler.dart';
import 'package:test/test.dart';

// See https://github.com/dart-lang/angular/issues/1538.
void main() {
test('should disallow @deferred on a `<template>` element', () async {
await compilesExpecting("""
import '$ngImport';
@Component(
selector: 'example',
directives: [DeferMe],
template: r'''
<template @deferred>
<defer-me></defer-me>
</template>
''',
)
class Example {}
@Component(
selector: 'defer-me',
template: '',
)
class DeferMe {}
""", errors: [
contains('Invalid @deferred annotation'),
]);
});

test('should disallow @deferred with a structural directive', () async {
await compilesExpecting("""
import '$ngImport';
@Component(
selector: 'example',
directives: [DeferMe, NgIf],
template: r'''
<defer-me *ngIf="true" @deferred></defer-me>
''',
)
class Example {}
@Component(
selector: 'defer-me',
template: '',
)
class DeferMe {}
""", errors: [
contains('Invalid @deferred annotation'),
]);
});
}
28 changes: 28 additions & 0 deletions angular/CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,3 +1,31 @@
### Bug fixes

* [#1538]() A compile-time error is reported if the `@deferred` template
annotation is present on a `<template>` element or is a sibling to a
structural directive (such as `*ngIf`). Before we would silently drop/ignore
the annotation, so this might be considered a breaking change of an
incorrect program. The fix is just to move the annotation, such as:

```html
<!-- Before (Both are identical) -->
<template @deferred [ngIf]="showArea>
<expensive-comp></expensive-comp>
</template>
<expensive-comp *ngIf="showArea" @deferred></expensive-comp>
<!-- After (Both are identical) -->
<template [ngIf]="showArea">
<expensive-comp @deferred></expensive-comp>
</template>
<ng-container *ngIf="showArea">
<expensive-comp @deferred></expensive-comp>
</ng-container>
```
[#1538]: https://github.com/dart-lang/angular/issues/153
## 5.0.0
Welcome to AngularDart v5.0.0, with full support for Dart 2. Please note that
Expand Down
3 changes: 3 additions & 0 deletions angular_ast/CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,5 +1,8 @@
* Annotations may now have compound names (for example `@foo.bar`).

* It is now an error to use `@deferred` on a `<template>` tag or combined with
a structural (i.e. `*ngIf`) directive.

## 0.5.6

* Maintenance release to support Dart 2.0 stable.
Expand Down
11 changes: 11 additions & 0 deletions angular_ast/lib/src/exception_handler/exceptions.dart
Original file line number Diff line number Diff line change
Expand Up @@ -177,6 +177,17 @@ class NgParserWarningCode extends ErrorCode {
NgParserWarningCode('INVALID_DECORATOR_IN_TEMPLATE',
"Invalid decorator in 'template' element");

static const NgParserWarningCode INVALID_DEFERRED_ON_TEMPLATE =
NgParserWarningCode(
'INVALID_DEFERRED_ON_TEMPLATE',
"Invalid @deferred annotation on 'template' element",
"The @deferred annotation cannot be placed on an element with "
"a structural directive (such as *ngIf) or on a <template> tag. "
"Consider moving the structural directive as the parent, such as:\n\n"
" <ng-container *ngIf=\"someCondition\">\n"
" <expensive-comp @defered></expensive-comp>\n"
" </ng-container>");

static const NgParserWarningCode INVALID_LET_BINDING_IN_NONTEMPLATE =
NgParserWarningCode('INVALID_LET_BINDING_IN_NONTEMPLATE',
"'let-' binding can only be used in 'template' element");
Expand Down
30 changes: 26 additions & 4 deletions angular_ast/lib/src/visitors/desugar_visitor.dart
Original file line number Diff line number Diff line change
Expand Up @@ -84,14 +84,19 @@ class DesugarVisitor implements TemplateAstVisitor<TemplateAst, String> {
astNode.bananas.clear();
}

if (astNode.stars.isNotEmpty) {
return _desugarStar(astNode, astNode.stars);
}

if (astNode.annotations.isNotEmpty) {
final i = astNode.annotations.indexWhere((ast) => ast.name == 'deferred');
if (i != -1) {
final deferredAst = astNode.annotations.removeAt(i);
// Fail on invalid use (i.e. on a <template> tag):
// (https://github.com/dart-lang/angular/issues/1538)
if (astNode.stars.isNotEmpty) {
exceptionHandler.handle(AngularParserException(
NgParserWarningCode.INVALID_DEFERRED_ON_TEMPLATE,
deferredAst.sourceSpan.start.offset,
deferredAst.sourceSpan.length,
));
}
final origin = _toolFriendlyAstOrigin ? deferredAst : null;
return EmbeddedTemplateAst.from(
origin,
Expand All @@ -101,6 +106,10 @@ class DesugarVisitor implements TemplateAstVisitor<TemplateAst, String> {
}
}

if (astNode.stars.isNotEmpty) {
return _desugarStar(astNode, astNode.stars);
}

return astNode;
}

Expand All @@ -119,6 +128,19 @@ class DesugarVisitor implements TemplateAstVisitor<TemplateAst, String> {

@override
TemplateAst visitEmbeddedTemplate(EmbeddedTemplateAst astNode, [_]) {
if (astNode.annotations.isNotEmpty) {
final i = astNode.annotations.indexWhere((ast) => ast.name == 'deferred');
if (i != -1) {
final deferredAst = astNode.annotations.removeAt(i);
// Fail on invalid use (i.e. on a <template> tag):
// (https://github.com/dart-lang/angular/issues/1538)
exceptionHandler.handle(AngularParserException(
NgParserWarningCode.INVALID_DEFERRED_ON_TEMPLATE,
deferredAst.sourceSpan.start.offset,
deferredAst.sourceSpan.length,
));
}
}
_visitChildren(astNode);
return astNode;
}
Expand Down

0 comments on commit a1b4b60

Please sign in to comment.