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

Aurelia should support boolean values for attributes. #347

Closed
AshleyGrant opened this issue Mar 14, 2016 · 40 comments
Closed

Aurelia should support boolean values for attributes. #347

AshleyGrant opened this issue Mar 14, 2016 · 40 comments

Comments

@AshleyGrant
Copy link
Collaborator

I should be able to tell Aurelia that an attribute's value should be treated as a boolean. This should work for both simple custom attributes as well as attributes on custom elements.

Usage:

<foo bar="false"></foo>

Custom Element VM:

export class Foo {
  @bindable({ isBoolean: true }) bar;
}

If the custom attribute or custom element tells Aurelia that it expects its value to be a boolean, the Aurelia will coerce the value to a boolean before setting the VM property and calling any changed callbacks.

@EisenbergEffect
Copy link
Contributor

I'm thinking this will merge with our support for typed bindables. A typed bindable would look like this:

@bindable.boolean bar;

This would ensure that the property value was always a boolean type, using a value converter under the hood. These value converters could have a property set on them which indicates if they should reflectToAttribute. If that is set, when the property is updated it will update the DOM. All primitive converters would update the DOM (boolean, int, float and string). Additionally, we can allow for boolean properties to add/remove the attribute instead of updating the value. That could be done like this potentially:

@bindable.boolean({addRemove: true}) bar;

@kevmeister68
Copy link

I was going to raise a similar issue but found this. When you declare a bindable attribute on your custom element, you don't know whether someone is going to directly assign a literal value to it, or bind to it, and therefore the value of the underlying property could end up as a literal string, or as whatever type is bound to it.

I think I would prefer to see the @bindable( ) decorator being extended so the options object allows an implicit "value converter" being able to be defined by the user, plus a precanned set of Aurelia-supplied converters.

Otherwise, it seems to me that if someone wants their own coercion function, they may have to build their own decorator, which falls into the category of sledgehammer cracking walnut.

@EisenbergEffect
Copy link
Contributor

Yes, the options object would be extended to enable providing a value converter. However, we could also provide a nice syntax, as above, for the common primitive types. That syntax would essentially just create the options object and configure the value converter automatically.

@fopsdev
Copy link

fopsdev commented Apr 6, 2016

like this feature.
just in case -> float numbers can be formatted differently depending on the used culture
i'm just telling that because i see developers running into this issue again and again :)

@ajayvikas
Copy link

Is this planned for final release or thereafter?

@EisenbergEffect
Copy link
Contributor

It's planned for a point release. Don't we are stopping with 1.0. We just need to cut an official release rather than let it go on forever. After that, we turn right back around to implement some new features. This is one that is near the top of the list.

@jods4
Copy link
Contributor

jods4 commented Jul 4, 2016

Would love to have this, especially the add/remove part, so that you can do <data-field required /> and have a boolean required attribute in code-behind. I would go as far as say that for bindable.boolean it should be the (overidable) default.

@thomas-darling
Copy link

Yep, we really need this too, and looking very much forward to it - both the type support, the reflectToAttribute option, and the add/remove behavior for boolean attributes.

We would also really like to see the @bindable decorator leverage TypeScript metadata, if available, to automatically determine the type - that way, this will just work:

@bindable
public foo: boolean;

@jdanyow
Copy link
Contributor

jdanyow commented Aug 24, 2016

@thomas-darling here's the add/remove behavior for boolean attributes: https://gist.run/?id=744ae2416278d35b5a7d450f3cc118f5

@thomas-darling
Copy link

thomas-darling commented Aug 24, 2016

@jdanyow Thanks, didn't think of using a binding behavior for that - might come in handy!

I think there's more to it that that, though. Let's take the disabled attribute on input elements as an example - that is a boolean attribute with no value - it's the fact that it is present on the element that makes it true.

From the spec:

A number of attributes are boolean attributes. The presence of a boolean attribute on an element represents the true value, and the absence of the attribute represents the false value.

If the attribute is present, its value must either be the empty string or a value that is an ASCII case-insensitive match for the attribute's canonical name, with no leading or trailing whitespace.

NOTE: The values "true" and "false" are not allowed on boolean attributes. To represent a false value, the attribute has to be omitted altogether.

https://www.w3.org/TR/html5/infrastructure.html#boolean-attributes

