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

Stache conditionals within custom attributes break in 2.3.0-pre.2 #1770

Closed
dylanrtt opened this Issue Jul 2, 2015 · 10 comments

Comments

Projects
None yet
4 participants
@dylanrtt
Contributor

dylanrtt commented Jul 2, 2015

The following is possible in 2.2.6:

<input type="text" can-value="{{#contact}}contact.{{/contact}}name">

This is helpful in situations where the context in which a template is rendered is dynamic. I am aware that can-value doesn't actually listen to changes to the attribute so it's not possible to change the field after the template is rendered, but it does work for making the initial render dynamic.

However, it seems to breaks the parser in 2.3.0-pre.2 (only version I tested since 2.2.6) on view/stache/text_section.js#L29 and/or view/stache/utils.js:32. I don't know if there are plans to fix it before release but figured I would bring it up as soon as possible.

I have a feeling that some of you might say doing it this way is bad. Perhaps. If you guys do want to continue to support it, I can make a jsbin to reproduce the bug. If not, I can fix my code.

@justinbmeyer

This comment has been minimized.

Show comment
Hide comment
@justinbmeyer

justinbmeyer Jul 27, 2015

Contributor

I'm surprised this was working before. Especially as I'm not sure how the parsing rules might have changed.

I'll have to see what happens in 2.6 with this code.

Contributor

justinbmeyer commented Jul 27, 2015

I'm surprised this was working before. Especially as I'm not sure how the parsing rules might have changed.

I'll have to see what happens in 2.6 with this code.

@matthewp

This comment has been minimized.

Show comment
Hide comment
@matthewp

matthewp Jul 27, 2015

Contributor

It's probably caused by the change to the regexes to accommodate the new binding syntaxes.

Contributor

matthewp commented Jul 27, 2015

It's probably caused by the change to the regexes to accommodate the new binding syntaxes.

@justinbmeyer

This comment has been minimized.

Show comment
Hide comment
@justinbmeyer

justinbmeyer Jul 27, 2015

Contributor

This is caused by this code: https://github.com/bitovi/canjs/blob/v2.3.0-pre.2/view/stache/stache.js#L278

This was added so we can differentiate between two way {name} and one way {{name}} bindings in custom attributes. For example:

<my-el #other-export="{{name}}">

does something different than:

<my-el #other-export="{name}">

I'm not sure what to do with this. In the future, I'd like to replace can-value with just:

<input value="{name}"/>

With this, there's currently no "meta" way of providing the name of the attribute that should be two way bound. Although, this would be interesting.

For example, perhaps:

<input value="{| {{#contact}}contact.{{/contact}}name |}">

Would let you pass a renderer's output value as the property to be two-way bound.

Unfortunately, I don't think this is something we can support in 2.3. Can-value was never intended to work this way. Instead, it would be good to find a work around.

Contributor

justinbmeyer commented Jul 27, 2015

This is caused by this code: https://github.com/bitovi/canjs/blob/v2.3.0-pre.2/view/stache/stache.js#L278

This was added so we can differentiate between two way {name} and one way {{name}} bindings in custom attributes. For example:

<my-el #other-export="{{name}}">

does something different than:

<my-el #other-export="{name}">

I'm not sure what to do with this. In the future, I'd like to replace can-value with just:

<input value="{name}"/>

With this, there's currently no "meta" way of providing the name of the attribute that should be two way bound. Although, this would be interesting.

For example, perhaps:

<input value="{| {{#contact}}contact.{{/contact}}name |}">

Would let you pass a renderer's output value as the property to be two-way bound.

Unfortunately, I don't think this is something we can support in 2.3. Can-value was never intended to work this way. Instead, it would be good to find a work around.

justinbmeyer added a commit that referenced this issue Jul 27, 2015

@dylanrtt

This comment has been minimized.

Show comment
Hide comment
@dylanrtt

dylanrtt Jul 28, 2015

Contributor

Alright, I've updated my code to get around this. While testing the upgrade again, I found that the following also no longer works:

<select can-value="{model}">
  <option value="Person">Person</option>
  <option value="Car">Car</option>
</select>

<input type="text" auto-complete="{{ model }}">

In this case, I get the string "{{ model }}" instead of the value of model. I would have to strip the curly braces and bind to the outer scope to get changes like can-value does... is that how all custom attributes should be implemented? I could just as easily avoid that by having a separate attribute for the model as described below but I don't think I should have to.

