unwanted side effect when calling Tag.updateValue during a call to setProperty

[johan-ohrn]

I've stumbled upon this "thing".

In my template I have a tag expression like this:
{^{dataGrid dataSource=ajaxResponse^items currentPage=currentPage /}}

Basically it's a tag that is supposed to render an array of items in a tabular fasion. The code is really simplified for this demo.

Initially the items array is empty and setTimeout simulates an ajax call to retrieve data.
Upon data retrieval the ajaxResponse object is observably replaced on the viewmodel:
$.observable(viewModel).setProperty("ajaxResponse", ...);

The tag listens to changes in the dataSource array and it's supposed to reset pagination to first page when the tag is refreshed with new data:

$.observe(this.tagCtx.props, "dataSource", function(e,args) {
    tagInstance.updateValue(0, 1);
})

updateValue() correctly writes the value 0 back viewModel.currentPage but as you can see in the console output the previous value is later written back to viewModel.currentPage by the setProperty method.

What's important to notice is that Tag.updateValue() is invoked during a call to setProperty().

If I observably refresh the items array directly instead of observably replacing the array entierly with setProperty the call to updateValue() work as intended:
$.observable(viewModel.ajaxResponse.items).refresh(...)

Here is a fiddle that demonstrates the problem.

From what I can tell setProperty() ends up calling onDataLinkedTagChange() which creates a hash object like {dataSource: [<new items here>], currentPage: 10}
From this point setProperty() is called for each of these keys:

// this will update currentPage
setProperty("dataSource", [<new items here>])
// this will revert currentPage
setProperty("currentPage", 10)

I would like it if only the property that was actually affected by the original call to setProperty is included in the hash object {dataSource: [<new items here>]}

[BorisMoore]

That's an interesting scenario.

JsViews looks at the bound properties, (in this case, dataSource and currentPage, from bindTo...) and works out the dependencies for those paths, (in this case ajaxResponse, ajaxResponse and currentPage) but does not track which dependencies come from which property. (Indeed, sometimes the same dependency may apply to two or more props). Now if a dependency (ajaxResponse) changes, it will as you say, call setProperty on all of them, (in this case first dataSource and then currentPage). But if the dependency (ajaxResponse) only applies to one of them (dataSource) then calling setProperty on the other (currentPage) will be a no-op, since it has not changed (10).

But in your case, you are listening to the change on dataSource, and then immediately updating currentPage (0). Now the calling code continues, moves to updating currentPage, and finds it has indeed changed (it is now 0, changing to 10) so it updates it to 10.

So the obvious fix for your example is to make the update to page 0 be async, so it happens only after the calling code has completed. The calling code will be a no-op for currentPage, and your code will then successfully update to 0.

$.observe(this.tagCtx.props, "dataSource", function(e,args) {
    setTimeout(function() { // async call
      tagInstance.updateValue(0, 1);
    });
 });