I'd also say it should be @bindable that controls whether the attribute uses the add/remove behavior. Think of a custom input element, which comes with styles for when the element is disabled - in this case, the CSS distributed with that element might apply different styles depending on the presence of the disabled attribute. It would be highly error prone if we had to remember to add a binding behavior to every disabled attribute, in order to make that styling work.

So, I'd say this property, which we know is boolean

class Foo
{
    @bindable
    public disabled: boolean;
}

Should be true for:

<foo disabled></foo>
<foo disabled=""></foo>
<foo disabled="disabled"></foo>
<foo disabled.bind="true"></foo>

And false for:

<foo></foo>
<foo disabled.bind="false"></foo>

And if reflectToAttribute is enabled for that @bindable, I'd say it should actually default to the add/remove behavior for boolean properties.

So, while the element in my markup might be written as:

<foo disabled.bind="true">I'm disabled</foo>
<foo disabled.bind="false">I'm enabled</foo>

The element in the DOM should be:

<foo disabled>I'm disabled</foo>
<foo>I'm enabled</foo>

That way CSS like this will always work as expected:

foo[disabled] { color: gray }

Makes sense?

@atsu85
Copy link

atsu85 commented Aug 28, 2016

@thomas-darling,

<foo disabled.bind(true)></foo>

I guess You are not proposing alternative syntax, but meant
<foo disabled.bind="true"></foo>
instead?

@atsu85
Copy link

atsu85 commented Aug 28, 2016

@jdanyow, I guess You are afraid to make braking change for stock html elements to avoid adding & removeAttribute ?

If so, is there a good reason, why Your RemoveAttributeBindingBehavior solution shouldn't be shipped with Aurelia by default? What are the disadvantages in addition to adding extra & removeAttribute everywhere?

@thomas-darling
Copy link

@atsu85 Wow, thanks, not sure how I managed to screw that up :-P
But yes, existing syntax - it's fixed in the comment now :-)

@thomas-darling
Copy link

@EisenbergEffect
I was wondering if there's any news or plans to tackle this issue?

We're starting to really feel some pain from this and #96, both due to the lack of type inference/conversion, but also because we would like to style some elements based on the value of certain bindable properties, which can't be done without the reflectToAttribute option mentioned in the discussion above.

@EisenbergEffect
Copy link
Contributor

@thomas-darling I feel your pain. I wanted this just the other day for a new library we're working on (which we'll be announcing soon). I'm not sure how soon we can get to it. I might be able to prioritize it in a couple of weeks since it is becoming a dependency. Any interest in contributing? It is a bit of a tricky part of the codebase but it's a great feature.

@thomas-darling
Copy link

@EisenbergEffect Thanks, it would be much appreciated, as it's probably one of our biggest issues at this point - there's a couple of others, but this one just keeps popping up in so many places.

Regarding contributing, it's something I personally would absolutely love to do, and also something we, as a company, are actively talking about doing, once we get past our own upcoming release. That said though, we have some very tight deadlines ahead, and as I'm already working 12+ hours a day, it doubt this is something we could easily tackle at this time - I wish we could though, as this is a part of Aurelia I would really like to get to know better myself, and it would definitely be a great contribution.

Would you maybe be able to give just a couple of quick pointers to where one might start digging, just in case I, or anyone else, were to find some time to look into it?

@EisenbergEffect
Copy link
Contributor

The place to look at is in the templating library. There's a class called BindableProperty. You would also want to look at the associated decorator.

@kevmeister68
Copy link

One comment I would like to make with the ValueConverter type scenarios (with or without reflectToAttribute) is whether you guys have given any consideration to error handling within the conversion pipeline (or what that even means).

For a trivial example:

<button disabled.bind="fred">

where the "fred" property could have numerous dubious or illegal values such as "jabberwocky" (is that true or false)? or a Date object, etc.

I mention this because in our current app we went to a little bit of effort to ensure that our "input controls" (Date Time Pickers, Text Boxes, Currency Inputs and the like) performed type-checks on their bound-to values and went into a kind of "bad data" mode if they received bad data. We could not use the existing type converter paradigm because there is (was?) no way to trap/block erroneous data.

@EisenbergEffect
Copy link
Contributor

We've encountered the issue but hadn't designed a solution yet. Definitely open to any ideas you have. We'd like to do it without making a breaking change. Let us know what you think.

