Skip to content
Open
Show file tree
Hide file tree
Changes from 7 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
61 changes: 60 additions & 1 deletion src/bindable-property.js
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,13 @@ export class BindableProperty {
* Creates an instance of BindableProperty.
* @param nameOrConfig The name of the property or a cofiguration object.
*/
constructor(nameOrConfig: string | Object) {
constructor(nameOrConfig: string | {

@jods4 jods4 Aug 31, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for typing that Object!
Should we have a named interface for that rather? BindableOptions? That would make the code cleaner + give a place to comment on members... I know what attribute does but it's not totally obvious, especially now with reflect in the mix.

defaultBindingMode?: number,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

number :(
Don't we have an enum to match? Can we create one?

reflect?: boolean | {(el: Element, name: string, newVal, oldVal): any},

@jods4 jods4 Aug 31, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is bike-shedding but I'm not a fan of the name.
reflect is a little too generic for my taste, makes me think of Reflect first.
Polymer uses, reflectToAttribute, maybe we should play along? If someone sees this pop up in IntelliSense, its meaning is somewhat clear. reflect not as much.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not convinced by the API design of the function part.
It feels like it takes too much... the element and old value and a name (of what)?

This is meant to just reflect a value in an attribute. The callback here really feels like changeHandler. Accepting a bool | string with the string allowing to reflect to a different attribute name seems simpler.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we restricted it to only choose the name, whjat would we do if it is not always 1:1 prop->attr ?

@jods4 jods4 Aug 31, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you mean like one binding updates 2 attributes? For example condition boolean property is reflected into the presence of either a true or a false attribute on target element?

Personally I would say this is narrow use cases that can be filled today by using changeHandler, which has practically the same signature.

Making the API surface larger and more complex means a bigger Aurelia, more docs and more stuff to support and maintain in the future.

@bigopon bigopon Sep 1, 2017

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's the case where value of property is an object, we dont want [object Object] in the attribute.

Another motivation, is that pretty much everything in Aurelia is intercept-able, So I would choose go with the rest on this.

But we can limit it by accepting a function that will take value input an return a string, instead of passing in the whole 4 params like what I did. Can call it serializer

name?: string,
attribute?: any,
changeHandler?: string
}) {
if (typeof nameOrConfig === 'string') {
this.name = nameOrConfig;
} else {
Expand All @@ -58,17 +64,55 @@ export class BindableProperty {
* @param descriptor The property descriptor for this property.
*/
registerWith(target: Function, behavior: HtmlBehaviorResource, descriptor?: Object): void {
let { reflect } = this;
behavior.properties.push(this);
behavior.attributes[this.attribute] = this;
this.owner = behavior;

if (reflect) {
behavior._registerReflection(this.name, typeof reflect === 'function' ? reflect : propToAttr);
this._configureReflection(target);
}

if (descriptor) {
this.descriptor = descriptor;
return this._configureDescriptor(descriptor);
}

return undefined;
}

_configureReflection(target) {
if (target.__reflectionConfigured__) return;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Aurelia includes a Symbol polyfill.
Is the __xxx__ field how we want to roll when hacking into objects we don't own? Shouldn't we use symbols instead?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some thing I couldn't think of. Can only 馃憤


let method = 'propertyChanged';
let onChanged = target.prototype[method];
let hasHandler = !!onChanged;

let alteredHandler;
if (hasHandler) { // avoid ternary to make it consisten with the rest ?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought about this part some more...
It does something very similar to createObserver (same file).

Wouldn't it be more efficient and more clear to integrate callReflection inside createObserver?

alteredHandler = function propertyChanged(name, newVal, oldVal) {
onChanged.call(this, name, newVal, oldVal);
let { __element__, __reflections__ } = this.__observers__;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure, so just asking: is accessing __observers__ directly the way to go?
Isn't that an implementation detail of observation and an API should be used here rather?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This one is really silly of mine, was too hasty to see this. 馃憤
How would you fix this though, there is already getObserver in the scope, in which it access __observers__ directly. Maybe i should create getObserverLookup and point getObserver there. Or just leave getObserver alone to avoid performance cost of fn calling ?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did not see that getObserver function. I think the right thing to do is to use it to not break the abstraction level here.

If you want to maximize perfs and make sure getObserver gets optimized by JIT you can split it in two so that it is small and only contains the happy path (lookup !== undefined) and put the initialization code in a second function.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I will move the whole block of reflection out of that function, to a separate fn

if (!__element__) return;
__reflections__[name].call(this, __element__, name, newVal, oldVal)
}
} else {
alteredHandler = function propertyChanged(name, newVal, oldVal) {
let { __element__, __reflections__ } = this.__observers__;
if (!__element__) return;
__reflections__[name].call(this, __element__, name, newVal, oldVal);
}
}

if (!Reflect.defineProperty(target.prototype, method, {
configurable: true,
value: alteredHandler
})) {
throw new Error(`Cannot setup property reflection on <${this.name}/> for ${target.name}`);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we go for throw on failure, why don't use Object.defineProperty. Unlike Reflect it exists on older browsers, does the same thing, and throws when it fails.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is because my lack of knowledge about compiler result. I didn't know if 'use strict' will always be there so i tried to do something.

};
target.__reflectionConfigured__ = true;
}

_configureDescriptor(descriptor: Object): Object {
let name = this.name;
Expand Down Expand Up @@ -248,3 +292,18 @@ export class BindableProperty {
observer.selfSubscriber = selfSubscriber;
}
}

/**
* @private
* Used for avoid creating mapping function multiple times
* @param {Element} element
* @param {string} propertyName
* @param {any} newValue
*/
function propToAttr(element, propertyName, newValue) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems to me this doesn't take into account the camelCase to kebab-case between properties and html attributes?

Also in HTML a few weird properties do not map directly to same-name attribute. I think we have a kind of map somewhere in Aurelia that converts some of those so that it "just work" when binding HTML attributes, shouldn't we re-use it here so that it just works for those?

Should this API interact with the attribute binding option? It allows users to map a property to a different name attribute, wouldn't they expect reflectToAttribute to take that into account? Seems more intuitive.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No camelCase to kebab-case is a bad mistake. It should be fixed.

Code for the special pairs belong to templating-binding, should I copy it over ? I'm not sure how to handle this, because props of custom element in Aurelia, donot necessarily map to attribute like built-in props.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are right. This is for custom attributes, it has nothing to do with built-in HTML attributes.
Forget my remark about the map 鈽猴笍

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the attribute, it sounds really good. Maybe i'll go for it and update the PR doc.

if (newValue == null) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like this, but I think the rest of the team is linting for strict === always.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will be fixed. Was trying to roll it out so a bit lazy for || and === 馃槃

element.removeAttribute(propertyName);
} else {
element.setAttribute(propertyName, newValue);
}
}
31 changes: 31 additions & 0 deletions src/html-behavior.js
Original file line number Diff line number Diff line change
Expand Up @@ -330,6 +330,10 @@ export class HtmlBehaviorResource {
let childBindings = this.childBindings;
let viewFactory;

if (element !== null) {
this._setupReflections(element, viewModel);
}

if (this.liftsContent) {
//template controller
au.controller = controller;
Expand Down Expand Up @@ -401,6 +405,33 @@ export class HtmlBehaviorResource {
return controller;
}

/**
* Allow a bindable property on custom element to register how to reflect prop value to attribute
* @param {string} propertyName
* @param {{(element: Element, name: string, newVal, oldVal) => any}} instruction A function with suitable parameters to react for setting attribute on the element
*/
_registerReflection(propertyName, instruction) {
let reflections = this.reflections || (this.reflections = {});
if (propertyName in reflections) {
throw new Error(`Reflection for ${propertyName} was already registered`);
}
if (typeof instruction !== 'function') {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't parameter validation go first, before mutating this?

Judging by the _ prefix, this is a private API, do we validate parameters on private APIs?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It should be removed.

throw new Error('Invalid reflection instruction');
}
reflections[propertyName] = instruction;
}

/**
* @param {Element} element

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why use Typescript-like type annotations in bindable-property.js and JSDoc comments here?

* @param {object} viewModel
*/
_setupReflections(element, viewModel) {
if (!this.reflections) return;
let lookup = this.observerLocator.getOrCreateObserversLookup(viewModel);
lookup.__reflections__ = this.reflections;
lookup.__element__ = element;
}

_ensurePropertiesDefined(instance: Object, lookup: Object) {
let properties;
let i;
Expand Down
5 changes: 5 additions & 0 deletions src/view-factory.js
Original file line number Diff line number Diff line change
Expand Up @@ -104,6 +104,10 @@ function setAttribute(name, value) {
this._element.setAttribute(name, value);
}

function removeAttribute(name) {
this._element.removeAttribute(name);
}

function makeElementIntoAnchor(element, elementInstruction) {
let anchor = DOM.createComment('anchor');

Expand All @@ -119,6 +123,7 @@ function makeElementIntoAnchor(element, elementInstruction) {
anchor.hasAttribute = hasAttribute;
anchor.getAttribute = getAttribute;
anchor.setAttribute = setAttribute;
anchor.removeAttribute = removeAttribute;
}

DOM.replaceNode(anchor, element);
Expand Down
23 changes: 23 additions & 0 deletions test/html-behavior.spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ import {Container} from 'aurelia-dependency-injection';
import {ObserverLocator, bindingMode} from 'aurelia-binding';
import {TaskQueue} from 'aurelia-task-queue';
import {HtmlBehaviorResource} from '../src/html-behavior';
import {BindableProperty} from '../src/bindable-property';
import {ViewResources} from '../src/view-resources';

describe('html-behavior', () => {
Expand Down Expand Up @@ -58,4 +59,26 @@ describe('html-behavior', () => {

expect(resource.attributes['test'].defaultBindingMode).toBe(bindingMode.twoWay);
});

describe('Prop to attribute reflection', () => {
it('should have reflections when bindable properties are registered with `reflect`', () => {
let resource = new HtmlBehaviorResource();
let Target = class {};

var prop1 = new BindableProperty({
reflect: true,
name: 'prop1'
});
prop1.registerWith(Target, resource);

var prop2 = new BindableProperty({
reflect() {},
name: 'prop2'
});
prop2.registerWith(Target, resource);

expect(typeof resource.reflections.prop1).toBe('function');
expect(resource.reflections.prop2).toBe(prop2.reflect);
});
});
});