Inherited bindable properties in custom elements

[jods4]

Overview

This PR adds support for @bindable properties inherited from a base class in a custom element.
This has been asked for several times, the open issue for that being #463.

The way HtmlBehaviorResource is used by Aurelia is unfortunately not inheritance-friendly. For this reason, this PR is very narrow in scope:

• It only adds support for @bindable properties. No other aspect from the base HtmlBehaviorResource is propagated to descendant classes.
• It only applies to custom elements. Custom attributes could easily share the same code, but we would need to define how the default value property would works precisely.

Additionally, the following restrictions apply:

• The derived classes need their own HtmlBehaviorResource metadata. So be sure to have at least one @bindable or @customElement or something like that on the derived class. Note that @useView is not stored there.
• You need an ES6 compliant inheritance structure. In particular, it currently doesn't work with ES5 code generated from Typescript, see this comment for details and workarounds. See also my constructor remark in the review section below.

Usage

Here's a trivial example of what is now possible. A base class provides some common behavior, with exposed bindable properties and two descendant classes create actual custom elements with some customizations.

calculator.ts

import { bindable, useView, customElement } from 'aurelia-framework';

class CalculatorCustomElement {
  @bindable({ changeHandler: 'compute' })
  x = 0;
  @bindable({ changeHandler: 'compute' })
  y = 0;  
  result: number;
}

@useView('calculator.html')
export class AddCustomElement extends CalculatorCustomElement { 
  @bindable whatever;
  
  compute() {
    this.result = this.x + this.y;
  }
}

@customElement('multiply')  // required to give the class its own behavior
@useView('calculator.html')
export class MultiplyCustomElement extends CalculatorCustomElement {
  compute() {
    this.result = this.x * this.y;
  }
}

calculator.html

<template> = ${result}</template>

app.html

<template>
  <div>
    <p>2 + 3 <add x.bind=2 y.bind=3></add></p>
    <p>2 * 3 <multiply x.bind=2 y.bind=3></multiply></p>
  </div>
</template>

Tests

I included new tests to demonstrate the changes in metadata.

That said, this is clearly not enough to check that everything works properly in practice and in all browsers, as the restrictions described above show.

I tested that the sample above works properly in ES6, and TypeScript ES5 (with patched __extends copied from Babel).

Review

The changes are small but this is a tricky part of Aurelia. To make reviewing easier I split the implementation and dist in two commits.

For the most part, the changes have no impact on code that doesn't use inheritance.
For people who already use inheritance by repeating the @bindable metadata, this is supported (last declaration wins).

And then there's bindable-property.js, which you'll want to review carefully.
I had to change the way getObserver install itself and especially how it finds the correct behavior. That was one part that was clearly not inheritance friendly.
Previously it found its behavior by capturing it and passing it around. I am now doing it by looking at metadata.
This is all fine, but it assumes prototype.constructor is properly set. This is the case with ES6, Typescript or Babel generated code. I don't know if Aurelia supports people writing plain ES5 for everything and not properly setting prototype.constructor. That kind of code would break.

That's all folks! 🎉

/cc @EisenbergEffect @jdanyow

[jods4]

It looks my antique JS-fu is not as good as it used to be. prototype.constructor is not a new thing and should always be set right.

function Frob() { }
var x = new Frob();
x.constructor; // Frob
Object.getPrototypeOf(x).constructor; // Frob

So I guess everything is fine 🙂

[RWOverdijk]

Boop.

[RWOverdijk]

Re-boop. Is this still being worked on? This would greatly reduce code and maintenance in our codebase as we now have to copy and maintain all attributes for similar components (for instance our form plugin)

[jods4]

I'm not working on it because it's done. Waiting for @EisenbergEffect to decide if he wants to merge that.

[RWOverdijk]

@jods4 Thanks. I guess I'm at the mercy of @EisenbergEffect then :)

[EisenbergEffect]

It needs a solid code review and unfortunately I've had to spend time on other areas of the framework recently. Maybe @jdanyow can review?