In fact there is an undocumented parameter, function updateValue(val, index, tagElse, async... which you can use, though technically it might change in the future (it is used internally)

$.observe(this.tagCtx.props, "dataSource", function(e,args) {
    tagInstance.updateValue(0, 1, 0, true); // async call
 });

[johan-ohrn]

I found a fix for the problem but I dont know if it could cause issues of it's own?
The idea is to modify the setProperty method so that it performs the no-op check before it performs the updates.
The change it self is in jsviews.js on line 3694:

// Object representation where property name is path and property value is value.
var changed = {};
for (key in path) {
    if (object[key] !== path[key]) {
        changed[key] = path[key];
    }
}
//for (key in path) {
for (key in changed) {

If possible I would prefer a solution where the consumer doesn't have to consider how jsviews work internally.
In my particular case what would happen is that the tag would render a html table and then when the setTimeout fires it would reset the pagination which would render the table all over again leading to flickering and lower performance.
Even if calling updateValue async is a viable solution I always consider synchronization using setTimeout a hackish solution.
Also I've noticed that setTimeout synchronization tends to throw developers off. Many don't understand why it's neccessary and it makes the code more complicated.

Changing a property when some other property is changed seem like a normal enough scenario in a bigger application.

What's your take on this?

[BorisMoore]

Thanks Johan. I need to finish some work in another area, first, but I'll then get back to taking a closer look at this.

[BorisMoore]

Hi Johan,

I have some issues with the approach you suggest, but it was very helpful in motivating some other alternatives I looked at. In fact there are some improvements to $.observable that I had been wanting to make, and I believe that those changes also address (as a 'spin off') your issue...

One of the of the results of those changes is that calling $.observable(...).setProperty({a: ..., b: ...}) now has a slightly different processing model: It will first update the values of all the properties (a, b, ...) then raise the observable change events for each property (a, b ...).

So in your scenario your call to update the currentPage when dataSource changes should work as intended, since it will occur after the initial updating of both dataSource and currentPage, and will not get overridden by the latter. This should work whether or not the initial call changes currentPage.

Here is an update that you can test:

jsviews_1.0.3_candidateA.zip

Let me know if it works correctly for you...

[johan-ohrn]

Thanks I'll test it next week and get back to you afterwards.

[johan-ohrn]

I have verified that this does indeed solve our problem.
My initial testing hasn't revealed any issued caused by this change. I will continue to watch and see if anything shows up.

[johan-ohrn]

I did come across an issue which I think is related to some work on computed observables.

Depending on your thoughts on this matter you might want to move this to a separate thread? Considering that this version is not yet official I post my findings here.

In our template we want to bind event handlers for keydown event on an input.
To make the handler simpler we have created a helper that filters out unwanted keys and optionally stops event bubbling.

So to invoke myhandler on keydown we use a template like this:

<input data-link="{on 'keydown' ~key.enter.stop myhandler}" />

key is a helper such as (code simplified):

$.views.helpers({
  get key() {
    var enter = function() {
      // call myhandler if pressed key is enter
    };
    enter.stop = function () {
      // call my handler if pressed key is enter and stop event bubbling
    };
    return {enter: enter};
  }
})

I found that with this update the link expression "~key.enter.stop" behaves differently.
The related code is in jquery.observable.js at line 610:

 if ($isFunction(prop)) {
	fnProp = prop;
	if (dep = prop.depends) {
		// This is a computed observable. We will observe any declared dependencies.
		if (obj._vw && obj._ocp) {
			// Observable contextual parameter, so context was ocp object. Now move context to view.data for dependencies
			obj = obj._vw; // storeView or tag (scope of contextual parameter)
			if (obj._tgId) {
				// Is a tag, so get view
				obj = obj.tagCtx.view;
			}
			obj = obj.data; // view.data
		}
		observeObjects(concat.apply([], [[obj], dependsPaths(dep, obj, callback)]));
	}
	if (!(prts[0] && (prop = fnProp.call(obj)))) {
		break;
	}
}

The value produced by the token "enter" is checked for a function, which it is. What's new is that the function is invoked directly.
Before the parser would not execute the function but would instead move on to the next token in the expression, "stop".

The code examples related to computed observables all use parentheses in the template expressions to indicate a computed observable such as

<input data-link="person.fullName()" /> 

Perhaps the detection of a computed observable could be more strict such to prevent false positives. I.e. the function must have a property (function) named "set" to be considered a computed observable?
This would allow me to write the following template expressions

var person = {
    fullname: ...
    someFunction: function() { return { prop: 1 } },
    someOtherFunction: ...
};
person.fullname.set = function() {...}
person.someOtherFunction.obj = { prop: 1 }

<!-- this is a computed observable -->
<input data-link="person.fullName()" />

<!-- this is executing a function and linking to a property on the returned object -->
<input data-link="person.someFunction().prop" />

<!-- this is linking to a property on the function -->
<input data-link="person.someOtherFunction.obj" />

Anyway in my specific case I could just change the key helper into:

$.views.helpers({
  key: {
    enter: function() {
      // call myhandler if pressed key is enter
    },
    enter_stop: function () {
      // call my handler if pressed key is enter and stop event bubbling
    }
  }
})

and my template becomes
<input data-link="{on 'keydown' ~key.enter_stop myhandler}" />

However I find the dot notation more elegant. Also you can do more powerful stuff if it's possible to differentiate between pointing out a computed observable, executing a function and pointing out a property on a function.

[BorisMoore]

Thanks again for this valuable feedback. Interesting to see how you are using JsViews, and those dot paths on functions.

The reason for the new code that invokes the function is related to supporting chained dependencies for a hierarchy of getter functions, or of VMs, such as Person, Address, Street.

In the template you might have

{^{:person().address().street()}}

But you should also be able to do:

$.observe(app, "person.address.street", handler);

which was not working as expected.

I'm looking into a fix that might be compatible with your scenario. Not sure yet...

Another related issue is that for a computed or VM which returns an array, the following will listen to both property change (setting the _people array) and array changes on that array.

But if you have a getter function, people(), then this:

$.observe(app, "people", handler); 

does not currently listen to array changes.

I am trying to make things more consistent here between getter functions and plain objects/arrays, so considering a change...

But of course these are breaking changes, - as would be your suggestion of requiring a getter function to be detected by the presence of a setter, fn.set (or perhaps a fn.get property)

I'll keep you posted when I reach some conclusions...

[johan-ohrn]

Given the person example. When a function is found (i.e. "person" in the expression "person.address.street") you could decide to execute it or not based on checking the token on the right hand side of the dot (i.e. "address").
If "address" exists as a property on the function then return it's value instead of executing person().
If not then execute person().

It is still a breaking change but it seem to me that the likelihood to cause issues is low: If the function is a getter in the first place then why would you add functions as properties to it? Even more so what's the chance a getter function such as person() when executed would return an object with a property named "address" and also have a property named "address" on it? Famous last words.
This structure just seem odd:

var person = function() {
	return {
		address: function () {
			return ...
		}
	}
}
person.address = ...

[johan-ohrn]

Hmm I was a bit hasty with my last comment.
That idea only work for path segments other than the leaf. I.e. "person.address.street" would fail because there is nothing following the "street" segment.

Given the current approach to invoke the function directly there is no way to pass a function as an argument to an other function - which is something else we're doing. I.e.

data-link="{on 'click' onClick someOtherFunction}"

onClick would call someOtherFunction when done with it's work.

[BorisMoore]

Yes, we need to be able to deal with leaf properties that are functions, too.

I have a new candidate update here: jsviews_1.0.3_candidateB.zip

With this version, you can write dependency or observe paths like this:

$.observe(object, "person().address().street()", handler)

If person etc are functions they don't need to have person.get === true. But if they do you can write the paths without parens:

$.observe(object, "person.address.street", handler)

and that will work the same.

Now, if person.get is not set to true, then you can do things like observing additional properties of the person functions:

$.observe(object, "person.someOtherFunction.obj", handler)

or in a template, write:

<input data-link="person.someOtherFunction.obj" />

In fact you do side-by-side use of both styles - 'functions as getters' with parens, and 'functions as objects', not using parens:

<input data-link="person.someOtherFunction.obj" />
<input data-link="person().name()" />

If you use compiled VMs then the compiled getters are automatically created with get set to true, so you can omit the parens in property paths. This is partly what helps preserve partial backward compatibility with these changes...

Let me know how it works for you...

[BorisMoore]

BTW I may have a modified version in a day or two, if you want to wait before trying the update.... I'm hesitating on whether to simply require parens in observer paths, and not base things at all on having/not having a get=true property. It is generally better not to have too many ways of doing the same thing, but I still have to look carefully at back compat implications...

[johan-ohrn]

I ran into this odd issue that I can reproduce but I'm having a hard time understanding why it's happening.
Save the following into a local html file and download jquery or just change the link to a cdn.
What I expect to happen is I type something into the textbox and the stop() function should be called.
For some reason the enter() function is called without any input to the textbox. It's like the initial link() call schedules an async call to the enter() function.

<html>
  <head>
    <script src="jquery-1.12.4.js"></script>
    <!--<script src="jquery.observable.js"></script>
    <script src="jsrender.js"></script>
    <script src="jquery.views.js"></script>-->
    <script src="jsviews.js"></script>
    <script type="text/javascript">
      $.views.helpers({
        key: (function () {
          var enter = function() {
            // this function is called for some reason as a result of calling $.views.templates("#mytemplate").link(".container", vm);
            debugger;
          };
          enter.stop = function() {
            debugger;
            enter();
          };
          return {
            enter
          };
        })()
      });
      
      $(document).ready(function() {
        var vm = {
          onClick: function () {
            debugger;
          }
        };
      
        /* this will somehow, somewhere do the equivalent of:
         * $(document).ready(function() {
         *    call ~key.enter()
         * });
         */
        $.views.templates("#mytemplate").link(".container", vm);
        
        $(document).ready(function() {
          // this breakpoint is triggered after ~key.enter has been called
          debugger;
        });
      });
    </script>
  </head>

  <body>
    <div class="container"></div>
  </body>
  
  <script id="mytemplate" type="text/x-jsrender">
    <input type="text" data-link="{on 'keydown' ~key.enter.stop onClick}" >
  </script>
</html>

Regarding a get=true property.
I personally think it's fine as long as the natural way is a logical way. What I mean by this is you can write arbitrary javascript expressions in a template such as {{if someArray.indexOf(whatever) !== -1}} involving both member access and function invocations and so it's easy to assume that there is a one to one mapping between javascript and the template syntax. Given this line of thought it makes sense that functions as getters/setters must be invoked with parentheses since in javascript you couldn't get the address by simpy executing person.address. You'd have to execute person().address().

However jsrender is free to make life easier for the developers. Not having to write out the parentheses looks cleaner. I think get=true does a good job compensating for this shortcoming of javascript. Especially if we back time up a bit before there were things like typescript and transpilers and native support for get/set properties forcing you to use functions for getters/setters. But I don't think the default behavior should be to treat functions as getters for these reasons.

[BorisMoore]

OK, I took a look. That strange behavior is because I had enabled data-linking and observing changes of properties on functions, so you could do:

<input data-link="person.someOtherFunction.obj" />

But as a result, JsViews is calling

$(person.someOtherFunction).on(namespace, null, evData, onDataChange);

and jQuery, in addition to attaching the listener, also treats the call $(person.someOtherFunction) as a call to invoke the function person.someOtherFunction on document ready.

In the next version I send you, I'll remove the support for listening to observable changes on a path like "person.someOtherFunction.obj", since those semantics are clearly no appropriate...

[BorisMoore]

Here is an update: jsviews_1.0.3_candidateC.zip

It should fix that behavior re document ready. You can't now observe properties of functions. You can and should use parens in observe/depends paths with chaining, such as depends = "person.address()^street()" - though you can omit parens at the leaf, and write instead depends = "person.address()^street"

[johan-ohrn]

As far as I can tell It's now working correctly.
I'll keep my eyes out and let you know if I find anything else.

[johan-ohrn]

I found an issue with when scripts are loaded separately.

If you load jquery.observable.js first and then do something like this before loading any of the other scripts

var obj = { prop: "value" };
$.observable(obj).setProperty("prop", "new value");

you get the following error Cannot read property 'asyncObserve' of undefined

[BorisMoore]

Yes, thanks again, another good catch. It should be fixed in this version:

jsviews_1.0.3_candidateD.zip

[johan-ohrn]

FYI:
I will test this new candidate release starting at tomorrow. We are preparing a release and will use the current version for it.

[BorisMoore]

See #420 for an update which supercedes candidateD above.

[johan-ohrn]

Initial testing is positive. I'll keep monitoring to see if anything shows up.

[BorisMoore]

The changes discussed above are included in the new v1.0.3 release. (Let me know if you see any problems with the update. Thanks!)