Skip to content

Commit 94e203b

Browse files
Bertrand Laportemhevery
authored andcommitted
feat(DirectiveParser): throw errors when expected directives are not present
closes angular#527 Closes angular#570
1 parent 715ee14 commit 94e203b

18 files changed

+354
-123
lines changed

modules/angular2/src/core/compiler/compiler.js

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@ import {Template} from '../annotations/template';
1717
import {ShadowDomStrategy} from './shadow_dom_strategy';
1818
import {CompileStep} from './pipeline/compile_step';
1919

20+
2021
/**
2122
* Cache that stores the ProtoView of the template of a component.
2223
* Used to prevent duplicate work and resolve cyclic dependencies.
@@ -134,7 +135,15 @@ export class Compiler {
134135
// TODO(vicb): union type return ProtoView or Promise<ProtoView>
135136
_compileTemplate(template: Template, tplElement: Element, component: Type) {
136137
var pipeline = new CompilePipeline(this.createSteps(component, template));
137-
var compileElements = pipeline.process(tplElement);
138+
var compilationCtxtDescription = stringify(this._reader.read(component).type);
139+
var compileElements;
140+
141+
try {
142+
compileElements = pipeline.process(tplElement, compilationCtxtDescription);
143+
} catch(ex) {
144+
return PromiseWrapper.reject(ex);
145+
}
146+
138147
var protoView = compileElements[0].inheritedProtoView;
139148

140149
// Populate the cache before compiling the nested components,

modules/angular2/src/core/compiler/pipeline/compile_element.js

Lines changed: 44 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
import {List, Map, ListWrapper, MapWrapper} from 'angular2/src/facade/collection';
22
import {Element, DOM} from 'angular2/src/facade/dom';
3-
import {int, isBlank, isPresent, Type} from 'angular2/src/facade/lang';
3+
import {int, isBlank, isPresent, Type, StringJoiner, assertionsEnabled} from 'angular2/src/facade/lang';
44
import {DirectiveMetadata} from '../directive_metadata';
55
import {Decorator, Component, Viewport} from '../../annotations/annotations';
66
import {ElementBinder} from '../element_binder';
@@ -38,8 +38,9 @@ export class CompileElement {
3838
distanceToParentInjector:number;
3939
compileChildren: boolean;
4040
ignoreBindings: boolean;
41+
elementDescription: string; // e.g. '<div [class]="foo">' : used to provide context in case of error
4142

42-
constructor(element:Element) {
43+
constructor(element:Element, compilationUnit = '') {
4344
this.element = element;
4445
this._attrs = null;
4546
this._classList = null;
@@ -66,6 +67,14 @@ export class CompileElement {
6667
this.compileChildren = true;
6768
// set to true to ignore all the bindings on the element
6869
this.ignoreBindings = false;
70+
// description is calculated here as compilation steps may change the element
71+
var tplDesc = assertionsEnabled()? getElementDescription(element) : null;
72+
if (compilationUnit !== '') {
73+
this.elementDescription = compilationUnit;
74+
if (isPresent(tplDesc)) this.elementDescription += ": " + tplDesc;
75+
} else {
76+
this.elementDescription = tplDesc;
77+
}
6978
}
7079

7180
refreshAttrs() {
@@ -165,3 +174,36 @@ export class CompileElement {
165174
return this._allDirectives;
166175
}
167176
}
177+
178+
// return an HTML representation of an element start tag - without its content
179+
// this is used to give contextual information in case of errors
180+
function getElementDescription(domElement:Element):string {
181+
var buf = new StringJoiner();
182+
var atts = DOM.attributeMap(domElement);
183+
184+
buf.add("<");
185+
buf.add(DOM.tagName(domElement).toLowerCase());
186+
187+
// show id and class first to ease element identification
188+
addDescriptionAttribute(buf, "id", MapWrapper.get(atts, "id"));
189+
addDescriptionAttribute(buf, "class", MapWrapper.get(atts, "class"));
190+
MapWrapper.forEach(atts, (attValue, attName) => {
191+
if (attName !== "id" && attName !== "class") {
192+
addDescriptionAttribute(buf, attName, attValue);
193+
}
194+
});
195+
196+
buf.add(">");
197+
return buf.toString();
198+
}
199+
200+
201+
function addDescriptionAttribute(buffer:StringJoiner, attName:string, attValue) {
202+
if (isPresent(attValue)) {
203+
if (attValue.length === 0) {
204+
buffer.add(' ' + attName);
205+
} else {
206+
buffer.add(' ' + attName + '="' + attValue + '"');
207+
}
208+
}
209+
}

modules/angular2/src/core/compiler/pipeline/compile_pipeline.js

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -15,13 +15,13 @@ export class CompilePipeline {
1515
this._control = new CompileControl(steps);
1616
}
1717

18-
process(rootElement:Element):List {
18+
process(rootElement:Element, compilationCtxtDescription:string = ''):List {
1919
var results = ListWrapper.create();
20-
this._process(results, null, new CompileElement(rootElement));
20+
this._process(results, null, new CompileElement(rootElement, compilationCtxtDescription), compilationCtxtDescription);
2121
return results;
2222
}
2323

24-
_process(results, parent:CompileElement, current:CompileElement) {
24+
_process(results, parent:CompileElement, current:CompileElement, compilationCtxtDescription:string = '') {
2525
var additionalChildren = this._control.internalProcess(results, 0, parent, current);
2626

2727
if (current.compileChildren) {
@@ -31,7 +31,7 @@ export class CompilePipeline {
3131
// next sibling before recursing.
3232
var nextNode = DOM.nextSibling(node);
3333
if (DOM.isElementNode(node)) {
34-
this._process(results, current, new CompileElement(node));
34+
this._process(results, current, new CompileElement(node, compilationCtxtDescription));
3535
}
3636
node = nextNode;
3737
}

modules/angular2/src/core/compiler/pipeline/default_steps.js

Lines changed: 4 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,6 @@ import {ShimShadowCss} from './shim_shadow_css';
1313
import {ShimShadowDom} from './shim_shadow_dom';
1414
import {DirectiveMetadata} from 'angular2/src/core/compiler/directive_metadata';
1515
import {ShadowDomStrategy, EmulatedShadowDomStrategy} from 'angular2/src/core/compiler/shadow_dom_strategy';
16-
import {stringify} from 'angular2/src/facade/lang';
1716
import {DOM} from 'angular2/src/facade/dom';
1817

1918
/**
@@ -28,23 +27,21 @@ export function createDefaultSteps(
2827
directives: List<DirectiveMetadata>,
2928
shadowDomStrategy: ShadowDomStrategy) {
3029

31-
var compilationUnit = stringify(compiledComponent.type);
32-
33-
var steps = [new ViewSplitter(parser, compilationUnit)];
30+
var steps = [new ViewSplitter(parser)];
3431

3532
if (shadowDomStrategy instanceof EmulatedShadowDomStrategy) {
3633
var step = new ShimShadowCss(compiledComponent, shadowDomStrategy, DOM.defaultDoc().head);
3734
ListWrapper.push(steps, step);
3835
}
3936

4037
steps = ListWrapper.concat(steps,[
41-
new PropertyBindingParser(parser, compilationUnit),
38+
new PropertyBindingParser(parser),
4239
new DirectiveParser(directives),
43-
new TextInterpolationParser(parser, compilationUnit),
40+
new TextInterpolationParser(parser),
4441
new ElementBindingMarker(),
4542
new ProtoViewBuilder(changeDetection, shadowDomStrategy),
4643
new ProtoElementInjectorBuilder(),
47-
new ElementBinderBuilder(parser, compilationUnit)
44+
new ElementBinderBuilder(parser)
4845
]);
4946

5047
if (shadowDomStrategy instanceof EmulatedShadowDomStrategy) {

modules/angular2/src/core/compiler/pipeline/directive_parser.js

Lines changed: 88 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
1-
import {isPresent, isBlank, BaseException} from 'angular2/src/facade/lang';
2-
import {List, MapWrapper} from 'angular2/src/facade/collection';
1+
import {isPresent, isBlank, BaseException, assertionsEnabled, RegExpWrapper} from 'angular2/src/facade/lang';
2+
import {List, MapWrapper, StringMapWrapper} from 'angular2/src/facade/collection';
33
import {DOM} from 'angular2/src/facade/dom';
44
import {SelectorMatcher} from '../selector';
55
import {CssSelector} from '../selector';
@@ -10,6 +10,10 @@ import {CompileStep} from './compile_step';
1010
import {CompileElement} from './compile_element';
1111
import {CompileControl} from './compile_control';
1212

13+
import {isSpecialProperty} from './element_binder_builder';;
14+
15+
var PROPERTY_BINDING_REGEXP = RegExpWrapper.create('^ *([^\\s\\|]+)');
16+
1317
/**
1418
* Parses the directives on a single element. Assumes ViewSplitter has already created
1519
* <template> elements for template directives.
@@ -29,13 +33,13 @@ export class DirectiveParser extends CompileStep {
2933
_selectorMatcher:SelectorMatcher;
3034
constructor(directives:List<DirectiveMetadata>) {
3135
super();
36+
var selector;
37+
3238
this._selectorMatcher = new SelectorMatcher();
3339
for (var i=0; i<directives.length; i++) {
3440
var directiveMetadata = directives[i];
35-
this._selectorMatcher.addSelectable(
36-
CssSelector.parse(directiveMetadata.annotation.selector),
37-
directiveMetadata
38-
);
41+
selector=CssSelector.parse(directiveMetadata.annotation.selector);
42+
this._selectorMatcher.addSelectable(selector, directiveMetadata);
3943
}
4044
}
4145

@@ -67,19 +71,85 @@ export class DirectiveParser extends CompileStep {
6771
// Note: We assume that the ViewSplitter already did its work, i.e. template directive should
6872
// only be present on <template> elements any more!
6973
var isTemplateElement = DOM.isTemplateElement(current.element);
70-
this._selectorMatcher.match(cssSelector, (directive) => {
71-
if (directive.annotation instanceof Viewport) {
72-
if (!isTemplateElement) {
73-
throw new BaseException('Viewport directives need to be placed on <template> elements or elements with template attribute!');
74-
} else if (isPresent(current.viewportDirective)) {
75-
throw new BaseException('Only one template directive per element is allowed!');
76-
}
77-
} else if (isTemplateElement) {
78-
throw new BaseException('Only template directives are allowed on <template> elements!');
79-
} else if ((directive.annotation instanceof Component) && isPresent(current.componentDirective)) {
80-
throw new BaseException('Only one component directive per element is allowed!');
81-
}
74+
var matchedProperties; // StringMap - used in dev mode to store all properties that have been matched
75+
76+
this._selectorMatcher.match(cssSelector, (selector, directive) => {
77+
matchedProperties = updateMatchedProperties(matchedProperties, selector, directive);
78+
checkDirectiveValidity(directive, current, isTemplateElement);
8279
current.addDirective(directive);
8380
});
81+
82+
// raise error if some directives are missing
83+
checkMissingDirectives(current, matchedProperties, isTemplateElement);
84+
}
85+
}
86+
87+
// calculate all the properties that are used or interpreted by all directives
88+
// those properties correspond to the directive selectors and the directive bindings
89+
function updateMatchedProperties(matchedProperties, selector, directive) {
90+
if (assertionsEnabled()) {
91+
var attrs = selector.attrs;
92+
if (!isPresent(matchedProperties)) {
93+
matchedProperties = StringMapWrapper.create();
94+
}
95+
if (isPresent(attrs)) {
96+
for (var idx = 0; idx<attrs.length; idx+=2) {
97+
// attribute name is stored on even indexes
98+
StringMapWrapper.set(matchedProperties, attrs[idx], true);
99+
}
100+
}
101+
// some properties can be used by the directive, so we need to register them
102+
if (isPresent(directive.annotation) && isPresent(directive.annotation.bind)) {
103+
var bindMap = directive.annotation.bind;
104+
StringMapWrapper.forEach(bindMap, (value, key) => {
105+
// value is the name of the property that is intepreted
106+
// e.g. 'myprop' or 'myprop | double' when a pipe is used to transform the property
107+
108+
// keep the property name and remove the pipe
109+
var bindProp = RegExpWrapper.firstMatch(PROPERTY_BINDING_REGEXP, value);
110+
if (isPresent(bindProp) && isPresent(bindProp[1])) {
111+
StringMapWrapper.set(matchedProperties, bindProp[1], true);
112+
}
113+
});
114+
}
115+
}
116+
return matchedProperties;
117+
}
118+
119+
// check if the directive is compatible with the current element
120+
function checkDirectiveValidity(directive, current, isTemplateElement) {
121+
if (directive.annotation instanceof Viewport) {
122+
if (!isTemplateElement) {
123+
throw new BaseException(`Viewport directives need to be placed on <template> elements or elements ` +
124+
`with template attribute - check ${current.elementDescription}`);
125+
} else if (isPresent(current.viewportDirective)) {
126+
throw new BaseException(`Only one viewport directive can be used per element - check ${current.elementDescription}`);
127+
}
128+
} else if (isTemplateElement) {
129+
throw new BaseException(`Only template directives are allowed on template elements - check ${current.elementDescription}`);
130+
} else if ((directive.annotation instanceof Component) && isPresent(current.componentDirective)) {
131+
throw new BaseException(`Multiple component directives not allowed on the same element - check ${current.elementDescription}`);
132+
}
133+
}
134+
135+
// validates that there is no missing directive - dev mode only
136+
function checkMissingDirectives(current, matchedProperties, isTemplateElement) {
137+
if (assertionsEnabled()) {
138+
var ppBindings=current.propertyBindings;
139+
if (isPresent(ppBindings)) {
140+
// check that each property corresponds to a real property or has been matched by a directive
141+
MapWrapper.forEach(ppBindings, (expression, prop) => {
142+
if (!DOM.hasProperty(current.element, prop) && !isSpecialProperty(prop)) {
143+
if (!isPresent(matchedProperties) || !isPresent(StringMapWrapper.get(matchedProperties, prop))) {
144+
throw new BaseException(`Missing directive to handle '${prop}' in ${current.elementDescription}`);
145+
}
146+
}
147+
});
148+
}
149+
// template only store directives as attribute when they are not bound to expressions
150+
// so we have to validate the expression case too (e.g. !if="condition")
151+
if (isTemplateElement && !current.isViewRoot && !isPresent(current.viewportDirective)) {
152+
throw new BaseException(`Missing directive to handle: ${current.elementDescription}`);
153+
}
84154
}
85155
}

modules/angular2/src/core/compiler/pipeline/element_binder_builder.js

Lines changed: 12 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -34,6 +34,7 @@ function ariaSetterFactory(attrName:string) {
3434
return setterFn;
3535
}
3636

37+
const CLASS_ATTR = 'class';
3738
const CLASS_PREFIX = 'class.';
3839
var classSettersCache = StringMapWrapper.create();
3940

@@ -54,6 +55,7 @@ function classSetterFactory(className:string) {
5455
return setterFn;
5556
}
5657

58+
const STYLE_ATTR = 'style';
5759
const STYLE_PREFIX = 'style.';
5860
var styleSettersCache = StringMapWrapper.create();
5961

@@ -89,6 +91,13 @@ function roleSetter(element:Element, value) {
8991
}
9092
}
9193

94+
// tells if an attribute is handled by the ElementBinderBuilder step
95+
export function isSpecialProperty(propName:string) {
96+
return StringWrapper.startsWith(propName, ARIA_PREFIX)
97+
|| StringWrapper.startsWith(propName, CLASS_PREFIX)
98+
|| StringWrapper.startsWith(propName, STYLE_PREFIX);
99+
}
100+
92101
/**
93102
* Creates the ElementBinders and adds watches to the
94103
* ProtoChangeDetector.
@@ -115,11 +124,9 @@ function roleSetter(element:Element, value) {
115124
*/
116125
export class ElementBinderBuilder extends CompileStep {
117126
_parser:Parser;
118-
_compilationUnit:any;
119-
constructor(parser:Parser, compilationUnit:any) {
127+
constructor(parser:Parser) {
120128
super();
121129
this._parser = parser;
122-
this._compilationUnit = compilationUnit;
123130
}
124131

125132
process(parent:CompileElement, current:CompileElement, control:CompileControl) {
@@ -207,7 +214,7 @@ export class ElementBinderBuilder extends CompileStep {
207214
if (isBlank(bindingAst)) {
208215
var attributeValue = MapWrapper.get(compileElement.attrs(), elProp);
209216
if (isPresent(attributeValue)) {
210-
bindingAst = this._parser.wrapLiteralPrimitive(attributeValue, this._compilationUnit);
217+
bindingAst = this._parser.wrapLiteralPrimitive(attributeValue, compileElement.elementDescription);
211218
}
212219
}
213220

@@ -229,4 +236,4 @@ export class ElementBinderBuilder extends CompileStep {
229236
var parts = StringWrapper.split(bindConfig, RegExpWrapper.create("\\|"));
230237
return ListWrapper.map(parts, (s) => s.trim());
231238
}
232-
}
239+
}

0 commit comments

Comments
 (0)