feat(BindableProperty): Reflect prop to attribute

[bigopon]

This PR addresses:

• ~~aurelia/binding#347~~ @AshleyGrant (sorry Ashley 😄 )
• aurelia/templating#283

Related:

• aurelia/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]

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

[AshleyGrant]

Great comments from @jods4!

[jods4]

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 https://github.com/aurelia/binding/issues/347#issuecomment-242106920

[bigopon]

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]

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]

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]

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

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]

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

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

I think an attr command would be more generally applicable and easier for folks to use. https://github.com/aurelia/templating-binding/issues/106#issuecomment-302850701

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]

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]

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]

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]

Yes, that's definitely one of them.

[bigopon]

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]

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]

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

[bigopon]

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.