@ghiscoding
Copy link

Any news on this?

So far, the only way I got this working is by defining a parseBool(value) function in my ViewModel and using it in the View which isn't the most convenient (however it does work).

@teoxoy
Copy link

teoxoy commented Jul 24, 2017

Any news on this?

@nashwaan
Copy link

Any update on this?

@bigopon
Copy link
Member

bigopon commented Oct 18, 2017

If things go well with #623

You will be able to do:

export class VideoPlayer {
  @bindable({
    coerce(val) {
      if (val || val === '') {
        return true;
      }
      return false;
    }
  })
  playing
}

Usage:

<!-- app.html -->
<template>
  <video-player playing></video-player>
  <video-player playing=''></video-player>
  <video-player playing='true'></video-player>
  <video-player playing='playing'></video-player>
</template>

With only playing attribute or an empty string / some common sense string values on the element like above example in the app.html, it will yield the desire value in playing


There is another pending PR aurelia/templating#560, to help you sync back the property playing to attribute on the custom element. Which means whenever playing is set to true, playing will be set on the custom element, like this <video-player playing />

@nashwaan
Copy link

@bigopon I like coerce() idea and it looks promising, but I am not sure how it can be used for:

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

to force number input to be converted from string to number (see also this).

Also, will it play nicely with & validate?

@bigopon
Copy link
Member

bigopon commented Oct 18, 2017

For: <input type=number/>.

The coercion happens when a value is assigned to a property with coerce registered. You just need to bind the input value to a property like that.


@nashwaan Update: I failed to address your question about input number coercion. Number coercion is one of the built-ins along with string, boolean, date

So it will look like this:

export class NumberField {
  @bindable({ coerce: 'number' })
  value
}
<template>
  <input type='number' value.bind='value' />
</template>

For validation:

Any binding behavior will play nicely with coerce as long as it let coercion happen first. And from the aurelia validation code, I see there are no issues.

@Alexander-Taran
Copy link
Contributor

simlar to discussion about number value converter to be built in
aurelia/templating#96

good candidate for contrib

maybe can be closed

@EisenbergEffect
Copy link
Contributor

Let's keep this open for now since @bigopon has some in-progress work on it.

@Alexander-Taran
Copy link
Contributor

Schrodingers issue?

@EisenbergEffect
Copy link
Contributor

Clicked the wrong button...

@ghiscoding
Copy link

@EisenbergEffect
Will Aurelia vNext have a better support of boolean and number with bindable? It's kinda of annoying seeing their type changing to string instead of what they really are. Until then, I'll keep the number value converter that I've put in place to deal with them.

Quite nice to see so much commits and PRs happening on the aurelia/aurelia vNext. Keep it up :)

@EisenbergEffect
Copy link
Contributor

@ghiscoding That's a great question. To be honest, that's a behavior we've struggled with on what the right solution is. @fkleuver is doing most of the vNext binding engine work. I'd love to hear what his thoughts are on it. I'd wager a bit that it's much easier to accomplish this in vNext given that the new engine is much more powerful and extensible.

@fkleuver
Copy link
Member

@ghiscoding
As you may know this is a problem with the HTML specification. The value property of an input element will always coerce to string, no matter what its type is.
The current binding system simply keeps the values as they are, which on a fundamental level is the right thing to do as it is consistent with how HTML behaves. But in this case HTML kind of misbehaves.

About a month ago I created an issue for vNext to deal with exactly this (and other related things) by going systematically through the HTML specs and making sure we've got every scenario covered with a certain degree of configurability.

@EisenbergEffect
This isn't necessarily "easier" in vNext insomuch that it isn't "difficult" in vCurrent. It's just a matter extending/implementing the correct observers. I've no doubt we can backport most of the element observation specific logic to vCurrent when it's ready in vNext. I'm adding more details in the issue I mentioned.

@EisenbergEffect
Copy link
Contributor

Thanks @fkleuver !

@Alexander-Taran
Copy link
Contributor

There is now an aurelia-contrib plugin for that.
https://github.com/aurelia-contrib/aurelia-typed-observable-plugin

@EisenbergEffect issue can be closed

@thomas-darling
Copy link

thomas-darling commented Jan 7, 2019

@Alexander-Taran,
I strongly disagree - this absolutely needs to work out of the box.

