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

can.Mustache: #if sections are rendered twice when activated from live-binding #477

Closed
bmomberger-reciprocity opened this Issue Sep 14, 2013 · 10 comments

Comments

Projects
None yet
2 participants
@bmomberger-reciprocity
Contributor

bmomberger-reciprocity commented Sep 14, 2013

http://jsfiddle.net/air_hadoken/V8VvF/2/

The mustache helper "bar" "foo" is called twice when the value of "quux" is flipped to true, as evidenced by the return value having two instances of "bar". When "baz" is called on initial render, it is only called once.

This happens because live binding on the section is triggered both by the "change" function on the attribute and on the attribute name event (not shown in the fiddle but it's what I concluded after investigating the problem). EDIT: I was wrong about this. Still investigating.

Suggestion: listen on the change event only.

@justinbmeyer

This comment has been minimized.

Show comment
Hide comment
@justinbmeyer

justinbmeyer Sep 14, 2013

Contributor

the compute created live-binding is listening to change? It should not be.

Contributor

justinbmeyer commented Sep 14, 2013

the compute created live-binding is listening to change? It should not be.

@ghost ghost assigned justinbmeyer Sep 14, 2013

@justinbmeyer

This comment has been minimized.

Show comment
Hide comment
@justinbmeyer

justinbmeyer Sep 14, 2013

Contributor

There isn't a "bar" helper?

Contributor

justinbmeyer commented Sep 14, 2013

There isn't a "bar" helper?

@bmomberger-reciprocity

This comment has been minimized.

Show comment
Hide comment
@bmomberger-reciprocity

bmomberger-reciprocity Sep 14, 2013

Contributor

There isn't; that was supposed to be the "foo" helper.

Contributor

bmomberger-reciprocity commented Sep 14, 2013

There isn't; that was supposed to be the "foo" helper.

@justinbmeyer

This comment has been minimized.

Show comment
Hide comment
@justinbmeyer

justinbmeyer Sep 14, 2013

Contributor

If you check out http://jsfiddle.net/V8VvF/3/ and open the console ... you see that "foo is called" twice total. I'm not seeing the issue. I'd expect it to be called 3 times if the problem you described was happening.

Contributor

justinbmeyer commented Sep 14, 2013

If you check out http://jsfiddle.net/V8VvF/3/ and open the console ... you see that "foo is called" twice total. I'm not seeing the issue. I'd expect it to be called 3 times if the problem you described was happening.

@bmomberger-reciprocity

This comment has been minimized.

Show comment
Hide comment
@bmomberger-reciprocity

bmomberger-reciprocity Sep 14, 2013

Contributor

the second "should be" line is calling "baz" instead, which is reading from a different var. there should be only one call to "foo" after obs.quux is updated.

Contributor

bmomberger-reciprocity commented Sep 14, 2013

the second "should be" line is calling "baz" instead, which is reading from a different var. there should be only one call to "foo" after obs.quux is updated.

@justinbmeyer

This comment has been minimized.

Show comment
Hide comment
@justinbmeyer

justinbmeyer Sep 14, 2013

Contributor

Sorry, I'm an idiot. It shouldn't have been called the first time.

Contributor

justinbmeyer commented Sep 14, 2013

Sorry, I'm an idiot. It shouldn't have been called the first time.

@bmomberger-reciprocity

This comment has been minimized.

Show comment
Hide comment
@bmomberger-reciprocity

bmomberger-reciprocity Sep 14, 2013

Contributor

You're about the farthest thing from an idiot I can fathom. It wasn't called the first time, and I could have made that clearer. Here: http://jsfiddle.net/air_hadoken/V8VvF/4/

Contributor

bmomberger-reciprocity commented Sep 14, 2013

You're about the farthest thing from an idiot I can fathom. It wasn't called the first time, and I could have made that clearer. Here: http://jsfiddle.net/air_hadoken/V8VvF/4/

@justinbmeyer

This comment has been minimized.

Show comment
Hide comment
@justinbmeyer

justinbmeyer Sep 14, 2013

Contributor

Awwww Thanks! I confirmed this is a problem. This will be fixed in 1.2.

Contributor

justinbmeyer commented Sep 14, 2013

Awwww Thanks! I confirmed this is a problem. This will be fixed in 1.2.

@justinbmeyer

This comment has been minimized.

Show comment
Hide comment
@justinbmeyer

justinbmeyer Sep 14, 2013

Contributor

Ok, I figured out what happens. When is evaluating:

{{#if quux}}

It looks up quux and then creates a compute out of it. That compute is then read. When the compute is read, live binding listens to the compute changing, but when the compute is read, it ends up calling obs.attr('quxx') which also sets up a live-binding on obs's quxx property.

So when quxx changes, it triggers a redraw, but that also changes the compute, which also triggers a redraw.

If you are reading a "bound" compute, it doesn't actually read from any child observes and just uses the cached value. But the compute is not bound yet, so the read goes through to the child observes.

There's two fixes that should happen and each one would fix it.

  1. A compute should use the same batchNum as the event that triggered them. This is a hacky fix, but one that will improve performance in general.
  2. I'm not sure about the other one. We can't really avoid creating computes for arguments because this is how the two-way binding helpers works.

Might it be possible to create a "capturing" compute. That you ignore any non-bound comptues? No because you might pass a non-bound compute to the template that gets read and you want to bind on it.

Maybe it's possible to make a temporary compute that gets ignored if it's read within the master compute? These computes will be created within can.Mustache ... if they are read within the primary compute's read ... the bindings get passed through. I think this will work!

(sorry for the random musings)

Contributor

justinbmeyer commented Sep 14, 2013

Ok, I figured out what happens. When is evaluating:

{{#if quux}}

It looks up quux and then creates a compute out of it. That compute is then read. When the compute is read, live binding listens to the compute changing, but when the compute is read, it ends up calling obs.attr('quxx') which also sets up a live-binding on obs's quxx property.

So when quxx changes, it triggers a redraw, but that also changes the compute, which also triggers a redraw.

If you are reading a "bound" compute, it doesn't actually read from any child observes and just uses the cached value. But the compute is not bound yet, so the read goes through to the child observes.

There's two fixes that should happen and each one would fix it.

  1. A compute should use the same batchNum as the event that triggered them. This is a hacky fix, but one that will improve performance in general.
  2. I'm not sure about the other one. We can't really avoid creating computes for arguments because this is how the two-way binding helpers works.

Might it be possible to create a "capturing" compute. That you ignore any non-bound comptues? No because you might pass a non-bound compute to the template that gets read and you want to bind on it.

Maybe it's possible to make a temporary compute that gets ignored if it's read within the master compute? These computes will be created within can.Mustache ... if they are read within the primary compute's read ... the bindings get passed through. I think this will work!

(sorry for the random musings)

@justinbmeyer

This comment has been minimized.

Show comment
Hide comment
@justinbmeyer

justinbmeyer Sep 15, 2013

Contributor

Another, possibility is to automatically prevent indicating that computes created within another compute are read.

I'll put that another way ... the outer compute in:

var me = new can.Map({name: "Justin"});

var outer = can.compute(function(){

  var inner = can.compute(function(){
    return me.attr("name")
  })

  return inner() + " might have figured this out"
})

will not listen to changes in inner.

The problem with this might be comptues that are used many times. inner might be called many times. Binding on it, and forcing it to cache it's value could have performance improvements.

What's really needed is is a way to say "you will be bound to" to the inner compute and somehow prevent any reads within that.

Maybe if there is a __reading ... a compute being read should add itself to the ___reading, basically preventing other reads. Ah, I think this is it!

Contributor

justinbmeyer commented Sep 15, 2013

Another, possibility is to automatically prevent indicating that computes created within another compute are read.

I'll put that another way ... the outer compute in:

var me = new can.Map({name: "Justin"});

var outer = can.compute(function(){

  var inner = can.compute(function(){
    return me.attr("name")
  })

  return inner() + " might have figured this out"
})

will not listen to changes in inner.

The problem with this might be comptues that are used many times. inner might be called many times. Binding on it, and forcing it to cache it's value could have performance improvements.

What's really needed is is a way to say "you will be bound to" to the inner compute and somehow prevent any reads within that.

Maybe if there is a __reading ... a compute being read should add itself to the ___reading, basically preventing other reads. Ah, I think this is it!

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