gvas / knockout-jqueryui

Knockout bindings for the jQuery UI widgets.
http://gvas.github.com/knockout-jqueryui/
MIT License
103 stars 38 forks source link

Options don't seem to take expressions #10

Closed juuxstar closed 10 years ago

juuxstar commented 11 years ago

So my specific use case is that I want a dialog title to be bound to not just an observable but an expression referencing an observable.

See: http://codepen.io/anon/pen/gKpin

Note the two dialog popups. One one, you can change the model value (in this example, change the value of the top input box). Upon blur, the model value is updated which updates one of the dialog box titles (the one bound straight to the model observable) but the other dialog title does not change because it is bound to an entire expression.

Upon cursory look into the code, it would seem that the culprit is the subscribeToObservableOptions() function. The code checks to see if the given property is an observable and only then creates a separate subscription (via the computed) to monitor changes to that property. I do see how that's an efficient idea.

However, if that property value is as a result of an expression, then it doesn't come up as an observable and hence no binding. A change to it though does trigger an update to the entire binding so I'm thinking that an update method could be written that calls Jquery setOption with all the options in valueAccessor(). I grant you that since there's no obvious way to tell which part of the bound value has changed (ie. which option) then all the options would have to get updated even if only changed. Not terribly efficient.

Thoughts?

BTW. In the subscribeToObservableOptions() function, you could use Object.getOwnPropertyNames(options) on the for loop and get rid of the options.hasOwnProperty(prop).

gvas commented 11 years ago

I think knockout's expression rewriting could be used to support expressions in the options. But, to be honest, it's currently over my head. Maybe I'll look into it later.

As for using getOwnPropertyNames instead of hasOwnProperty, it wouldn't work with old browsers (i.e. IE < 9).

gvas commented 10 years ago

Normally knockout tries to invoke the binding's update() method if the binding's value changes. My binding factory doesn't create that method on the bindings, instead it creates direct subscriptions to its options but only if they are observables. This technique has its advantages, but it results in dropping support for expressions. I think I should change this behavior because the update() method is more ko-ish. It looks like that the tradeoff is that the bindings' options won't be updated independently neither from each other nor from the other bindings (in case of ko < v3). I should check the performance degradation.

gvas commented 10 years ago

I've added support for expressions in the latest release.