A point of inconsistency that bothers me is that I can have helper attributes rendered with stache but not the main custom attribute itself. For example, this is how I might use a datepicker:

<input type="text" can-value="{minDate}" date-picker>
<input type="text" can-value="{maxDate}" date-picker>

<input type="text" can-value="{myDate}" date-picker
       dp-min-date="{{ minDate }}"
       dp-max-date="{{ maxDate }}">

The dp-min-date and dp-max-date attributes update automatically due to stache so the 'attributes' event observes the updates and can pass them to jQuery datepicker easily, but it's not always clear just by looking at a template which attributes can be rendered with stache and which cannot because that is determined by whether they are registered with can.view.attr.

Contributor

dylanrtt commented Jul 28, 2015

Alright, I've updated my code to get around this. While testing the upgrade again, I found that the following also no longer works:

<select can-value="{model}">
  <option value="Person">Person</option>
  <option value="Car">Car</option>
</select>

<input type="text" auto-complete="{{ model }}">

In this case, I get the string "{{ model }}" instead of the value of model. I would have to strip the curly braces and bind to the outer scope to get changes like can-value does... is that how all custom attributes should be implemented? I could just as easily avoid that by having a separate attribute for the model as described below but I don't think I should have to.

A point of inconsistency that bothers me is that I can have helper attributes rendered with stache but not the main custom attribute itself. For example, this is how I might use a datepicker:

<input type="text" can-value="{minDate}" date-picker>
<input type="text" can-value="{maxDate}" date-picker>

<input type="text" can-value="{myDate}" date-picker
       dp-min-date="{{ minDate }}"
       dp-max-date="{{ maxDate }}">

The dp-min-date and dp-max-date attributes update automatically due to stache so the 'attributes' event observes the updates and can pass them to jQuery datepicker easily, but it's not always clear just by looking at a template which attributes can be rendered with stache and which cannot because that is determined by whether they are registered with can.view.attr.

@justinbmeyer

This comment has been minimized.

Show comment
Hide comment
@justinbmeyer

justinbmeyer Sep 11, 2015

Contributor

@dylanrtt how about instead of:

<input type="text" can-value="{{#contact}}contact.{{/contact}}name">

We support a [] operator. Letting you write:

viewModel: {
  nameUpdateProperty: function(){
    return ( this.attr("contact") ? "contact." : "")+"name"
  }
}
<input value="{[nameUpdateProperty]}"/>
Contributor

justinbmeyer commented Sep 11, 2015

@dylanrtt how about instead of:

<input type="text" can-value="{{#contact}}contact.{{/contact}}name">

We support a [] operator. Letting you write:

viewModel: {
  nameUpdateProperty: function(){
    return ( this.attr("contact") ? "contact." : "")+"name"
  }
}
<input value="{[nameUpdateProperty]}"/>
@justinbmeyer

This comment has been minimized.

Show comment
Hide comment
@justinbmeyer

justinbmeyer Sep 11, 2015

Contributor

@dylanrtt regarding your last comment, I'm not entirely clear what you are saying:

In this case, I get the string "{{ model }}" instead of the value of model.

Are you sure? That seems like a bug if you do. auto-complete won't be set to "Person" or "Car"?

but it's not always clear just by looking at a template which attributes can be rendered with stache and which cannot because that is determined by whether they are registered with can.view.attr

Yes, this is why I would like to standardize on having things registered with can.view.attr always point to values surrounded with {}. For example:

value="{key}"
can-value="{key}"

(click)="{method(event)}"
can-click="{method event}"

This way, at least by looking at a template, it would be obvious.

Contributor

justinbmeyer commented Sep 11, 2015

@dylanrtt regarding your last comment, I'm not entirely clear what you are saying:

In this case, I get the string "{{ model }}" instead of the value of model.

Are you sure? That seems like a bug if you do. auto-complete won't be set to "Person" or "Car"?

but it's not always clear just by looking at a template which attributes can be rendered with stache and which cannot because that is determined by whether they are registered with can.view.attr

Yes, this is why I would like to standardize on having things registered with can.view.attr always point to values surrounded with {}. For example:

value="{key}"
can-value="{key}"

(click)="{method(event)}"
can-click="{method event}"

This way, at least by looking at a template, it would be obvious.

@justinbmeyer

This comment has been minimized.

Show comment
Hide comment
@justinbmeyer

justinbmeyer Sep 11, 2015

Contributor

