Skip to content

Commit

Permalink
feat(render): don’t use the reflector for setting properties
Browse files Browse the repository at this point in the history
BREAKING CHANGES:
- host actions don't take an expression as value any more but only a method name,
  and assumes to get an array via the EventEmitter with the method arguments.
- Renderer.setElementProperty does not take `style.`/... prefixes any more.
  Use the new methods `Renderer.setElementAttribute`, ... instead

Part of #2476
Closes #2637
  • Loading branch information
tbosch committed Jun 23, 2015
1 parent 2932377 commit 0a51ccb
Show file tree
Hide file tree
Showing 32 changed files with 642 additions and 567 deletions.
74 changes: 61 additions & 13 deletions modules/angular2/src/change_detection/binding_record.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,13 +5,17 @@ import {DirectiveIndex, DirectiveRecord} from './directive_record';

const DIRECTIVE = "directive";
const DIRECTIVE_LIFECYCLE = "directiveLifecycle";
const ELEMENT = "element";
const ELEMENT_PROPERTY = "elementProperty";
const ELEMENT_ATTRIBUTE = "elementAttribute";
const ELEMENT_CLASS = "elementClass";
const ELEMENT_STYLE = "elementStyle";
const TEXT_NODE = "textNode";

export class BindingRecord {
constructor(public mode: string, public implicitReceiver: any, public ast: AST,
public elementIndex: number, public propertyName: string, public setter: SetterFn,
public lifecycleEvent: string, public directiveRecord: DirectiveRecord) {}
public elementIndex: number, public propertyName: string, public propertyUnit: string,
public setter: SetterFn, public lifecycleEvent: string,
public directiveRecord: DirectiveRecord) {}

