refactor(DirectiveParser): remove checks for missing directives

Based on the discussion in #776 we can't reliably check if a given
element has a particular property at the compilation time. As such
the existing algorithm detecting "missing" directives can't be used.

We need to see if there is a different / better algorithm or maybe
those checks need to be moved later in the process (runtime). Leaving
integration tests in place (disabled) so we can come back to the
topic after unblocking the situation.

This commit effectivelly reverts 94e203b9df
This commit is contained in:
Pawel Kozlowski 2015-03-19 20:56:19 +01:00
parent b35f288794
commit 81f3f32217
3 changed files with 63 additions and 123 deletions

View File

@ -1,5 +1,5 @@
import {isPresent, isBlank, BaseException, assertionsEnabled, RegExpWrapper} from 'angular2/src/facade/lang'; import {isPresent, isBlank, BaseException, assertionsEnabled, RegExpWrapper} from 'angular2/src/facade/lang';
import {List, MapWrapper, StringMapWrapper} from 'angular2/src/facade/collection'; import {List, MapWrapper} from 'angular2/src/facade/collection';
import {DOM} from 'angular2/src/dom/dom_adapter'; import {DOM} from 'angular2/src/dom/dom_adapter';
import {SelectorMatcher} from '../selector'; import {SelectorMatcher} from '../selector';
import {CssSelector} from '../selector'; import {CssSelector} from '../selector';
@ -10,9 +10,6 @@ import {CompileStep} from './compile_step';
import {CompileElement} from './compile_element'; import {CompileElement} from './compile_element';
import {CompileControl} from './compile_control'; import {CompileControl} from './compile_control';
import {isSpecialProperty} from './element_binder_builder';
import {dashCaseToCamelCase, camelCaseToDashCase} from './util';
var PROPERTY_BINDING_REGEXP = RegExpWrapper.create('^ *([^\\s\\|]+)'); var PROPERTY_BINDING_REGEXP = RegExpWrapper.create('^ *([^\\s\\|]+)');
/** /**
@ -60,51 +57,13 @@ export class DirectiveParser extends CompileStep {
}); });
// Note: We assume that the ViewSplitter already did its work, i.e. template directive should // Note: We assume that the ViewSplitter already did its work, i.e. template directive should
// only be present on <template> elements any more! // only be present on <template> elements!
var isTemplateElement = DOM.isTemplateElement(current.element); var isTemplateElement = DOM.isTemplateElement(current.element);
var matchedProperties; // StringMap - used in dev mode to store all properties that have been matched
this._selectorMatcher.match(cssSelector, (selector, directive) => { this._selectorMatcher.match(cssSelector, (selector, directive) => {
matchedProperties = updateMatchedProperties(matchedProperties, selector, directive); current.addDirective(checkDirectiveValidity(directive, current, isTemplateElement));
checkDirectiveValidity(directive, current, isTemplateElement);
current.addDirective(directive);
});
// raise error if some directives are missing
checkMissingDirectives(current, matchedProperties, isTemplateElement);
}
}
// calculate all the properties that are used or interpreted by all directives
// those properties correspond to the directive selectors and the directive bindings
function updateMatchedProperties(matchedProperties, selector, directive) {
if (assertionsEnabled()) {
var attrs = selector.attrs;
if (!isPresent(matchedProperties)) {
matchedProperties = StringMapWrapper.create();
}
if (isPresent(attrs)) {
for (var idx = 0; idx<attrs.length; idx+=2) {
// attribute name is stored on even indexes
StringMapWrapper.set(matchedProperties, dashCaseToCamelCase(attrs[idx]), true);
}
}
// some properties can be used by the directive, so we need to register them
if (isPresent(directive.annotation) && isPresent(directive.annotation.bind)) {
var bindMap = directive.annotation.bind;
StringMapWrapper.forEach(bindMap, (value, key) => {
// value is the name of the property that is interpreted
// e.g. 'myprop' or 'myprop | double' when a pipe is used to transform the property
// keep the property name and remove the pipe
var bindProp = RegExpWrapper.firstMatch(PROPERTY_BINDING_REGEXP, value);
if (isPresent(bindProp) && isPresent(bindProp[1])) {
StringMapWrapper.set(matchedProperties, dashCaseToCamelCase(bindProp[1]), true);
}
}); });
} }
}
return matchedProperties;
} }
// check if the directive is compatible with the current element // check if the directive is compatible with the current element
@ -125,26 +84,6 @@ function checkDirectiveValidity(directive, current, isTemplateElement) {
} else if (isComponent && alreadyHasComponent) { } else if (isComponent && alreadyHasComponent) {
throw new BaseException(`Multiple component directives not allowed on the same element - check ${current.elementDescription}`); throw new BaseException(`Multiple component directives not allowed on the same element - check ${current.elementDescription}`);
} }
}
// validates that there is no missing directive - dev mode only return directive;
function checkMissingDirectives(current, matchedProperties, isTemplateElement) {
if (assertionsEnabled()) {
var ppBindings=current.propertyBindings;
if (isPresent(ppBindings)) {
// check that each property corresponds to a real property or has been matched by a directive
MapWrapper.forEach(ppBindings, (expression, prop) => {
if (!DOM.hasProperty(current.element, prop) && !isSpecialProperty(prop)) {
if (!isPresent(matchedProperties) || !isPresent(StringMapWrapper.get(matchedProperties, prop))) {
throw new BaseException(`Missing directive to handle '${camelCaseToDashCase(prop)}' in ${current.elementDescription}`);
}
}
});
}
// template only store directives as attribute when they are not bound to expressions
// so we have to validate the expression case too (e.g. !if="condition")
if (isTemplateElement && !current.isViewRoot && !isPresent(current.viewportDirective)) {
throw new BaseException(`Missing directive to handle: ${current.elementDescription}`);
}
}
} }

View File

@ -93,12 +93,16 @@ function styleSetterFactory(styleName:string, stylesuffix:string) {
return setterFn; return setterFn;
} }
// tells if an attribute is handled by the ElementBinderBuilder step const ROLE_ATTR = 'role';
export function isSpecialProperty(propName:string) { function roleSetter(element, value) {
return StringWrapper.startsWith(propName, ATTRIBUTE_PREFIX) if (isString(value)) {
|| StringWrapper.startsWith(propName, CLASS_PREFIX) DOM.setAttribute(element, ROLE_ATTR, value);
|| StringWrapper.startsWith(propName, STYLE_PREFIX) } else {
|| StringMapWrapper.contains(DOM.attrToPropMap, propName); DOM.removeAttribute(element, ROLE_ATTR);
if (isPresent(value)) {
throw new BaseException("Invalid role attribute, only string values are allowed, got '" + stringify(value) + "'");
}
}
} }
/** /**

View File

@ -2,6 +2,7 @@ import {
AsyncTestCompleter, AsyncTestCompleter,
beforeEach, beforeEach,
ddescribe, ddescribe,
xdescribe,
describe, describe,
el, el,
expect, expect,
@ -566,6 +567,8 @@ export function main() {
})); }));
}); });
xdescribe('Missing directive checks', () => {
if (assertionsEnabled()) { if (assertionsEnabled()) {
function expectCompileError(inlineTpl, errMessage, done) { function expectCompileError(inlineTpl, errMessage, done) {
@ -581,14 +584,6 @@ export function main() {
); );
} }
it('should raise an error if no directive is registered for an unsupported DOM property', inject([AsyncTestCompleter], (async) => {
expectCompileError(
'<div [some-prop]="foo"></div>',
'Missing directive to handle \'some-prop\' in MyComp: <div [some-prop]="foo">',
() => async.done()
);
}));
it('should raise an error if no directive is registered for a template with template bindings', inject([AsyncTestCompleter], (async) => { it('should raise an error if no directive is registered for a template with template bindings', inject([AsyncTestCompleter], (async) => {
expectCompileError( expectCompileError(
'<div><div template="if: foo"></div></div>', '<div><div template="if: foo"></div></div>',
@ -622,6 +617,8 @@ export function main() {
})); }));
} }
}); });
});
} }