aurelia / templating

An extensible HTML templating engine supporting databinding, custom elements, attached behaviors and more.
MIT License
116 stars 103 forks source link

Add a type option to bindable for type conversion #96

Closed Aaike closed 6 years ago

Aaike commented 9 years ago

I was wondering if it would be a good idea to add an option to the @bindable decorator to make it convert a value to a different type than a string.

This can be useful when creating a custom element that accepts numeric or boolean options :

<slider min="0" max="10" precision="0.1" snap="true"></slider>

Right now the values would have to be manually converted every time the variable is used. perhaps an option could be added like this:

@bindable({type:"int"}) min;
@bindable({type:"int"}) max;
@bindable({type:"float"}) precision;
@bindable({type:"boolean"}) snap;
EisenbergEffect commented 9 years ago

Absolutely. One of the ideas I have is to let you specify a ValueConverter as part of your bindable definition. This would then enable arbitrary type conversion to be built in to the property. To simplify common use cases, you could specify a type, and then we would select a built-in converter based on that type. In the case of TypeScript, we can even leverage the type metadata to do this automatically. Thoughts?

Aaike commented 9 years ago

yes using some common defaults and then able to pass custom value converter seems much more flexible :)

EisenbergEffect commented 9 years ago

@Aaike I'm thinking of something like this:

For TS, we could write:

export class MyCustomElement {
  @bindable foo:boolean = false;
}

Assuming they turned on TS metadata, we could interpret that as this:

bindable({name:'foo', defaultValue: false, converter:BooleanConverter});

For ES6 we could enable something like this:

export class MyCustomElement {
  @bindable.boolean foo = false;
}

Thoughts?

Aaike commented 9 years ago

Those defaults look great yes, and this also still allows custom converers for ES6 in the following form i assume ?

export class MyCustomElement {
  @bindable({converter:SomeConverter}) foo = false;
}
EisenbergEffect commented 9 years ago

Yes. That's my thought.

brettveenstra commented 8 years ago

If I was taking the lazy way and tried to bind a complex JSON object into my viewmodel, would I have to create a custom valueconverter or define a nested @bindable or define on the element itself?

I think I'd prefer something like <input type='text' value.bind='item.address.active:bool'>

jadrake75 commented 8 years ago

I think the datatype to the binding signature would be a great feature. Just discovered our numeric inputs are converting to strings so this would have been nice to coerce the binding without having to use a value converter

jdanyow commented 8 years ago

@brettveenstra / @jadrake75 would you agree that <input type="text" value.bind="item.address.active|bool"> is just as good as https://github.com/aurelia/templating/issues/96#issuecomment-172212076, assuming we have a built-in bool value converter?

csaloio commented 8 years ago

Apologies if I misunderstand, but I don't think a ValueConverter (used in a view) is a substitute for being able to specify a data type in a @bindable (say inside a CustomElement) because as a CustomElement author I don't want consumers of the CustomElement to need to remember to use the ValueConverter to pass the correct type.

jdanyow commented 8 years ago

@csaloio right- I was specifically talking about this comment https://github.com/aurelia/templating/issues/96#issuecomment-172212076 by @brettveenstra.

csaloio commented 8 years ago

Ah gotcha. Thought I might have misunderstood :)

jadrake75 commented 8 years ago

I agree.... the ability to bind the value to a type independently of how it is used as part of the view-model definition is quite powerful.

Note having some simple valueConverters in the framework would help anyways but I think it is a different strategy then encoding it with the binding. I myself have used ones for empty values, date formats, number conversions, currencies, object to array, upper casing, amongst others.

fopsdev commented 8 years ago

i've also recognised when going beyond the skeleton (or starter) - projects there is a high chance to need a value converter. in my recent test app i've ended up using a value converter for every input field. so i see some potential there for optimization as well (doing it in the bindable)

niieani commented 8 years ago

@EisenbergEffect I don't think we should by default infer that the user wants a given @bindable to be magically enforced to the type provided by TypeScript. This can lead to crazy errors that will be really hard to trace. I think for that use case we could have a separate decorator, like @bindableTypeEnforce or better yet a separate decorator that only deals with the type enforcing/conversion, e.g.:

