feat(BindableProperty): Reflect prop to attribute #560

bigopon · 2017-08-31T02:05:10Z

This PR addresses:

~~Aurelia should support boolean values for attributes. binding#347~~ @AshleyGrant (sorry Ashley 😄 )
Reflecting properties to attributes #283

Support assignment of attribute litterals via interpolation templating-binding#106

Note:

It doesn't support app root component
It doesn't support compose element
It still set the attribute on containerless element
It only has setup test so far
It ignores null / undefined value, so to achieve add / remove attribute behavior, set null / undefined. This is to align with normal behavior of special built-in properties. Another way is to create a common instruction, where it treat empty string / false values as a sign to remove attribute.
Demo

Usage with `bindable` decorator:

class MyComponent {
  @bindable({
    // This instruct it to map prop -> attr 1:1
    // pass null / undefined to remove attribute, otherwise will be converted to string
    reflect: true
  })
  prop1

  @bindable({
    // Alternatively can pass a function as instruction how to reflect
    reflect(element: Element, propertyName: string, newVal, oldVal) {
      element.setAttribute(propertyName, `${oldVal} -- ${newVal}`);
    }
  })
  prop2
}

cc @EisenbergEffect @jdanyow

bigopon · 2017-08-31T02:21:52Z

@AshleyGrant If the original request was to have the ability to reflect the prop to every element bound, not just the custom element itself, maybe it better suits a new binding command attr or a separate bindingBehavior:

<div prop.attr='someThing'></div>

<div prop.bind='someThing & reflectToAttribute'></div>

jods4 · 2017-08-31T21:52:03Z

src/bindable-property.js

