Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add a type option to bindable for type conversion #96

Closed
Aaike opened this issue Jun 15, 2015 · 37 comments
Closed

Add a type option to bindable for type conversion #96

Aaike opened this issue Jun 15, 2015 · 37 comments

Comments

@Aaike
Copy link
Contributor

Aaike commented Jun 15, 2015

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
Copy link
Contributor

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
Copy link
Contributor Author

Aaike commented Jun 15, 2015

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

@EisenbergEffect
Copy link
Contributor

@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
Copy link
Contributor Author

Aaike commented Jun 23, 2015

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
Copy link
Contributor

Yes. That's my thought.

@brettveenstra
Copy link

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
Copy link

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
Copy link
Contributor

jdanyow commented Mar 22, 2016

@brettveenstra / @jadrake75 would you agree that <input type="text" value.bind="item.address.active|bool"> is just as good as #96 (comment), assuming we have a built-in bool value converter?

@csaloio
Copy link

csaloio commented Mar 22, 2016

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
Copy link
Contributor

jdanyow commented Mar 22, 2016

@csaloio right- I was specifically talking about this comment #96 (comment) by @brettveenstra.

@csaloio
Copy link

csaloio commented Mar 22, 2016

Ah gotcha. Thought I might have misunderstood :)

@jadrake75
Copy link

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
Copy link

fopsdev commented Mar 23, 2016

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
Copy link
Contributor

niieani commented Jul 26, 2016

@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
Copy link
Contributor

jdanyow commented Jul 26, 2016

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
Copy link
Contributor

niieani commented Jul 26, 2016

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

type CoerceInstruction = 'string' | 'number' | 'date' | { (value: any): any };

@EisenbergEffect
Copy link
Contributor

EisenbergEffect commented Jul 26, 2016

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
Copy link

@EisenbergEffect There's also aurelia/binding#347 (comment) 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 #96 (comment) 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 aurelia/binding#347. Just keep it in mind :-)

@npelletm
Copy link

npelletm commented Oct 21, 2016

I also like @jdanyow's proposal.

@RWOverdijk
Copy link

Does something like this exist now?

@RicoSuter
Copy link

RicoSuter commented Apr 7, 2017

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: #546

@gheoan
Copy link
Contributor

gheoan commented Oct 20, 2017

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

@gheoan tried this, doesnt work.
See #546

@bigopon
Copy link
Member

bigopon commented Oct 20, 2017

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
Copy link

nashwaan commented Nov 2, 2017

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 aurelia/templating-binding#112, no solution.
Also proposed here #103
Also here aurelia/binding#287
And here aurelia/binding#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 ):
#558
aurelia/binding#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
Copy link
Member

bigopon commented Nov 4, 2017

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
Copy link

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
Copy link
Contributor

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

@bigopon
Copy link
Member

bigopon commented Mar 3, 2018

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
Copy link
Contributor

Love to get some feedback from @jdanyow

@jnm2
Copy link

jnm2 commented Mar 19, 2018

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
Copy link

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

jnm2 commented Mar 19, 2018

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
Copy link

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
Copy link

jnm2 commented Mar 20, 2018

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
Copy link

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

@Alexander-Taran
Copy link

can be closed with a plugin

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests