BorisMoore / jsviews

Interactive data-driven views, MVVM and MVP, built on top of JsRender templates
http://www.jsviews.com/#jsviews
MIT License
856 stars 130 forks source link

linked tag property changed handler called when it shouldn't #440

Closed johan-ohrn closed 4 years ago

johan-ohrn commented 4 years ago

Run this fiddle with the console open and you'll see that externally updating one of the two bound properties actually triggers setValue to be called for both properties even though only one of them was changed. What's worse is that I can't determine if the value was actually changed or not so any code I put in setValue would execute needlessly.

$.views.tags({
  mytag: {
    bindTo:["prop1", "prop2"],
    template: "",
    setValue: function(value, index, elseBlock) {
      if (index == 0)
        console.log("prop1 changed from " + value + " to " +this.tagCtx.props.prop1);
      else if (index == 1)
        console.log("prop2 changed from " + value + " to " +this.tagCtx.props.prop2);
     }
  }
});

var vm = {
  prop1:"a",
  prop2:"b"
};
$.templates("{^{mytag prop1=prop1 prop2=prop2 /}}").link("#ph", vm);

setTimeout(function() {
  console.log("timeout..")
  $.observable(vm).setProperty("prop1", "blah")
});

Usually I prefer to manually attach/detach $.observe listeners like this fiddle. Here you'll notice that only the event for the changed property is triggered and I have access to both the old value and the new value.

$.views.tags({
  mytag: {
    bindTo:["prop1", "prop2"],
    template: "",
    init: function() {
      $.observe(this.tagCtx.props, 'prop1', this.onProp1Change);
      $.observe(this.tagCtx.props, 'prop2', this.onProp2Change);
    },
    onProp1Change: function(e, ev) {
      console.log("prop1 changed from " + ev.oldValue + " to " + ev.value);
    },
    onProp2Change: function(e, ev) {
      console.log("prop2 changed from " + ev.oldValue + " to " + ev.value);
    }
  }
});

var vm = {
  prop1:"a",
  prop2:"b"
};
$.templates("{^{mytag prop1=prop1 prop2=prop2 /}}").link("#ph", vm);

setTimeout(function() {
  console.log("timeout..")
  $.observable(vm).setProperty("prop1", "blah")
});

I know I've asked about this part before but I want to reiterate again, this time with some ideas. Anyway I'm taking option 2 one step further as in this fiddle you can see how I introduced a helper function on prop2. I've deliberately named it scramble to make it obvious that it does have side effects. When prop1 is changed observably it triggers the whole tag to re-link and re-evaluate all it's bindings. This is giving me quite some headache.

$.views.tags({
  mytag: {
    bindTo:["prop1", "prop2"],
    template: "",
    init: function() {
      $.observe(this.tagCtx.props, 'prop1', this.onProp1Change);
      $.observe(this.tagCtx.props, 'prop2', this.onProp2Change);
    },
    onProp1Change: function(e, ev) {
      console.log("prop1 changed from " + ev.oldValue + " to " + ev.value);
    },
    onProp2Change: function(e, ev) {
      console.log("prop2 changed from " + ev.oldValue + " to " + ev.value);
    }
  }
});

$.views.helpers({
  scramble: function(v) {
    return v.split("").sort(function(){return Math.random() <0.5 ? -1 : 1}).join("");
  }
});

var vm = {
  prop1:"hello",
  prop2:"world"
};
$.templates("{^{mytag prop1=prop1 prop2=~scramble(prop2) /}}").link("#ph", vm);

setTimeout(function() {
  console.log("timeout..")
  $.observable(vm).setProperty("prop1", "blah")
});

This is a stack trace as seen by setting a breakpoint in the onProp2Change handler

onProp2Change ((index):45)
onDataChange (jsviews.js:3087)
dispatch (jquery-1.9.1.js:3074)
elemData.handle (jquery-1.9.1.js:2750)
trigger (jquery-1.9.1.js:2986)
triggerHandler (jquery-1.9.1.js:3683)
_trigger (jsviews.js:3882)
batchTrigger (jsviews.js:3223)
setProperty (jsviews.js:3802)
mergeCtxs (jsviews.js:6425)
onDataLinkedTagChange (jsviews.js:4524)
handler (jsviews.js:5925)
onDataChange (jsviews.js:3087)
dispatch (jquery-1.9.1.js:3074)
elemData.handle (jquery-1.9.1.js:2750)
trigger (jquery-1.9.1.js:2986)
triggerHandler (jquery-1.9.1.js:3683)
_trigger (jsviews.js:3882)
_setProperty (jsviews.js:3862)
setProperty (jsviews.js:3813)
(anonymous) ((index):64)
setTimeout (async)
(anonymous) ((index):62)
dispatch (jquery-1.9.1.js:3074)
elemData.handle (jquery-1.9.1.js:2750)
load (async)
add (jquery-1.9.1.js:2796)
(anonymous) (jquery-1.9.1.js:3622)
each (jquery-1.9.1.js:648)
each (jquery-1.9.1.js:270)
on (jquery-1.9.1.js:3621)
jQuery.fn.<computed> (jquery-1.9.1.js:7402)
jQuery.fn.load (jquery-1.9.1.js:7543)
(anonymous) ((index):31)

1) onDataLinkedTagChange(ev, eventArgs) is called and eventArgs contains the name of the property that was actually changed. In this case prop1. 2) onDataLinkedTagChange executes this sourceValue = linkFn(source, view, $sub); which is responsible for re-evaluating all the bindings. In my case it looks like this:

function anonymous(data,view,j,u) {
 // unnamed 1 mytag
return [
{view:view,content:false,tmpl:false,
  params:{args:[],
  props:{'prop1':'prop1','prop2':'~scramble(prop2)'}},
  args:[],
  props:{'prop1':data.prop1,'prop2':view.ctxPrm("scramble")(data.prop2)}}]
}

3) onDataLinkedTagChange goes on to call mergeCtxs(tag, sourceValue, forceUpdate); 4) mergeCtxs calls $observable(tagCtx.props).setProperty(newTagCtx.props); which will trigger calling onProp2Change because onDataLinkedTagChange re-evaluated the links in step 2 and the scramble method returned a new value.

One thing that I'm hoping would be easy to fix is that onDataLinkedTagChange could pass along the name of the prop that actually did change when calling mergeCtxs such that it could update the property that did change and nothing else. At least this prevents any change handlers from executing where you don't expect them to.

Next thing is the fact that all linked properties are re-evaluated and not just the one that changed. One way to prevent needless execution would be to change the way the compiled link function is generated so that links are evaluated lazily such as this:

function anonymous(data,view,j,u) {
 // unnamed 1 mytag
return [
{view:view,content:false,tmpl:false,
  params:{args:[],
  props:{'prop1':'prop1','prop2':'~scramble(prop2)'}},
  args:[],
  props:{'prop1':() =>data.prop1, 'prop2':() =>view.ctxPrm("scramble")(data.prop2)}}]
}

I'm hoping such a change wouldn't be all to hard to implement. I can't imagine that the added overhead of calling a function would give any performance penalty nor should it add much to the code size.

The benefit would be way more efficient templates. Some of the problems I've noticed during the course of our project is related to this. The fact that you expect one change handler to be triggered but some other change handler also trigger. If these change handlers in turn trigger other change handlers the code quickly becomes very complex and I find that we have to write quite a lot of code to try to ignore false change events and the like. Aside from less spaghetti code related to the change handlers, templates would also become more performant. In this contrived example nothing happens when prop2 is changed falsily but imagine that this tag was a root tag which contains a whole tree of other tags and changing prop2 as happens in this example would cause the whole tag tree to re-render and execute a bunch of logic.

So whatever minute performance impact adding lazyness to the compiled link function would incur it will most definitely be gained back by more efficient templates.

johan-ohrn commented 3 years ago

Thanks for commenting. Yes it's more of a "nice to have" than something really important. Better leave it as is if complexity is high and time is low. I think you can close this issue now.