@bindable @enforce('string') property;

and with the case of metadata from TypeScript:

@bindable @autoenforce property: string;

Where @autoenforce can follow the @autoinject naming convention.

I also think it would be great to be able to also pass a function to act as a simple custom enforcer using the same syntax, e.g. integer enforcer:

@bindable @enforce(value => parseInt(value)) property;
jdanyow commented 8 years ago

I agree it may not be good to automatically infer the type using TypeScript metadata. How about an API like this:

declare type CoerceInstruction = 'String' | 'Number' | 'Date' | { (value: any): any };

declare BindableConfig {
  name?: string;
  defaultBindingMode?: BindingMode;
  changeHandler? string;
  // new:
  coerce?: CoerceInstruction;
}
export class Foo {
 @bindable({ coerce: 'number' }) bar;
 @bindable({ coerce: x => +x }) baz;
}

I think this would closely mirror what built-ins like input value do in terms of type coercion.

niieani commented 8 years ago

I like @jdanyow's proposal. CoerseInstruction strings should be lowercase though:

type CoerceInstruction = 'string' | 'number' | 'date' | { (value: any): any };
EisenbergEffect commented 8 years ago

Yes, I'd prefer to not add another decorator if possible :) But, what about the fluent syntax I propose above (which could just use the coerce property under the hood):

@bindable.string firstName;
@bindable.boolean happy;
thomas-darling commented 8 years ago

@EisenbergEffect There's also https://github.com/aurelia/binding/issues/347#issuecomment-242106920 to consider - it's essentially the same as this issue, but focused more on boolean attributes, which may require some special handling. Please see my comments there.

@niieani, @jdanyow I'm not sure I understand why there would be any issue with inferring the type based on TypeScript metadata - the fact that the type is not inferred is what currently leads to "crazy errors that will be really hard to trace". In what scenario would you ever want the actual value to have a different type than the one declared? I really think this needs to be inferred.

Of course, there's limits to what can be inferred - it probably shouldn't try to support e.g union types, but making this "just work" for the 90% case of simple types like number, boolean and string, that would make things soooo much easier to work with.

Personally, I think the suggestion in https://github.com/aurelia/templating/issues/96#issuecomment-114561054 by @EisenbergEffect is absolutely the way to go. Use TypeScript metadata if available, maybe have a shorthand like @bindable.boolean for the ES6 crowd, and for everything else, just configure @bindable to use a ValueConverter. Nice and simple :-)

My only concern is that it should maybe be debated whether ValueConverter is too view focused to be used here. I can't really decide, but maybe consider whether introducing a similar, binding focused concept would be better - maybe call it a BindingConverter. I'm not sure, but depending how things are implemented, I think it might be needed to properly handle the reflectToAttribute option also discussed in https://github.com/aurelia/binding/issues/347. Just keep it in mind :-)

npelletm commented 8 years ago

I also like @jdanyow's proposal.

RWOverdijk commented 7 years ago

Does something like this exist now?

RicoSuter commented 7 years ago

I created decorators for that, but somehow the @bindable attribute completely breaks my custom decorators. See https://blog.rsuter.com/angular-2-typescript-property-decorator-that-converts-input-values-to-the-correct-type/

I've create a new issue for that: https://github.com/aurelia/templating/issues/546

gheoan commented 7 years ago

@bigopon thank you for working on this, however I am not convinced this is required to be built in the framework. Aurelia does not provide any value converters so why would the coerce functions be provided?

Can all this functionality be achived by using a decorator? This was previously suggested here. Here is a naive, untested implementation:

function convert(converter) {
    return function (target, key) {
        Object.defineProperty(target, key, {
            get: function () {
                return this["__" + key];
            },
            set: function (newValue) {
                this["__" + key] = converter(newValue);
            },
            enumerable: true,
            configurable: true
        });
    };
}
class Person {
    @bindable
    @convert(String)
    name: string;
    constructor(name: string) {
        this.name = name;
    }
}
RicoSuter commented 7 years ago

@gheoan tried this, doesnt work. See https://github.com/aurelia/templating/issues/546