…

On Feb 21, 2017 2:57 AM, "Wesley Overdijk" ***@***.***> wrote: @jods4 <https://github.com/jods4> Thanks. I guess I'm at the mercy of @EisenbergEffect <https://github.com/EisenbergEffect> then :) — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#507 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAIBnXHDtez9qjcce9iwJ8x9elvns6jcks5resMjgaJpZM4LP91D> .

[atsu85]

@jods4, thanks for the PR, I'm not an expert on internals of Aurelia to do proper CR, but i added one comment

[atsu85]

@jods4, i think You should revert all changes to generated code (last time i checked, generating code was part of release process)

[jods4]

Yes, when I opened this PR two months ago I did not know what the policy for dist folder was. I updated the PR to remove those files.

[icegnome]

Did this get merged in? I have a whole slew of CustomElements extending a base element that aren't playing nice in 0.26.1

[jods4]

@icegnome nope. As per the status of this PR and the comments above it's still awating review.

[EisenbergEffect]

It's my fault really. I'm just a little hesitant due to the caveats. @jods4 Maybe you and I can do a quick meeting some time to discuss in detail.

[jods4]

@EisenbergEffect we can. Note that the main caveat was TS inheritance not working in ES5 but this has been fixed in TS 2.2.

[EisenbergEffect]

It's been fixed! Nice.

[jods4]

See microsoft/TypeScript#12488, I'm really glad this one finally landed. 🎉
It's been a long time that I used my own __extend to work around this.

[Spontifixus]

After about half a year I reviewed my old StackOverflow-question regarding this topic: http://stackoverflow.com/questions/38607571/how-to-inherit-bindable-properties-in-aurelia

I am also one of those desperately looking for inheriting the bindable properties.

@jods4 Is there any chance that you publish this "fix" as an addon that can override the default behavior?

[jods4]

@Spontifixus I'm sorry this is rather deep inside Aurelia, I don't think I can cleanly extract it.

In the meantime, if you don't want to repeat each @bindable property in your derived class, I would suggest that you create another decorator that does this automatically:

class ParentResource {
  @bindable propA;
  @bindable propB;
}

@extendBehavior(ParentResource)
class DerivedResource extends ParentResource {
  @bindable propC;
}

This is rather neat and shouldn't be hard to do.

[Spontifixus]

@jods4 Thank you for the quick reply! Creating a decorator should not be that hard, but do you have a tipp for me on how to extend a class with decorated properties?

[Spontifixus]

Has this PR been merged in the meantime? I have tried the following:

1. I created a simple aurelia app with ASP.NET Core using dotnet new aurelia.

2. I added an abstract class named Parent:

    export abstract class Parent {
        @bindable property1: string;
   
        @computedFrom('property1')
        get otherProperty() :string {
            return this.property1.toLocaleUpperCase();
        }
    }
   

3. I created a class named Child, that extends Parent:

    export class Child extends Parent {
        @computedFrom('property1')
        get otherProperty() :string {
           return this.property1.toLocaleLowerCase();
        }
    }
   

4. I created a simple template for the Child-component that just displays the value of the otherProperty:

    <template>
        <h3>${otherProperty}</h3>
    </template>
   

5. In the home-view of the application I added both an input field and the Child-component, and bound the value of the input field to the property1-property which the Child-component inherited from the Parent-component.

    <input type="text" value.two-way="someValue" />
    <child property1.bind="someValue"></child>
   

6. It just worked.

Is this intended or is this just one of the glitches @EisenbergEffect mentioned above?

[jods4]

Sure!
My idea was to do in the decorator what my _copyInheritedProperties does.

You don't need to walk the inheritance chain and all that shit as you get the class to copy from in a parameter.

Then you can get it's metadata with help from aurelia-metadata. I do it right here.