callOnChange(): boolean {
return isPresent(this.directiveRecord) && this.directiveRecord.callOnChange;
Expand All @@ -25,41 +29,85 @@ export class BindingRecord {

isDirectiveLifecycle(): boolean { return this.mode === DIRECTIVE_LIFECYCLE; }

isElement(): boolean { return this.mode === ELEMENT; }
isElementProperty(): boolean { return this.mode === ELEMENT_PROPERTY; }

isElementAttribute(): boolean { return this.mode === ELEMENT_ATTRIBUTE; }

isElementClass(): boolean { return this.mode === ELEMENT_CLASS; }

isElementStyle(): boolean { return this.mode === ELEMENT_STYLE; }

isTextNode(): boolean { return this.mode === TEXT_NODE; }

static createForDirective(ast: AST, propertyName: string, setter: SetterFn,
directiveRecord: DirectiveRecord): BindingRecord {
return new BindingRecord(DIRECTIVE, 0, ast, 0, propertyName, setter, null, directiveRecord);
return new BindingRecord(DIRECTIVE, 0, ast, 0, propertyName, null, setter, null,
directiveRecord);
}

static createDirectiveOnCheck(directiveRecord: DirectiveRecord): BindingRecord {
return new BindingRecord(DIRECTIVE_LIFECYCLE, 0, null, 0, null, null, "onCheck",
return new BindingRecord(DIRECTIVE_LIFECYCLE, 0, null, 0, null, null, null, "onCheck",
directiveRecord);
}

static createDirectiveOnInit(directiveRecord: DirectiveRecord): BindingRecord {
return new BindingRecord(DIRECTIVE_LIFECYCLE, 0, null, 0, null, null, "onInit",
return new BindingRecord(DIRECTIVE_LIFECYCLE, 0, null, 0, null, null, null, "onInit",
directiveRecord);
}

static createDirectiveOnChange(directiveRecord: DirectiveRecord): BindingRecord {
return new BindingRecord(DIRECTIVE_LIFECYCLE, 0, null, 0, null, null, "onChange",
return new BindingRecord(DIRECTIVE_LIFECYCLE, 0, null, 0, null, null, null, "onChange",
directiveRecord);
}

static createForElement(ast: AST, elementIndex: number, propertyName: string): BindingRecord {
return new BindingRecord(ELEMENT, 0, ast, elementIndex, propertyName, null, null, null);
static createForElementProperty(ast: AST, elementIndex: number,
propertyName: string): BindingRecord {
return new BindingRecord(ELEMENT_PROPERTY, 0, ast, elementIndex, propertyName, null, null, null,
null);
}

static createForElementAttribute(ast: AST, elementIndex: number,
attributeName: string): BindingRecord {
return new BindingRecord(ELEMENT_ATTRIBUTE, 0, ast, elementIndex, attributeName, null, null,
null, null);
}

static createForElementClass(ast: AST, elementIndex: number, className: string): BindingRecord {
return new BindingRecord(ELEMENT_CLASS, 0, ast, elementIndex, className, null, null, null,
null);
}

static createForElementStyle(ast: AST, elementIndex: number, styleName: string,
unit: string): BindingRecord {
return new BindingRecord(ELEMENT_STYLE, 0, ast, elementIndex, styleName, unit, null, null,
null);
}

static createForHostProperty(directiveIndex: DirectiveIndex, ast: AST,
propertyName: string): BindingRecord {
return new BindingRecord(ELEMENT, directiveIndex, ast, directiveIndex.elementIndex,
propertyName, null, null, null);
return new BindingRecord(ELEMENT_PROPERTY, directiveIndex, ast, directiveIndex.elementIndex,
propertyName, null, null, null, null);
}

static createForHostAttribute(directiveIndex: DirectiveIndex, ast: AST,
attributeName: string): BindingRecord {
return new BindingRecord(ELEMENT_ATTRIBUTE, directiveIndex, ast, directiveIndex.elementIndex,
attributeName, null, null, null, null);
}

static createForHostClass(directiveIndex: DirectiveIndex, ast: AST,
className: string): BindingRecord {
return new BindingRecord(ELEMENT_CLASS, directiveIndex, ast, directiveIndex.elementIndex,
className, null, null, null, null);
}

static createForHostStyle(directiveIndex: DirectiveIndex, ast: AST, styleName: string,
unit: string): BindingRecord {
return new BindingRecord(ELEMENT_STYLE, directiveIndex, ast, directiveIndex.elementIndex,
styleName, unit, null, null, null);
}

static createForTextNode(ast: AST, elementIndex: number): BindingRecord {
return new BindingRecord(TEXT_NODE, 0, ast, elementIndex, null, null, null, null);
return new BindingRecord(TEXT_NODE, 0, ast, elementIndex, null, null, null, null, null);
}
}
4 changes: 2 additions & 2 deletions modules/angular2/src/core/compiler/element_binder.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,9 +6,9 @@ import {List, StringMap} from 'angular2/src/facade/collection';
import * as viewModule from './view';

export class ElementBinder {
// updated later when events are bound
nestedProtoView: viewModule.AppProtoView = null;
// updated later, so we are able to resolve cycles
nestedProtoView: viewModule.AppProtoView = null;
// updated later when events are bound
hostListeners: StringMap<string, Map<number, AST>> = null;

constructor(public index: int, public parent: ElementBinder, public distanceToParent: int,
Expand Down
4 changes: 2 additions & 2 deletions modules/angular2/src/core/compiler/element_injector.ts
Original file line number Diff line number Diff line change
Expand Up @@ -315,13 +315,13 @@ export class EventEmitterAccessor {
}

export class HostActionAccessor {
constructor(public actionExpression: string, public getter: Function) {}
constructor(public methodName: string, public getter: Function) {}

subscribe(view: viewModule.AppView, boundElementIndex: number, directive: Object) {
var eventEmitter = this.getter(directive);
return ObservableWrapper.subscribe(
eventEmitter,
actionObj => view.callAction(boundElementIndex, this.actionExpression, actionObj));
actionArgs => view.invokeElementMethod(boundElementIndex, this.methodName, actionArgs));
}
}

Expand Down
34 changes: 28 additions & 6 deletions modules/angular2/src/core/compiler/proto_view_factory.ts
Original file line number Diff line number Diff line change
Expand Up @@ -66,9 +66,20 @@ class BindingRecordsCreator {

_createElementPropertyRecords(bindings: List<BindingRecord>, boundElementIndex: number,
renderElementBinder: renderApi.ElementBinder) {
MapWrapper.forEach(renderElementBinder.propertyBindings, (astWithSource, propertyName) => {

bindings.push(BindingRecord.createForElement(astWithSource, boundElementIndex, propertyName));
ListWrapper.forEach(renderElementBinder.propertyBindings, (binding) => {
if (binding.type === renderApi.PropertyBindingType.PROPERTY) {
bindings.push(BindingRecord.createForElementProperty(binding.astWithSource,
boundElementIndex, binding.property));
} else if (binding.type === renderApi.PropertyBindingType.ATTRIBUTE) {
bindings.push(BindingRecord.createForElementAttribute(binding.astWithSource,
boundElementIndex, binding.property));
} else if (binding.type === renderApi.PropertyBindingType.CLASS) {
bindings.push(BindingRecord.createForElementClass(binding.astWithSource, boundElementIndex,
binding.property));
} else if (binding.type === renderApi.PropertyBindingType.STYLE) {
bindings.push(BindingRecord.createForElementStyle(binding.astWithSource, boundElementIndex,
binding.property, binding.unit));
}
});
}

Expand Down Expand Up @@ -103,10 +114,21 @@ class BindingRecordsCreator {
for (var i = 0; i < directiveBinders.length; i++) {
var directiveBinder = directiveBinders[i];
// host properties
MapWrapper.forEach(directiveBinder.hostPropertyBindings, (astWithSource, propertyName) => {
ListWrapper.forEach(directiveBinder.hostPropertyBindings, (binding) => {
var dirIndex = new DirectiveIndex(boundElementIndex, i);

bindings.push(BindingRecord.createForHostProperty(dirIndex, astWithSource, propertyName));
if (binding.type === renderApi.PropertyBindingType.PROPERTY) {
bindings.push(BindingRecord.createForHostProperty(dirIndex, binding.astWithSource,
binding.property));
} else if (binding.type === renderApi.PropertyBindingType.ATTRIBUTE) {
bindings.push(BindingRecord.createForHostAttribute(dirIndex, binding.astWithSource,
binding.property));
} else if (binding.type === renderApi.PropertyBindingType.CLASS) {
bindings.push(
BindingRecord.createForHostClass(dirIndex, binding.astWithSource, binding.property));
} else if (binding.type === renderApi.PropertyBindingType.STYLE) {
bindings.push(BindingRecord.createForHostStyle(dirIndex, binding.astWithSource,
binding.property, binding.unit));
}
});
}
}
Expand Down
19 changes: 14 additions & 5 deletions modules/angular2/src/core/compiler/view.ts
Original file line number Diff line number Diff line change
Expand Up @@ -99,11 +99,20 @@ export class AppView implements ChangeDispatcher, EventDispatcher {

// dispatch to element injector or text nodes based on context
notifyOnBinding(b: BindingRecord, currentValue: any): void {
if (b.isElement()) {
if (b.isElementProperty()) {
this.renderer.setElementProperty(this.render, b.elementIndex, b.propertyName, currentValue);
} else {
// we know it refers to _textNodes.
} else if (b.isElementAttribute()) {
this.renderer.setElementAttribute(this.render, b.elementIndex, b.propertyName, currentValue);
} else if (b.isElementClass()) {
this.renderer.setElementClass(this.render, b.elementIndex, b.propertyName, currentValue);
} else if (b.isElementStyle()) {
var unit = isPresent(b.propertyUnit) ? b.propertyUnit : '';
this.renderer.setElementStyle(this.render, b.elementIndex, b.propertyName,
`${currentValue}${unit}`);
} else if (b.isTextNode()) {
this.renderer.setText(this.render, b.elementIndex, currentValue);
} else {
throw new BaseException('Unsupported directive record');
}
}

Expand All @@ -124,8 +133,8 @@ export class AppView implements ChangeDispatcher, EventDispatcher {
return isPresent(childView) ? childView.changeDetector : null;
}

callAction(elementIndex: number, actionExpression: string, action: Object) {
this.renderer.callAction(this.render, elementIndex, actionExpression, action);
invokeElementMethod(elementIndex: number, methodName: string, args: List<any>) {
this.renderer.invokeElementMethod(this.render, elementIndex, methodName, args);
}

// implementation of EventDispatcher#dispatchEvent
Expand Down
24 changes: 20 additions & 4 deletions modules/angular2/src/dom/browser_adapter.dart
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
library angular.core.facade.dom;

import 'dart:html';
import 'dart:js' show JsObject;
import 'dom_adapter.dart' show setRootDomAdapter;
import 'generic_browser_adapter.dart' show GenericBrowserDomAdapter;
import '../facade/browser.dart';
Expand Down Expand Up @@ -97,9 +96,28 @@ final _keyCodeToKeyMap = const {
};

class BrowserDomAdapter extends GenericBrowserDomAdapter {
js.JsFunction _setProperty;
js.JsFunction _getProperty;
js.JsFunction _hasProperty;
BrowserDomAdapter() {
_setProperty = js.context.callMethod('eval', ['(function(el, prop, value) { el[prop] = value; })']);
_getProperty = js.context.callMethod('eval', ['(function(el, prop) { return el[prop]; })']);
_hasProperty = js.context.callMethod('eval', ['(function(el, prop) { return prop in el; })']);
}
static void makeCurrent() {
setRootDomAdapter(new BrowserDomAdapter());
}
bool hasProperty(Element element, String name) =>
_hasProperty.apply([element, name]);

void setProperty(Element element, String name, Object value) =>
_setProperty.apply([element, name, value]);

getProperty(Element element, String name) =>
_getProperty.apply([element, name]);

invoke(Element element, String methodName, List args) =>
this.getProperty(element, methodName).apply(args, thisArg: element);

// TODO(tbosch): move this into a separate environment class once we have it
logError(error) {
Expand All @@ -108,7 +126,7 @@ class BrowserDomAdapter extends GenericBrowserDomAdapter {

@override
Map<String, String> get attrToPropMap => const <String, String>{
'innerHtml': 'innerHtml',
'innerHtml': 'innerHTML',
'readonly': 'readOnly',
'tabindex': 'tabIndex',
};
Expand Down Expand Up @@ -221,8 +239,6 @@ class BrowserDomAdapter extends GenericBrowserDomAdapter {
ShadowRoot getShadowRoot(Element el) => el.shadowRoot;
Element getHost(Element el) => (el as ShadowRoot).host;
clone(Node node) => node.clone(true);
bool hasProperty(Element element, String name) =>
new JsObject.fromBrowserObject(element).hasProperty(name);
List<Node> getElementsByClassName(Element element, String name) =>
element.getElementsByClassName(name);
List<Node> getElementsByTagName(Element element, String name) =>
Expand Down
7 changes: 6 additions & 1 deletion modules/angular2/src/dom/browser_adapter.ts
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,12 @@ var _chromeNumKeyPadMap = {

export class BrowserDomAdapter extends GenericBrowserDomAdapter {
static makeCurrent() { setRootDomAdapter(new BrowserDomAdapter()); }
hasProperty(element, name: string) { return name in element; }
setProperty(el: /*element*/ any, name: string, value: any) { el[name] = value; }
getProperty(el: /*element*/ any, name: string): any { return el[name]; }
invoke(el: /*element*/ any, methodName: string, args: List<any>): any {
el[methodName].apply(el, args);
}

// TODO(tbosch): move this into a separate environment class once we have it
logError(error) { window.console.error(error); }
Expand Down Expand Up @@ -152,7 +158,6 @@ export class BrowserDomAdapter extends GenericBrowserDomAdapter {
getShadowRoot(el: HTMLElement): DocumentFragment { return (<any>el).shadowRoot; }
getHost(el: HTMLElement): HTMLElement { return (<any>el).host; }
clone(node: Node) { return node.cloneNode(true); }
hasProperty(element, name: string) { return name in element; }
getElementsByClassName(element, name: string) { return element.getElementsByClassName(name); }
getElementsByTagName(element, name: string) { return element.getElementsByTagName(name); }
classList(element): List<any> {
Expand Down
6 changes: 5 additions & 1 deletion modules/angular2/src/dom/dom_adapter.ts
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,11 @@ function _abstract() {
* Provides DOM operations in an environment-agnostic way.
*/
export class DomAdapter {
hasProperty(element, name: string): boolean { throw _abstract(); }
setProperty(el: /*element*/ any, name: string, value: any) { throw _abstract(); }
getProperty(el: /*element*/ any, name: string): any { throw _abstract(); }
invoke(el: /*element*/ any, methodName: string, args: List<any>): any { throw _abstract(); }

logError(error) { throw _abstract(); }

/**
Expand Down Expand Up @@ -70,7 +75,6 @@ export class DomAdapter {
getHost(el): any { throw _abstract(); }
getDistributedNodes(el): List<any> { throw _abstract(); }
clone(node): any { throw _abstract(); }
hasProperty(element, name: string): boolean { throw _abstract(); }
getElementsByClassName(element, name: string): List<any> { throw _abstract(); }
getElementsByTagName(element, name: string): List<any> { throw _abstract(); }
classList(element): List<any> { throw _abstract(); }
Expand Down
18 changes: 12 additions & 6 deletions modules/angular2/src/dom/html_adapter.dart
Original file line number Diff line number Diff line change
Expand Up @@ -10,13 +10,24 @@ class Html5LibDomAdapter implements DomAdapter {
setRootDomAdapter(new Html5LibDomAdapter());
}

hasProperty(element, String name) {
// This is needed for serverside compile to generate the right getters/setters...
return true;
}

void setProperty(Element element, String name, Object value) => throw 'not implemented';

getProperty(Element element, String name) => throw 'not implemented';

invoke(Element element, String methodName, List args) => throw 'not implemented';

logError(error) {
stderr.writeln('${error}');
}

@override
final attrToPropMap = const {
'innerHtml': 'innerHtml',
'innerHtml': 'innerHTML',
'readonly': 'readOnly',
'tabindex': 'tabIndex',
};
Expand Down Expand Up @@ -184,11 +195,6 @@ class Html5LibDomAdapter implements DomAdapter {
throw 'not implemented';
}
clone(node) => node.clone(true);

hasProperty(element, String name) {
// This is needed for serverside compile to generate the right getters/setters...
return true;
}
getElementsByClassName(element, String name) {
throw 'not implemented';
}
Expand Down
15 changes: 14 additions & 1 deletion modules/angular2/src/dom/parse5_adapter.ts
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,20 @@ function _notImplemented(methodName) {
export class Parse5DomAdapter extends DomAdapter {
static makeCurrent() { setRootDomAdapter(new Parse5DomAdapter()); }

hasProperty(element, name: string) { return _HTMLElementPropertyList.indexOf(name) > -1; }
// TODO(tbosch): don't even call this method when we run the tests on server side
// by not using the DomRenderer in tests. Keeping this for now to make tests happy...
setProperty(el: /*element*/ any, name: string, value: any) {
if (name === 'innerHTML') {
this.setInnerHTML(el, value);
} else {
el[name] = value;
}
}
// TODO(tbosch): don't even call this method when we run the tests on server side
// by not using the DomRenderer in tests. Keeping this for now to make tests happy...
getProperty(el: /*element*/ any, name: string): any { return el[name]; }

logError(error) { console.error(error); }

get attrToPropMap() { return _attrToPropMap; }
Expand Down Expand Up @@ -268,7 +282,6 @@ export class Parse5DomAdapter extends DomAdapter {
return newParser.parseFragment(serialized).childNodes[0];
}
}
hasProperty(element, name: string) { return _HTMLElementPropertyList.indexOf(name) > -1; }
getElementsByClassName(element, name: string) {
return this.querySelectorAll(element, "." + name);
}
Expand Down
Loading

0 comments on commit 0a51ccb

Please sign in to comment.