Don't call filters/functions twice when repeated.

[nevir]

Fixes #174

However, before this is committable:

• ObserverTransform#value_ seems wrong. Should I add an ObserverTransform#getValue?

Also, this raises a related question:

• Should we really be binding at all to repeated values via functions?

---

Why does this work?

1. In TemplateIterator#updateDependencies, we create and open the ObserverTransform for the filtered value.
2. The ObserverTransform reads the value of the function in open
3. updateDependencies -> updateIteratedValue.
4. In updateIteratedValue, we read the value via discardChanges; which causes the function to be called again.
5. Finally, the array observer is set up.

[sorvell]

@jmesserly It'd be great to get this fixed. Would you mind reviewing? Thanks!

[nevir]

Also, I'm having a hell of a time writing a test case to repro this (since the {{foo in foos}} syntax isn't directly supported by this lib) - leaving that out for now

[jmesserly]

(looking)

[jmesserly]

hmmm. Yeah, agree with you, I'm not sure about reading value_ like that. It looks like the code is designed to handle if and repeat on the same template, e.g. like this test https://github.com/Polymer/TemplateBinding/blob/master/tests/tests.js#L592 ... if we fail to call discardChanges() to compute the most recent value, it might not do the right thing for a combined if/repeat case. (Although none of the tests seem to catch that.)

Possibly what needs to happen here is to split updateIteratedValue into two methods, one that is called when ifValue changes and one that is called when repeatValue changes. That way we can recompute the other (if needed) without recomputing the value that was just delivered to us.

FYI, a test can probably be done using a custom bindingDelegate, e.g. https://github.com/Polymer/TemplateBinding/blob/master/tests/tests.js#L2620

btw @nevir I'm happy to dig into this more if you'd like & help out with testing. Let me know!

[jmesserly]

fyi, think I have an updated version, working on tests to cover the new cases & the ones I was worried about regressing.

[jmesserly]

[edit: ignore this, see next comment]

hmmm. Thinking on this more, I wonder if we should try mitigate this from the ObserverTransform side too. It could avoid calling getValueFn if `this.observable.discardChanges()returned the same value. That would require one extra field on ObserverTransform, but it has the benefit that anyone using it doesn't need to be careful about calling discardChanges triggering too much recomputation. Otherwise, we have to be really careful to avoid discardChanges calls when we already know the value (e.g. because it was just handed to us viaopen`)

[jmesserly]

oops, we can't mitigate this in ObserverTransform.discardChanges because we'd need deep equality (expensive). Thanks to the "reduce" test case for reminding me of this :)

[jmesserly]

My alternate approach is in https://github.com/Polymer/TemplateBinding/pull/180 ... appreciate your thoughts and comments