bigopon commented 7 years ago

Aurelia @bindable and @observable work by rewriting properties, not intercepting them. @bindable also works in different way in templating that intercepting the property get / set from outside cannot be done easily, so its better be done within Aurelia for easy usage. You can check its behavior by following the class BindableProperty.


For @observable, it is possible to add hooks from outside, and if you look at its code, you can intercept it. Just remember to add those by adding these lines (44 - 46) to avoid dirty checking.

nashwaan commented 7 years ago

When this will be released??????

None of the below currently works (I am using aurelia-framework v1.1.5).

@bindable.number number1;
@observable.number number2;
@bindable({ coerce: 'number' }) number1;
@observable({ coerce: 'number' }) number2;

This issue #96 is closed without a clear solution. Same happened for https://github.com/aurelia/templating-binding/issues/112, no solution. Also proposed here https://github.com/aurelia/templating/pull/103 Also here https://github.com/aurelia/binding/issues/287 And here https://github.com/aurelia/binding/issues/282 ... and so many others out there.

Because anyone who used aurelia for some time would had definetly come across this issue. Also ValueConvertors and BindingBehavior don't cut it. The solution that seems the cleanest is the one proposed by @EisenbergEffect here. 👍

I see there are attempts to address this in the following pull requests (and good efforts by @bigopon ): https://github.com/aurelia/templating/pull/558 https://github.com/aurelia/binding/pull/623 But it seems it will take more work before they will be accepted and released.


So, if this will take more time before it gets released, what is the current approach to support this:

<input type="number" value.bind="number1" />

So that number1 will contain a number not string. And should work without error with validation:

<input type="number" value.bind="number1 & validate" />

🙏 ?

bigopon commented 7 years ago

So, if this will take more time before it gets released, what is the current approach to support this:

   <input type="number" value.bind="number1" />

You can add a value converter to value.bind="number1" like this:

<input value.bind="number1 | number" />

with number is a value converter:

export class NumberValueConverter {
  fromView(val) {
    let number = Number(val);
    if (!isNaN && isFinite(number)) return number;
    return 0;
  }
}
Alexander-Taran commented 6 years ago

good candidate for contrib don't think this one will get to core unless it is planned for 2.0

maybe can be closed

EisenbergEffect commented 6 years ago

There's ongoing work for this. @bigopon What is the status? Are there still some issues?

bigopon commented 6 years ago

All work to support this was done. It can be merged anytime. Still only main block is the concern over the confusion it may give when values in view and view model don't match, especially when people use non primitive typed bindables. Aurelia binding is pretty clean at the moment, so this concerns @jdanyow

EisenbergEffect commented 6 years ago

Love to get some feedback from @jdanyow

jnm2 commented 6 years ago

Just ran into this. Especially when using TypeScript this is quite a bit counter-intuitive; you get string instances coming through your type system as numbers.

Alexander-Taran commented 6 years ago

@jnm2 all input values are strings (-: with some exceptions that you can <select> objects just because you can. Or <input type=file/>. even datePickers produce strings. So it is how browsers work.

jnm2 commented 6 years ago

I expected Aurelia to know about the statically-typed model it was binding and to parseFloat before setting it back to the model. You're not wrong, that was a rookie move on my part, but I wish there was some diagnostic to warn me that Aurelia is binding an input to a non-string field on the model before it goes unnoticed as a bug for some time.

Alexander-Taran commented 6 years ago

Hmmm... It is not that statically typed at runtime (-: It is JavaScript after all where anything can be anything. Strong typing is development only. So far.

Don't be upset about your rookie move. If it was the way you wish, I bet there would be a lot more confused developers (-:

jnm2 commented 6 years ago

I know that reflection is a thing with TypeScript, so I know in theory Aurelia could act like the model is statically typed at runtime if the tooling put it all together. I still think it should either do this reflection at runtime like I expected, or do this reflection at compile time and emit a warning because the type needed to be assignable from string.

Alexander-Taran commented 6 years ago

A solution now exists in a form of a plugin https://github.com/aurelia-contrib/aurelia-typed-observable-plugin

Alexander-Taran commented 6 years ago

can be closed with a plugin