Then you can look at its metadata properties and for each one, you can call the bindable decorator on your decorated class (that's slightly different than what I do in _copyInheritedProperties -- different context).

Doing so should be the automatic equivalent of re-declaring and decorating every property in the derived class.

[jods4]

That might be the TS glitch in action.
Are you compiling your classes to ES5 with TS before 2.2, or if using tslib, are you using helpers from before 2.2?

[Spontifixus]

I am using the following setup: Work is done by Webpack (2.1.0-beta.25) with ts-loader (^0.8.2), using Typescript ^2.2.1. The tsconfig.json looks as follows:

{
    "compilerOptions": {
        "moduleResolution": "node",
        "target": "es5",
        "sourceMap": true,
        "experimentalDecorators": true,
        "emitDecoratorMetadata": true,
        "skipDefaultLibCheck": true,
        "lib": [ "es6", "dom" ],
        "types": [ "node" ]
    },
    "exclude": [ "bin", "node_modules" ],
    "atom": { "rewriteTsconfig": false }
}

[jods4]

@Spontifixus I'm not sure, it's strange. Maybe it's the @computedFrom('property1') that creates metadata for property1.

That seems unlikely, though. Can you check your __extends implementation?
It should look like this:
https://github.com/Microsoft/tslib/blob/master/tslib.js#L51

[Spontifixus]

@jods4 where do I find that? There is no tslib to be seen anywhere near that project. All typescript related things I see are the typescript package and the ts-loader package.

(The binding to property1 works even if I completely remove the computed properties.)

[jods4]

Should be in your output JS code somewhere (top of file normally)

[Spontifixus]

Took me awhile to find it - because I looked in the wrong file. But eventually I found it. It looks exactly as in the code you referenced.

[Spontifixus]

@jods4 I am a bit stuck right now in applying your code to what I have intended. I now went for extending the class using decorators instead of inheriting them. I cannot however figure out how to use the BindableProperty class correctly. There is a StackOverflow-question regarding this by another user. Could you have a look there? http://stackoverflow.com/questions/43918811/how-to-add-bindable-attributes-or-any-other-decorators-to-a-typescript-class-via

[jods4]

@Spontifixus sorry I wanted to have a look at this again but just could not find the time to.
I have a feeling that since @EisenbergEffect is reluctant to merge that code, the decorator way might be the next best thing and easier to get merged (no impact on existing code).

Can you post your @extendBehavior implementation inspired by my comment above somewhere, as well as a short repro of the problem you experience with it? I'll try to have a look (no promise, though).

[tjad]

@jods4 I have a base component which offers a generic view that is configurable. I'd like to extend the component with hard coded configurations in a minimalistic manner, inheritance suits this approach. On the contrary, had I used decorators to do the job, I'd still have to replicate the @bindable definitions as well as make an implementation class(ViewModel) that gets decorated that I could pull into a View. Composition by properties could work, though I'd have to include the property path when binding (assuming the following would work my.composed.property.bind="hello bindable"), otherwise I would have to remap the bindings from the class intended to use inheritance to the class being composed.

[Spontifixus]

@jods4 Thank you for the reply. I have a whole bunch of other stuff to do at the moment, so I am not continuously working on this. At the moment I have no working copy at hand, but it seems getting the DI container working in a decorator is far from trivial, as @EisenbergEffect stated here.

But as I understand from the code you reference the DI container is essential in getting the behavior initialized which I need for both the defineOn or the registerWith method of the BindableProperty - or am I wrong here?

[tjad]

@jods4 @Spontifixus @EisenbergEffect

I have reviewed your PR and find it quite minimal as an addition to the framework (and the steps are well documented). I have further used this PR in conjunction with HEAD (master), and it seems to be running flawlessly.

@EisenbergEffect I would say this is a good solution for inheritance with binding, but you may have bigger intentions for fully inheritable components?

Thanks a lot @jods4!

UPDATE:
@useview, @containerless are probably other annotations that could be considered as quite useful for inheritance, (@Inject works, and with this PR @bindable works)

[cornillemichiel]

+1 for merging this

[jaanek]

Hello. I have stumbled into an issue designing my Aurelia application that partially seems to relate to this pull request (which seems to solve my issue when merged). I have created a stackoverflow question explaining my situation. It seems that having this pull request resolved will allow me to design my Aurelia application the way I planned. My stack overflow question - https://stackoverflow.com/questions/44680903/aurelia-issue-with-using-a-shared-class-specification-for-properties-in-parent

+1 for merging this

[barefootdeveloper]

I would also really like to see this merged, inheritance appears to work until you have multiple things inheriting from it, which of course is the whole point :) Definitely burned a few hours on it.

[tjad]

@EisenbergEffect, @jods4, @Spontifixus Re: inheritance of particular annotations strategy, would it be possible to add some meta data to the annotation definition that the framework could handle all these annotations(@useview, @viewResources, @containerless, @bindable etc) in a similar manner to this PR. Perhaps the user could override this metadata setting on startup if they opt not to have the inheritance (though I think this would be a rare case)

Otherwise the option for creating "Inhertiable components" would provide for micro optimizations.

[jods4]

@tjad
A first step that would really help the common scenario is merging this PR.

We can then think about demand for more inheritable features. I think one should be very cautious here and look at them on a case by case basis and not generalize too much.
Each of those aspects has specific logic in code and the "inheritable" part can interact in various ways.

For example, I purposefully did not patch custom attributes (only custom elements), because the interaction of inherited @bindable with the default value attribute was messy, unclear and possibly a breaking change.

Another example from your list: @useView is a totally different beast that is not stored in the same metadata as most other aspects (no idea why).

[Infuser]

Is this now done and in aurelia framework?

[jods4]

@Infuser PR is still open...

[Infuser]

Thanks @jods4 for the quick response sounds like the work is done any idea when it will be released

[jods4]

Nope. If you look up you'll see that this has been ready for a while now. :(

[atsu85]

@Infuser, I guess You should ping @EisenbergEffect about merging this PR.

As i understand, the last comment from @EisenbergEffect

It's been fixed! Nice.

