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

Live binding of not-yet-defined attr in a Component's template fails #579

Closed
robertheessels opened this Issue Nov 27, 2013 · 10 comments

Comments

Projects
None yet
5 participants
@robertheessels

robertheessels commented Nov 27, 2013

A fiddle where using a not yet defined attribute in a template does not live bind correctly:

http://jsfiddle.net/robertheessels/VRE8Z/3/

When the undefined attribute is used inside a mustache template, the live binding is correct, but...
when the attribute is used inside the template of a COMPONENT, the live binding fails.

{{click}} works, {{notyetset}} does not work.
{{../click}} and {{../notyetset}} do work, and is probably the right way anyway, but then why does {{click}} work also?

At the very least this is inconsistent.

@robertheessels

This comment has been minimized.

Show comment
Hide comment
@robertheessels

robertheessels Dec 2, 2013

I'm not sure, but it could be that it works in 2.0.0 while it does not work in 2.0.3.

robertheessels commented Dec 2, 2013

I'm not sure, but it could be that it works in 2.0.0 while it does not work in 2.0.3.

@daffl

This comment has been minimized.

Show comment
Hide comment
@daffl

daffl Dec 13, 2013

Contributor

Just verified, this doesn't work in 2.0.0 either. Hopefully we can fix this for 2.0.4.

Contributor

daffl commented Dec 13, 2013

Just verified, this doesn't work in 2.0.0 either. Hopefully we can fix this for 2.0.4.

@ghost ghost assigned justinbmeyer Dec 13, 2013

@justinbmeyer

This comment has been minimized.

Show comment
Hide comment
@justinbmeyer

justinbmeyer Dec 13, 2013

Contributor

The core reason this is happening is that not-yet-defined live binding only works on the most-likely and highest context for performance reasons. That means if I do:

{{#name}}{{age}}{{/name}}
var person = can.Map({
  name: {}
})
template(person)

When the template is rendered, age is undefined so it sets up a binding on the nested name map for "age". This means if I were to do:

person.attr("age",31)

The tempalte would not be updated, but it would be with:

person.attr("name.age",31)

It's this way so we only have to read one context when something changes. This makes it much faster to get a new value for live-binding. The cached lookup path is used here: https://github.com/bitovi/canjs/blob/master/view/scope/scope.js#L274.

How this relates to your fiddle ....

When a component is created, a new context is created from the "scope" object on the component (terrible naming I know). That context has no notyetset property. When _Click Me is clicked, it adds a notyetset to this.counter which is not the same object as the component's context. This is why notyetset does not show up within the component ... it's looking for notyetset on the component's context.

The only fix for this might be to make live-binding able to walk the entire context over and over (potentially binding on every can.Map in the scope) until a value is found, and then falling back to the higher performance method it uses today.

A fix for this would be to add ../ in {{../notyetset}} like I've done here: http://jsfiddle.net/VRE8Z/4/.

@daffl I'm not sure this should be in 2.0.4, 2.1 seems more reasonable as it won't be a super easy fix and there's a workaround.

Contributor

justinbmeyer commented Dec 13, 2013

The core reason this is happening is that not-yet-defined live binding only works on the most-likely and highest context for performance reasons. That means if I do:

{{#name}}{{age}}{{/name}}
var person = can.Map({
  name: {}
})
template(person)

When the template is rendered, age is undefined so it sets up a binding on the nested name map for "age". This means if I were to do:

person.attr("age",31)

The tempalte would not be updated, but it would be with:

person.attr("name.age",31)

It's this way so we only have to read one context when something changes. This makes it much faster to get a new value for live-binding. The cached lookup path is used here: https://github.com/bitovi/canjs/blob/master/view/scope/scope.js#L274.

How this relates to your fiddle ....

When a component is created, a new context is created from the "scope" object on the component (terrible naming I know). That context has no notyetset property. When _Click Me is clicked, it adds a notyetset to this.counter which is not the same object as the component's context. This is why notyetset does not show up within the component ... it's looking for notyetset on the component's context.

The only fix for this might be to make live-binding able to walk the entire context over and over (potentially binding on every can.Map in the scope) until a value is found, and then falling back to the higher performance method it uses today.

A fix for this would be to add ../ in {{../notyetset}} like I've done here: http://jsfiddle.net/VRE8Z/4/.

@daffl I'm not sure this should be in 2.0.4, 2.1 seems more reasonable as it won't be a super easy fix and there's a workaround.

@daffl daffl modified the milestone: 2.1.0 Mar 21, 2014

@daffl

This comment has been minimized.

Show comment
Hide comment
@daffl

daffl Mar 21, 2014

Contributor

Are we still going to add this to 2.1? Explicitly passing the property like

<test_component notyetset="notyetset"></test_component>

Gets the Fiddle to work in 2.0.5: http://jsfiddle.net/VRE8Z/5/

Contributor

daffl commented Mar 21, 2014

Are we still going to add this to 2.1? Explicitly passing the property like

<test_component notyetset="notyetset"></test_component>

Gets the Fiddle to work in 2.0.5: http://jsfiddle.net/VRE8Z/5/

@justinbmeyer

This comment has been minimized.

Show comment
Hide comment
@justinbmeyer

justinbmeyer Apr 25, 2014

Contributor

Punting on this for 2.1 b/c there is a work around and I'm nervous to change the performance of apps when this release is so close.

Contributor

justinbmeyer commented Apr 25, 2014

Punting on this for 2.1 b/c there is a work around and I'm nervous to change the performance of apps when this release is so close.

@ccummings ccummings modified the milestones: 2.2.0, 2.1.0 Apr 25, 2014

@daffl daffl modified the milestones: 2.3.0, 2.2.0 Feb 19, 2015

@justinbmeyer

This comment has been minimized.

Show comment
Hide comment
@justinbmeyer

justinbmeyer May 18, 2015

Contributor

This is probably much more easy to accomplish now that I've pulled out the compute that reads from the scope into this module: https://github.com/bitovi/canjs/blob/master/view/scope/compute_data.js#L39 to fix #1617.

It should be possible now to detect if we've actually found the right root observable (because there is a value) and only then stop walking up the entire scope.

Contributor

justinbmeyer commented May 18, 2015

This is probably much more easy to accomplish now that I've pulled out the compute that reads from the scope into this module: https://github.com/bitovi/canjs/blob/master/view/scope/compute_data.js#L39 to fix #1617.

It should be possible now to detect if we've actually found the right root observable (because there is a value) and only then stop walking up the entire scope.

@gsmeets

This comment has been minimized.

Show comment
Hide comment
@gsmeets

gsmeets May 20, 2015

Contributor

This is for me the most problematic behaviour when writing templates. Getting this fixed is important. I see it's been bumped a few times to future milestones. Can we get this fixed in 2.3? Do you need help with it?

Contributor

gsmeets commented May 20, 2015

This is for me the most problematic behaviour when writing templates. Getting this fixed is important. I see it's been bumped a few times to future milestones. Can we get this fixed in 2.3? Do you need help with it?

@justinbmeyer

This comment has been minimized.

Show comment
Hide comment
@justinbmeyer

justinbmeyer May 22, 2015

Contributor

@gsmeets Help would be great. However, my hesitation has been around performance. With a very nested structure, this could mean making 50 or so bindings, one on every observable object in the scope chain.

I suppose we could take the performance cost but let people know to either use:

  • leakScope: false
  • default their undefined values to null
  • use ../

To get back any performance losses.

Contributor

justinbmeyer commented May 22, 2015

@gsmeets Help would be great. However, my hesitation has been around performance. With a very nested structure, this could mean making 50 or so bindings, one on every observable object in the scope chain.

I suppose we could take the performance cost but let people know to either use:

  • leakScope: false
  • default their undefined values to null
  • use ../

To get back any performance losses.

@justinbmeyer

This comment has been minimized.

Show comment
Hide comment
@justinbmeyer

justinbmeyer May 23, 2015

Contributor

@gsmeets Added a pull request. Please try this with your app and let me know if it works and performs ok.

Contributor

justinbmeyer commented May 23, 2015

@gsmeets Added a pull request. Please try this with your app and let me know if it works and performs ok.

@gsmeets

This comment has been minimized.

Show comment
Hide comment
@gsmeets

gsmeets May 27, 2015

Contributor

I'll give it a go. I do run on 2.1.4 still though, so I'm hoping I don't have much patching to do.

Contributor

gsmeets commented May 27, 2015

I'll give it a go. I do run on 2.1.4 still though, so I'm hoping I don't have much patching to do.

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