@@ -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 | {


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.

jods4 · 2017-08-31T21:53:00Z

src/bindable-property.js

@@ -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 | {
+    defaultBindingMode?: number,


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

jods4 · 2017-08-31T21:54:46Z

src/bindable-property.js

-  constructor(nameOrConfig: string | Object) {
+  constructor(nameOrConfig: string | {
+    defaultBindingMode?: number,
+    reflect?: boolean | {(el: Element, name: string, newVal, oldVal): any},


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.

jods4 · 2017-08-31T21:57:16Z

src/bindable-property.js

-  constructor(nameOrConfig: string | Object) {
+  constructor(nameOrConfig: string | {
+    defaultBindingMode?: number,
+    reflect?: boolean | {(el: Element, name: string, newVal, oldVal): any},


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.

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

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.

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

jods4 · 2017-08-31T22:00:06Z

src/bindable-property.js

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

    return undefined;
  }
+
+  _configureReflection(target) {
+    if (target.__reflectionConfigured__) return;


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?

Some thing I couldn't think of. Can only 👍

jods4 · 2017-08-31T22:03:01Z

src/bindable-property.js

+    if (hasHandler) { // avoid ternary to make it consisten with the rest ?
+      alteredHandler = function propertyChanged(name, newVal, oldVal) {
+        onChanged.call(this, name, newVal, oldVal);
+        let { __element__, __reflections__ } = this.__observers__;


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?

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 ?

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.

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

AshleyGrant · 2017-08-31T22:04:32Z

Great comments from @jods4!

jods4 · 2017-08-31T22:05:25Z

src/bindable-property.js

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


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.

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.

jods4 · 2017-08-31T22:06:34Z

src/bindable-property.js

+ * @param {any} newValue 
+ */
+function propToAttr(element, propertyName, newValue) {
+  if (newValue == null) {


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

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

jods4 · 2017-08-31T22:10:48Z

src/bindable-property.js

+ * @param {string} propertyName 
+ * @param {any} newValue 
+ */
+function propToAttr(element, propertyName, newValue) {


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.

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.

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

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

jods4 · 2017-08-31T22:12:56Z

src/html-behavior.js

+    if (propertyName in reflections) {
+      throw new Error(`Reflection for ${propertyName} was already registered`);
+    }
+    if (typeof instruction !== 'function') {


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?

It should be removed.

jods4 · 2017-08-31T22:14:22Z

src/html-behavior.js

+  }
+
+  /**
+   * @param {Element} element 


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

jods4 · 2017-08-31T22:21:52Z

I had a quick look at the code. Not enough to validate the PR but the main mechanic looks good to me.

I am a little confused when you say it addresses aurelia/binding#347. What/how does it do that, I think I missed something?
Notably, one thing I really would like to be able to do is:

<input magical-bool-binding />

As described in this comment aurelia/binding#347 (comment)

bigopon · 2017-08-31T23:02:54Z

Thanks @jods4 , throughout as expected.

I understood of the original requirement was to reflect Aurelia custom element prop to attribute, so it needs some signal to know when the attribute should go. I was trying to do this by associating null / undefined value with the removal.

But then your comments make me I realise that if we are to support JS typing, and reflect a boolean property to prop, then we can provide two default reflecting behavior: 1:1 and boolean. Then by the setup time, bindable property will pickup the metadata and decide which to go. What you think ?
I'm still not sure how you support flexibility here if we don't allow reflectToAttribute to be a function. If you could point out a solution, it would be great.

jods4 · 2017-08-31T23:14:48Z

The thing about aurelia/binding#347 is that it's not just about updating attributes when the property changes.

It's also about having a shortcut notation where <div x> is shortcut for <div x.one-time="true">.

I think I don't fully grasp what use cases you want to address by allowing reflectToAttribute to be a function. Could you please provide a concrete example for the sake of discussion?

bigopon · 2017-08-31T23:39:14Z

I totally missed that point of original request.

I don't have a use case for reflectToAttribute as a function. The motivation behind was just to have the flexibility when setting the value, as it's not always 1:1 prop->attr. Maybe bindable config could use another property for value conversion ?

Edit 1 Not 1:1 always is for the case of object,toString, it will become [object Object] on the attribute
Edit 2: ~~I've found a use case for it, described here aurelia/templating-binding#106. Changing the current requires to reflect the property value to both aria-min & aria-max. Is it a valid one @jods4 ?~~ Sorry it fits changeHandler better

jods4 · 2017-09-03T11:24:37Z

@bigopon let's see what others think of the proposed function reflectToAttribute.

My opinion is that we shouldn't introduce complexity unless it has strong pay-off.

In this case:

We don't have a good, concrete use case for it.
It provides practically the same service as existing changeHandler.

So my vote would be to wait. Ship reflectToAttribute: boolean in v1 and add other options based on community feedback.

jods4 · 2017-09-03T11:26:54Z

src/bindable-property.js

+ * @param newValue
+ * @param oldValue
+ */
+function callRefelection(instance: Object, propertyName: string, newValue, oldValue) {


Typo here: Refelection

jods4 · 2017-09-03T11:29:42Z

src/bindable-property.js

-  if (newValue == null) {
-    element.removeAttribute(propertyName);
+function propToAttr(element: Element, propertyName: string, newValue: any) {
+  if (newValue === null || newValue === void 0) {


OK I might be nitpicking but I prefer newValue === undefined, for clarity.
Minifiers will do the job of using the smallest code.

I know users could redefine undefined but a lot of other parts won't work if they do.

I was trying to do minifier job, will avoid next PRs 👍

jods4 · 2017-09-03T11:34:02Z

src/bindable-property.js

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


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?

jods4 · 2017-09-03T11:36:56Z

So this doesn't play nice with lifted custom elements (@containerless), because they don't have a target element to reflect to.
Could we detect this situation and log an error in the console?
Should only be on actual usage, I'm thinking of weird composition / inheritance cases.

bigopon · 2017-09-03T12:39:38Z

@jods4 Thanks for your review, and I'm thinking of closing this PR. since it doesn't really add much value, like you suggested. (Wonder if you have thought of any scenarios this could be useful beside saving few lines of code).

And after realized I failed to address Ashley's case making me agree with you more. I feel like there is no point adding this once, as it is a bit complex to deal with inheritance and if anyone ever wanted this, it would just be a simple injection and reflect in valueChanged handler.

But I'd like to add that about your point of complex function as a value for reflectToAttribute, you are correct, maybe it should have the same signature with valueConverters, take newValue, oldValue and return something, then that will be mapped to the element, privately by the framework.

Maybe @EisenbergEffect can put a some thoughts before we close this PR ?

jods4 · 2017-09-03T16:16:46Z

@bigopon I don't know why I gave you the impression that this PR "doesn't really add much value". I honestly never intended that.

I personally think that the function parameter to reflectToAttribute adds too much burden for its value.

But as for reflectToAttribute: boolean it closes #283 so there's value in that.

I also like the extra typings that you put in this PR. It makes it much easier to work with Aurelia code. ❤️

jdanyow · 2017-09-04T15:26:20Z

I think an attr command would be more generally applicable and easier for folks to use.
aurelia/templating-binding#106 (comment)

Before this is closed, are there any use-cases or pain points that this @bindable({ reflect: true }) approach solves that can't be solved with foo.attr="something" ?

EisenbergEffect · 2017-09-04T17:44:17Z

Part of this addresses the issue with making bindable properties have the same capability as built-in elements, the potential to automatically reflect property changes to attributes. This includes boolean attributes (bindables that add/remove the attribute from the element altogether), which we need for the UX library.

If we add the attr command, that would need to work with bindable properties, since there's little reason to create a bindable where you don't want the property to be updated. So, it's not just an attr update in this case.

Beyond that, attr reflection should be independent of one-way or two-way capability. I'm not sure how this would be handled, even though it's a rare case.

jods4 · 2017-09-04T19:10:05Z

Also: this feature is copied from Polymer.
Polymer is meant for building/using web components and as Rob said, it's kind of standard for elements (built-in or custom) to reflect (some of) their properties into attributes.

jdanyow · 2017-09-05T02:26:14Z

Apologies- when I was reading the discussion I got the impression that @bigopon was closing this.

Still not clear on the use-case though. I see polymer has it, what's the purpose in the aurelia context? To enable targeting states with css attribute selectors? What's the aurelia-ux use-case?

EisenbergEffect · 2017-09-05T03:21:06Z

Yes, that's definitely one of them.

bigopon · 2017-09-05T03:47:18Z

I was thinking of it, as I'm not sure if it's worth the lines. @jdanyow

Also I was concerned by the fact that there may be confusion in the bindable, if we support attribute property in bindable config, as some will try to use attribute as some sort of how to reflect it back to the element. While some others will not touch it and think it as an instruction for custom attribute only.

Maybe we can have

type ReflectInstruction = string | {
  attr: string,
  converter: (newValue, oldValue) => any
}

reflectToAttribute: ReflectInstruction

bigopon · 2017-09-10T01:28:32Z

I've followed @jods4 's suggestion.

  // also support (element, attrName, value): any
  // the reason is object -> [object Object] when calling toString
  // Or should it just be gone ?
  @bindable({
    reflectToAttribute: 'boolean' | 'string', 
    attribute: 'given-name' // this means prop firstName will be reflected to given-name
  })
  firstName

jdanyow · 2017-09-23T22:07:48Z

Does this work in both directions? If someone calls setAttribute, will the associated bindable property update?

bigopon · 2017-09-23T23:11:44Z

Is it this feature request #458 you are referencing ? It's would be nice to have, but @EisenbergEffect didn't give any comment about resolution. I think you commented somewhere in some related issue about using MutationObserver for that.

bigopon added 7 commits August 31, 2017 02:12

feat(BindableProperty): Prop to attribute

eab77c9

Fix: Add back types for Bindable property constructor

bd63798

Fix method typings

006b799

Fix: Ignore null/ void by default

7f8770e

Fix: Ensure reflect each VM Class once

8c0d718

Fix param name! ...

7629605

Fix: Initiate reflection on each instance

ab7dab4

jods4 reviewed Aug 31, 2017

View reviewed changes

Add bindable typing, extract refelection to separate fn

746f194

jods4 reviewed Sep 3, 2017

View reviewed changes

move reflection to selfSubscriber, test to bindable property

47987e0

bigopon mentioned this pull request Oct 18, 2017

Aurelia should support boolean values for attributes. aurelia/binding#347

Closed

bigopon mentioned this pull request Dec 11, 2017

WIP feat(bindable): support coercing value #558

Closed

3 tasks

aluanhaddad mentioned this pull request Feb 13, 2018

feat(interfaces): define and specify a well typed configuration object #601

Open

bigopon mentioned this pull request Mar 31, 2018

Further enhancements aurelia-contrib/aurelia-typed-observable-plugin#1

Open

feat(BindableProperty): Reflect prop to attribute #560

Are you sure you want to change the base?

feat(BindableProperty): Reflect prop to attribute #560

Conversation

bigopon commented Aug 31, 2017 • edited Loading

Usage with bindable decorator:

bigopon commented Aug 31, 2017 • edited Loading

jods4 Aug 31, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jods4 Aug 31, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jods4 Aug 31, 2017 • edited Loading

Choose a reason for hiding this comment

bigopon Sep 1, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AshleyGrant commented Aug 31, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jods4 commented Aug 31, 2017

bigopon commented Aug 31, 2017 • edited Loading

jods4 commented Aug 31, 2017

bigopon commented Aug 31, 2017 • edited Loading

jods4 commented Sep 3, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jods4 commented Sep 3, 2017

bigopon commented Sep 3, 2017

jods4 commented Sep 3, 2017

jdanyow commented Sep 4, 2017

EisenbergEffect commented Sep 4, 2017

jods4 commented Sep 4, 2017

jdanyow commented Sep 5, 2017

EisenbergEffect commented Sep 5, 2017

bigopon commented Sep 5, 2017

bigopon commented Sep 10, 2017 • edited Loading

jdanyow commented Sep 23, 2017

bigopon commented Sep 23, 2017 • edited Loading

bigopon commented Aug 31, 2017 •

edited

Loading

Usage with `bindable` decorator:

bigopon commented Aug 31, 2017 •

edited

Loading

jods4 Aug 31, 2017 •

edited

Loading

jods4 Aug 31, 2017 •

edited

Loading

jods4 Aug 31, 2017 •

edited

Loading

bigopon Sep 1, 2017 •

edited

Loading

bigopon commented Aug 31, 2017 •

edited

Loading

bigopon commented Aug 31, 2017 •

edited

Loading

jods4 commented Sep 3, 2017 •

edited

Loading

bigopon commented Sep 10, 2017 •

edited

Loading

bigopon commented Sep 23, 2017 •

edited

Loading