could have been a reaction based on misunderstanding @jods4 comment that said that inheritance (__extend method used by TypeScript) has been changed, that allowed him to remove one workaround. It seems like @EisenbergEffect could have taken that comment as if this PR became obsolete with TS 2.2 (but it hasn't, as i understand).

I'd also like to see it merged even if custom decorator could sometimes be used to achieve similar result.

@jods4, @EisenbergEffect, could You add Your comments & thoughts?

[EisenbergEffect]

My comment above is related to the fix in TypeScript. I'm glad that's worked out. Generally speaking, I'm very hesitant about adding support for inheritance because I think inheritance is bad ;) However, I'd be willing to merge this PR in and add support on the condition that @jods4 or someone else write a new article for our documentation, explicitly covering inheritance with components. The article should show how, explain the limitations and constraints, and provide some guidance, along with examples. If we can have that, I'll merge this in and release it shortly thereafter. I really don't want to put this feature out there without clear documentation though.

[tjad]

@EisenbergEffect Does this mean that you are not inevitably looking at an "inheritable component"?

Re the docs: This PR wont particularly sit well(look good) if you want documentation on the explicit use of inheritance with components. This is only 1 of the required parts, hence currently the limitations/nuances are immense regarding inheritance - little things the Aurelia team seem to neglect (More than likely due to oversight)

@EisenbergEffect Honestly, being the leader of this project, your non-support for such an integral feature of ES6, which is in fact obscured by your framework (WHICH I LOVE USING!) doesn't instil confidence. At the least, I was hoping you could schedule some team support around this issue and help us with a bit of direction. Apart from community support, @jods4 seems to be the only help with direction.

I was keen to get helping with the necessary to fully enrich the extensibility of Aurelia, but you have a biggish project and without direction and no support from the core team this proves difficult.

[EisenbergEffect]

I have agreed to merge in the initial core support for inheritance, pending documentation. After that, we can continue to improve this feature going forward based on concrete use cases from the community. @tjad You are welcome to join the other 542 contributors to Aurelia by helping out in this effort.

[tjad]

Thanks @EisenbergEffect, this being the case, it should be worth my time helping with documenting the usage/nuances. Thanks for the prompt response.

[EisenbergEffect]

Absolutely. We want to take feedback from the community and use the real-world use cases to help drive framework evolution. Just consider this as a first, non-breaking step towards inheritance support. It will enable some new scenarios, but maybe not everything we want just yet. So, we iterate on it and continue to improve it over time.

[tjad]

@jods4 In trying to comply with the contribution guides, could you please amend the git message with a body as well as make the subject more elaborate, I believe the footer can be omitted.

https://github.com/DurandalProject/about/blob/master/CONTRIBUTING.md#subject

As a first draft, (Based on your opening comment of this PR)

"
feat(html-behavior): Allow inheritance of bindable properties for custom elements

Ability for @bindable properties to be inherited from a base class of custom elements
It only adds support for @bindable properties. No other aspect from the base HtmlBehaviorResource is propagated to descendant classes.
"
I will look at writing some use case documentation a bit later today.

[EisenbergEffect]

I'm going to make some tweaks to the documentation. After that, we'll get this out in the next release.

[Infuser]

Thanks everyone for getting this done very much appreciated

[Spontifixus]

Thank you for merging this!

For those of you looking for answers to the suggestion to solve this using composition rather than inheritance, I have written up how to achieve that over on StackOverflow: https://stackoverflow.com/a/45361429/1521227

[maroy1986]

Just being curious here, any timeline on when we could expect this feature to be released?

[tjad]

Just a heads up, I may have found an oversight with the merged fix. In a template, when I include 2 custom elements having the same inherited parent that has a common @bindable property, the second child listed will not be rendered. I suspect the resource silently fails upon resource loading/processing - before the rendering.
The first child resource to load succeeds, the rest silently fail.

https://gist.github.com/tjad/4414b3517f71112c0f49de59afb443a9

when I remove the commonBindable property, the resources load fine. - hence why I am logging this issue here.

I picked this issue up in my application, and though I get a No Aurelia APIs are defined for the element: error (followed by element NAME-IN-CAPS) I am unable to reproduce this as a standalone application. - hence my suspicion of silent failure on resource load. (in my app, the NAME-IN-CAPS would be the subsequent child resource.). This causes Aurelia to crash.

UPDATE: Will try do some further digging into the issue, but wont be too soon.

├── aurelia-dialog@1.0.0-beta.3.0.1
├── aurelia-fetch-client@1.1.2
├── aurelia-files@0.1.3
├─┬ aurelia-framework@1.1.0
│ ├── aurelia-binding@1.2.1
│ ├── aurelia-loader@1.0.0
│ └── aurelia-task-queue@1.2.0
├─┬ aurelia-froala-editor@2.7.3
│ └── froala-editor@2.6.5
├── aurelia-history-browser@1.0.0
├── aurelia-http-client@1.1.0
├── aurelia-loader-default@1.0.2
├── aurelia-logging-console@1.0.0
├── aurelia-pal-browser@1.2.0
├── aurelia-polyfills@1.2.1
├─┬ aurelia-router@1.2.1
│ └── aurelia-route-recognizer@1.1.0
├── aurelia-templating@1.7.0
├─┬ aurelia-templating-binding@1.4.0
│ └── aurelia-binding@1.6.0
├── aurelia-templating-resources@1.3.1
├── aurelia-templating-router@1.1.0
├── aurelia-testing@1.0.0-beta.4.0.0
├─┬ aurelia-tools@1.0.0
│ └─┬ breeze-dag@0.1.0
│ ├─┬ breeze-queue@0.1.0
│ │ └── breeze-nexttick@0.2.1
│ └── gaia-tsort@0.1.0

[jods4]

@tjad Please read the PR description above, in particular the restrictions:

The derived classes need their own HtmlBehaviorResource metadata. So be sure to have at least one @bindable or @customElement or something like that on the derived class. Note that @useView is not stored there.