So, this wouldn't be a problem if we had something other than {{}} represent one-way binding. Say, for instance, [] represented one-way binding. This way, stache could render any attribute:

<input value="[{{#contact}}contact.{{/contact}}name]"/>

The problem is that {{}} already represents setting up a "one-way to string" binding when used like:

<input value="{{contact.name}}"/>

So, we simply wanted to make the same thing work for custom attributes, but remove the limitation of having to go to a string. But this turns off stache processing inside custom attributes.

Contributor

justinbmeyer commented Sep 11, 2015

So, this wouldn't be a problem if we had something other than {{}} represent one-way binding. Say, for instance, [] represented one-way binding. This way, stache could render any attribute:

<input value="[{{#contact}}contact.{{/contact}}name]"/>

The problem is that {{}} already represents setting up a "one-way to string" binding when used like:

<input value="{{contact.name}}"/>

So, we simply wanted to make the same thing work for custom attributes, but remove the limitation of having to go to a string. But this turns off stache processing inside custom attributes.

@justinbmeyer

This comment has been minimized.

Show comment
Hide comment
@justinbmeyer

justinbmeyer Sep 11, 2015

Contributor

Actually, a problem with all of this:

<input value="[{{#contact}}contact.{{/contact}}name]"/>

Is that we'd have to be writing out value="[contact.name]" on the element. Then, we'd have to read the attribute value and then change the value back. Boo!

Contributor

justinbmeyer commented Sep 11, 2015

Actually, a problem with all of this:

<input value="[{{#contact}}contact.{{/contact}}name]"/>

Is that we'd have to be writing out value="[contact.name]" on the element. Then, we'd have to read the attribute value and then change the value back. Boo!

@dylanrtt

This comment has been minimized.

Show comment
Hide comment
@dylanrtt

dylanrtt Sep 11, 2015

Contributor

We support a [] operator

Seems reasonable.

Are you sure? That seems like a bug if you do. auto-complete won't be set to "Person" or "Car"?

If auto-complete is registered with can.view.attr then I get the raw stache code by reading the attribute.

http://jsbin.com/qarafoqugu/edit?html,js,console,output

Actually, a problem with all of this:

I don't think it's necessary to support more complex expressions like [{{#contact}}contact.{{/contact}}name]. For the simpler variants, I think simply coding the attribute callback to process the string is enough. Most of the use cases others presented in regards to this problem would be fixed by updating can-value and can-EVENT to read the string from {{key}} and then bind to that in the scope.

More complicated examples like below could be handled in the view model.

<div can-click="{{#if shown}}hide{{else}}show{{/if}}"></div>
Contributor

dylanrtt commented Sep 11, 2015

We support a [] operator

Seems reasonable.

Are you sure? That seems like a bug if you do. auto-complete won't be set to "Person" or "Car"?

If auto-complete is registered with can.view.attr then I get the raw stache code by reading the attribute.

http://jsbin.com/qarafoqugu/edit?html,js,console,output

Actually, a problem with all of this:

I don't think it's necessary to support more complex expressions like [{{#contact}}contact.{{/contact}}name]. For the simpler variants, I think simply coding the attribute callback to process the string is enough. Most of the use cases others presented in regards to this problem would be fixed by updating can-value and can-EVENT to read the string from {{key}} and then bind to that in the scope.

More complicated examples like below could be handled in the view model.

<div can-click="{{#if shown}}hide{{else}}show{{/if}}"></div>
@justinbmeyer

This comment has been minimized.

Show comment
Hide comment
@justinbmeyer

justinbmeyer Sep 11, 2015

Contributor

@dylanrtt well, I think we are going to propose something like angular 2.0's view binding syntaxes to fix this problem. Fundamentally, I'd like to keep stache able to run in any attribute. We can support this by moving the decorators from the attribute value, to the attribute name.

I'm creating an issue for discussion. There's lots of discussion going on starting here: https://gitter.im/bitovi/canjs?at=55f32a416fe7b2a123eb04d0

Contributor

justinbmeyer commented Sep 11, 2015

@dylanrtt well, I think we are going to propose something like angular 2.0's view binding syntaxes to fix this problem. Fundamentally, I'd like to keep stache able to run in any attribute. We can support this by moving the decorators from the attribute value, to the attribute name.

I'm creating an issue for discussion. There's lots of discussion going on starting here: https://gitter.im/bitovi/canjs?at=55f32a416fe7b2a123eb04d0

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