@EisenbergEffect,
This is, literally, the top of my wish list for the next version of Aurelia (a better router is at 5th place):

1. Support custom coercers

The @bindable decorator should support an option to specify which converter to use, which may be a named coercer registered somewhere, or a specific coercer class or instance - which supports coercion both to and from the view.

@bindable({ coerce: 'custom' })
public foo: Custom;

@bindable({ coerce: CustomCoercer })
public foo: Custom;

@bindable({ coerce: customCoercer })
public foo: Custom;

2. Leverage TypeScript metadata

The @bindable decorator should leverage TypeScript metadata, if available, to automatically determine which coercer to use - at least for simple types like string, number and boolean.
That way, this will just work:

3. Support reflection back to the DOM

The @bindable decorator should have an option to reflect the value back to the DOM, for styling pourposes. That way, this:

@bindable({ reflect: true })
public theme: string = "success";

public message: string = "This is a success message";
<message theme.bind="theme">${message}</message>

Would result in this DOM:

<message theme="success">This is a success message</message>

Where we can use this CSS:

[theme="success"] { color: green; }
[theme="error"] { color: red; }

4. Support reflection of boolean attributes

Reflection back to the DOM should support the special needs of boolean attributes
See my previous comment: #347 (comment)

Here's some initial thoughts on how the interface of a coercer might look:

interface ICoercer<TView, TModel>
{
    // Converts a value from the model for use in the view.
    toView?(value: TModel): TView;

    // Converts a value from the view for use in the model.
    fromView?(value: TView): TModel;

    // When the `@bindable` option `reflect` is set, this method will be called.
    // If it returns a value, that value will be assigned to the attribute in the DOM.
    // If it returns undefined, it takes full responsible for manipulating the element.
    // Note that the `reflect` option can also be a string, in which case it indicates 
    // how the value should be reflected - could be e.g. "boolean" or "yes-no".
    reflect?(element: Element, value: TModel, reflect?: string): string | undefined;
}

I think it makes sense to have the reflect method in there, as it is kinda related. Alternatively it could be broken out into a separate IReflector interface, and used in a way similar to how coercers are used.

Why?

Because literally everyone expects this to just work, and forgetting to convert the type has by far been our largest source of bugs - it's just way too easy to forget, and the bugs can be extremely nasty.
It's also super annoying, as the code to convert the values pollute our otherwise simple controllers.

This stuff is so tightly coupled to the binding internals, that I'm personally not comfortable taking a dependency on a third party plugin, which may or may not be updated to work with the latest version of the framework. This needs to be part of Aurelia itself.

@EisenbergEffect
Copy link
Contributor

@thomas-darling Can you move this comment to an RFC for vNext? Here's a link to where you can do that: https://github.com/aurelia/aurelia/issues/new/choose I do agree this should be discussed. I've found this to be a pain as well and we've heard pretty consistent feedback on that over the years.

@Alexander-Taran
Copy link
Contributor

@thomas-darling the plugin is from a core team member.
I doubt that we will see support for this in the core of vCurrent any time soon.
As I heard basic support for typed property bindings is already in the works in vNext.
I want to thank you as well for such a nice summary of your pains.

I still propose to close this issue.

@fkleuver
Copy link
Member

fkleuver commented Jan 7, 2019

I don't think this one should be closed just yet because it really isn't hard at all to add it to vCurrent. It's just a matter of making ObserverLocator smarter with awareness of what value types the various attributes can have, and letting it return correctly typed observers for that. Related to aurelia/aurelia#75

I wouldn't want to "half-ass" this though. I want do a proper and thorough check on the html spec to see which attributes are supposed to be what, implement the observers/locator in vNext accordingly, and then see which parts make sense to backport.

Right now vCurrent just about treats every attribute as a string and that's simply not correct - it even goes against the spec.

In general my idea with this, as eluded above, is to make Aurelia's binding more aware of what attributes are supposed to be according to the spec, and not add actual coercion configurability anywhere. Because if the defaults are correct, then you shouldn't need the configuration in 99% of cases and when you do, a value converter seems like the best way to go about it anyway.

@thomas-darling
Copy link

I've added a comprehensive RFC based on my previous comments.
Let's continue the discussion in there :-)

aurelia/aurelia#368

@EisenbergEffect
Copy link
Contributor

Closing this in favor of aurelia/aurelia#368

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