support observable view models at both the root and child context level

[mbest]

This is a major part of #321, but can be implemented separately. Depends on #483 and #484.

[mbest]

This would also benefit my repeat binding, allowing it to use a child context for each item and still update the bindings when the array is updated. Right now, I'm just using a special context variable to set up the dependency.

[mbest]

Here's a little history on this topic:

I first noticed that the binding system would handle an observable view model when working on my first iteration of independent bindings. At that time, I wasn't sure what to do with an observable view model. Since there wasn't an easy way to support it with independent bindings, I basically disabled it--I still unwrapped it, but wouldn't update any bindings if it was updated.

When I started working on my second iteration of independent bindings, I was ready to just drop support for observable view models. But then I decided to see if people might actually like the feature. So I posted a message about it on the forums. Since I got only positive feedback, I decided it was important enough to keep supporting.

The "feature" was originally added to Knockout with this commit, but it isn't mentioned in the commit notes. Nor is there a spec to test it. So at this point, it's unsupported (and maybe accidental?).

In this issue, I propose that we make it officially supported and expand its scope to cover both the root and child context levels.

[worldspawn]

+1

[drosen0]

+1

[mbest]

The current observable view model system has a problem because the viewModel and bindingContext objects can change, but binding's init functions don't get called again and thus don't get the updated objects.

Demonstration: http://jsfiddle.net/mbest/gdQFK/

We can fix part of this by making sure that the bindingContext object doesn't change, which is how it works in my Knockout fork. But the viewModel argument isn't as simple. It seems to me that the only way to make it work would require a breaking change to the binding handler interface. The simplest way would be to remove the viewModel parameter and expect the handlers to use the $data property of the context to access the view model. Without making it a breaking change, we can simply deprecate using that value.

[mbest]

The current (2.x) implementation of observable view models work like this: The view model is unwrapped by the binding processor within the ko.computed that also creates a binding context object, parses the bindings, and calls the binding handlers. Thus whenever the view model is updated, all the bindings are updated. As I mentioned before, the init functions of the bindings aren't called, and so they are left with stale data.

This system only works when the binding context can be created within the binding processor, in other words, for a top-level view model. Child observable view models (using with for example) must be updated by re-rendering the HTML and re-binding.

Thus the behavior of root-level and child-level observable view models is different:

1. At the root level, updating the view model will update each individual binding (just like any other observable dependency).
2. At the child level, updating the view model will clear and re-add the HTML, and then apply bindings with the new view model.

[mbest]

To officially support observable view models (in version 3.0), I think we should make sure support is consistent between the child and root levels. Of course, this gives us two options:

1. Use the 2.x child-level method. This behavior is already widely used through the with and template bindings, so it should be familiar to people. On the other hand, people are often surprised that updating a view model re-renders the HTML. This option should be fairly easy to implement: if given an observable view model, treat the given element as a template.
2. Use the 2.x root-level method. This behavior is probably what most people would expect--that the individual bindings are updated. But this option is more difficult to implement. It would require adding a new ko.computed to re-parse the bindings, and adding a dependency on the view model in the ko.computed of each binding.

In my Knockout fork, I chose to implement option 2. I have ko.bindingContext track observable view models and expose a observable for the binding processor to get notifications. Here's my withlight binding that can bind to an observable view model:

ko.bindingHandlers['withlight'] = {
    'flags': bindingFlags_contentBind | bindingFlags_canUseVirtual,
    'init': function(element, valueAccessor, allBindingsAccessor, viewModel, bindingContext) {
        var innerContext = bindingContext['createChildContext'](function() {
                return ko.utils.unwrapObservable(valueAccessor());
            });
        ko.applyBindingsToDescendants(innerContext, element);
    }
};

[SteveSanderson]

Can you help me clarify exactly what change of behavior we should expect to see if we go for option 2? I'm unsure if this would be a further breaking change, but it sounds like it might be.

My initial concern is with scenarios like the following: http://jsfiddle.net/ep27E/

In this scenario, the developer is benefitting from (and relying on) the existing behavior where the DOM is rebuilt. Would option 2 mean that old validation messages continue to appear even after the viewmodel is changed? If so, option 1 sounds much more promising, as not only is it easier to implement, but also the behavior is less breaking, since top-level observable viewmodels are far less common and haven't had such a clearly defined behavior in the past.

[mbest]

Going with option 2 wouldn't necessitate changing the with binding but would make it possible for custom bindings, such as the above withlight, to apply an observable child view model without having to re-render the DOM.

I tried your example, but I was not able to test the scenario you described. It always cleared the message as soon as I clicked on the drop-down.

[steveluscher]

Perhaps we could allow developers to install a callback that gets run when the view model is about to change. There, they could tear down any modifications that were made outside of Knockout, such as in @SteveSanderson's jquery.validate example, or in the common case of apps that use jquery.draggable, jquery.droppable, jquery.resizable, etc.

<!-- ko with: vm, subscribe: teardownValidations -->…<!-- /ko -->

[mbest]

I decided to put together the code changes for both options. Implementing option 1 (9a086db) was pretty straightforward and simple, and for option 2 (7ff1358), I was able to quickly adjust the code I already had in my fork.

Both versions add the same three tests, but they have slight differences. The latter two tests for option 1 pass plain values to bindingContext.createChildContext and bindingContext.extend; whereas for option 2, they pass functions that return the value.

[mbest]

I definitely want to see something like my option 2 included in Knockout. There's a lot of demand for using control-flow bindings such as with and if without having to re-render the HTML, which this feature would make possible. On the other hand, making it the default behavior for those bindings would be a breaking change, maybe one too many for this release.

Having been thinking about this over the last week, I want to include both options, with option 1 being the default behavior when providing an observable view model to ko.applyBindings. This keeps the default behavior consistent, and provides a means for custom bindings, etc. to do control-flow without re-rendering.

[mbest]

Code attached.

[SteveSanderson]

Thanks for proposing this implementation! I expect that we will use most of this, but I'd like to suggest refining the design a bit further.

I've been thinking this through over and over, because it feels like whichever option we take here, there are potential pitfalls and drawbacks. And taking no action isn't an option either :)

Both options 1 and 2 have issues:

• Always replacing nodes when viewmodels change (option 1), at the top level, would be a breaking change. I know we've never officially supported top-level observable viewmodels, but we do know that people are using that technique. Breaking this by default isn't consistent with our goals.
• Always retaining nodes when viewmodels change (option 2), for with/if etc., would be a very severe breaking change as discussed above.
• If we take no action in the v3 branch, this would prevent top-level observable view models, severely breaking real apps
• Permitting both strategies (1 and 2) via some kind of binding option complicates things for anyone trying to understand KO's binding mechanism or write custom bindings that work universally, though maybe to some extent this is unavoidable

After some consideration, my preference would be to rank the design objectives as follows:

• Firstly, backward compatibility
  • The default behavior would therefore be like in 2.x - a top-level observable view model retains elements when rebinding, and control-flow bindings continue to use template-style element replacement.
  • This can be achieved using the code Michael has already supplied with this pull request
• Secondly, making observable view models a real official thing, with the ability to retain elements both at the top level (like v2.x already does) and at child levels (e.g., with with), like Michael proposes
  • No new APIs are needed to expose this functionality at the top level - it can work the same as in 2.x
  • For custom bindings that want to rebind their children, perhaps we could create a ko.reapplyBindingsToDescendants function that changes the descendants' viewmodel. Again, this can be implemented straightforwardly using the code Michael has already supplied. This makes it trivial to implement a custom withLight binding that retains child nodes forever.
  • In the future, we could consider adding a retainNodes option to the with binding, but there's no need to do that for 3.0. (It would be preferable not to do that until real-world usage of reapplyBindingsToDescendants has verified the usefulness of this pattern in production apps.)
• Thirdly, avoiding expanding the API surface area more than necessary, minimising the amount of new code, and trying not to include obscure new options that have to be documented and learned by KO users
  • The above approach doesn't require us to export ko.bindingContext or vary the behavior of applyBindings depending on whether the parameter is a binding context or not (which would be quite obscure).
  • Also note that there's no need to support option 1 (template-style rebinding) at the top level, since that was never built into KO 2.x, we don't really know that anyone wants to do that, and if they did they could always just wrap their whole UI inside a with.

Apologies that this is such a big dump of thoughts! It's a tricky design issue.

Michael, if you want to proceed with this approach, I think the considerable majority of your existing pull request code will be applicable, and we can just tweak the API surface a bit. I'm happy to have a go at implementing this, or I'm happy if you want to, but I'll wait for your views on this first.

[mbest]

Steve, you're right that there are no easy answers here.

I suggested making option 1 the default because it's a bit easier to support officially, as it works correctly with what we've supported for Knockout in the past. With this approach, any binding that works under with will also work under a top-level observable view model.

I would prefer we move towards option 2, though, so I'm happy to see you support that as well. The biggest challenge of this approach is that certain binding handler interfaces and approaches are incompatible with it.

1. A binding handler that reads the view model object in init, such as event, won't see any updates through the viewModel parameter, as I mentioned earlier.
2. A binding handler that binds its element's descendants in init (like the examples here) will prevent those descendants from seeing updates to the view model.
3. A binding handler that accesses its valueAccessor in init outside of a ko.computed won't be notified when the view model is updated.

If we are to make observable view models a real official thing, we need to solve the above problems by changing the appropriate interfaces and/or documentation.

1. We should remove the viewModel parameter to init (and probably to update so that it matches), and replace it with bindingContext. Since the latter is used much more frequently than the former, we can minimize the compatibility issue by passing the binding context as both the fourth and fifth parameters.
2. My solution is to expand both BindingContext#createChildContext and BindingContext#extend to accept a function (can also be an observable) that returns the appropriate value. You can see this in my withlight example above. You suggested ko.reapplyBindingsToDescendants that I think is supposed to solve this problem also, but I'm not exactly sure how it would work. Of course, with any solution, the user will need to update their binding handler code to use the new interface.
3. For this, the user will need to use an appropriate method to track changes to the binding value caused by updating the view model, which is something that they probably didn't consider before. We'll need to make this information clear in the documentation.

[SteveSanderson]

Thanks for the additional info. This is not an easy one.

How far off do you think we are from being able to recreate the 2.x behavior in the 3.0 code? To avoid pushing 3.0 back further and introduction potential extra disincentives for people to upgrade, it would be great if we could re-establish at least the top-level observable view model behavior, even if it's not officially documented and supported. Then we can look at a bigger change to the binding interface in the future.

In the future, I'd love us to review the whole custom bindings API entirely, and possibly produce an alternative that is a bit more object-oriented (so each usage is regular instance that can hold its own data, and you can just inherit from another binding and have it work as you expect) and get rid of the cruft around semi-obsolete parameters like viewModel. Ideally this would also come at the same time as a notion of "disposing" bindings - people have been asking for a way to unbind for ages. Perhaps the old APIs could be retained for backward compatibility, but removed from docs, and the implementation just invoking the newer APIs. All of this would be too much to shove into v3.0 but would be great to have on the roadmap.

What do you think?

[steveluscher]

I'd love us to review the whole custom bindings API entirely, and possibly produce an alternative that is a bit more object-oriented (so each usage is regular instance that can hold its own data…

That would be ko.utils.domData.set(element, "amazing!")

[mbest]

The code was very close to ready. I removed the special-case code for top-level observable view models and made it so that the observable is instead passed directly to ko.BindingContext. Otherwise I made only minor changes. This matches the 2.x behavior in all of the important ways. The differences are, hopefully, improvements:

1. The binding context instance is maintained through model updates. So, handlers can depend on bindingContext.$data to always have the latest model value. The event and submit handlers now use bindingContext.$data.
2. Child and extended contexts automatically get a dependency on the parent context and are updated whenever the parent model updates. So $parent, $root, etc. are kept up-to-date.
3. Handlers get a dependency on the model through valueAccessor, which means that the init function can also get notified of updates.
4. bindingContext#createChildContext and bindingContext#extend accept a function/observable that returns the value. This allows the context to always be up-to-date when the model changes.
5. The binding context contains a $dataFn property, a function that returns the $data value and registers a dependency on the context if applicable. This could be used by a handler to get notified of view-model updates even when it doesn't access valueAccessor.

I replaced the attached branch so that we have a cleaner history. The previous code is at 485-observable-view-model-combined.

[SteveSanderson]

Makes sense and sounds right. Thanks for supplying the new version of the code as a clean diff on top of the existing v3 branch.

I'm in the process of reading through it to understand the new mechanism, and hopefully will merge this soon.

[SteveSanderson]

Thanks again for providing this!

I've been trying to understand the bindings updating mechanism, but to be honest I'm finding it super hard to read, because there are lots of moving parts with generic names like extendCallback, bindingsUpdater, getValueAccessor without any comments, so the only way to understand their role is to keep searching back and forth through the source, finding each place where they are used, and trying to reverse engineer and memorize the intent. For example, I'm trying to answer questions like "what can cause thing X to get re-evaluated?" and "why does this function return another function?"

I'm sure the actual logic and functionality is great, and no doubt I could eventually work out what each part of it does and what the design tradeoffs were, but it's not hugely efficient for me and all future readers of the KO source to each do that, when the info is already in your head :) Do you think you'd be able to add brief comments to each of the new bits of the mechanism that you've added, and where applicable give the new things more explicit (and possibly longer) names that clarify when and how they make a difference?

Again, this is not a criticism of the implementation at all - just a note to try to ensure that KO's binding internals remain clear and explicit enough that a newcomer could read the implementation and feel confident that they've understood the intent!

[mbest]

I took your advice and added a bunch of comments for the new code.

After going through that process, I decided to remove the changes from bindingProvider.js and keep all of the dependency logic within bindingAttributeSyntax.js.

Then I went for a walk and realized that it needs to be easier to add custom properties, such as $index to a child context, and that the existing method would break for nested foreach loops (the inner $index value would get clobbered). I wrote up the test first, which didn't do what I expected. Then I figured out that bindings in re-written templates weren't being updated because they had no bindingUpdater. I needed to go back to sending a function into the binding processor instead of just the bindings themselves.

So I hope that all makes sense. I expect you'll have questions and comments as you continue to go through the code. I didn't rename any variables because I don't think I'm very good at make those long names, but feel free to do so as you see fit.

[mbest]

I'm trying to answer questions like "what can cause thing X to get re-evaluated?"

That's always one of the tricky tasks of an observer-based system, but hopefully in this case, we can make it as obvious as possible.

The 2.x system for observable view models had one computed observable that would update the binding context (by creating a new instance), get the bindings, and update the binding handlers. In 3.x we had already separated those steps, with a computed observable only used for updating the bindings handlers.

To support observable view models in 3.x, we need to re-enable updating the binding context and bindings. My implementation uses two new computed observables for that, giving a hierarchy of three computed observables:

1. Update binding context, per view model (bindingContext._subscribable)
2. Get bindings from provider, per node (bindingsUpdater)
3. Update binding handlers, per binding

Each level is dependent on the one before it, with the binding context dependent on the view model (and/or a parent context). So the dependency tree would look something like this (arrows show the direction of updates):

[SteveSanderson]

Thanks very much for the additional comments - those are hugely useful. And thanks for the excellent diagram too! Having been over this a few times now, I'm reasonably satisfied that I can make sense of the changes and the overall approach you've taken.

I would admit I still feel some uncertainty about the tradeoff we're taking here - the significant complications to the binding system (e.g., the three layers of nested computables) could make maintenance and extension more complex or limited in the future, or be a potential source of bugs. This extra machinery and observably changing viewmodels also increases the burden on any developer who wants to write custom extensions to the binding system, or unusually sophisticated custom bindings. It's not completely clear that the benefit of this functionality, which relatively few KO developers have asked for, warrants it.

Having said all that, I really do appreciate that you've made every effort to review and refactor the implementation multiple times in response to feedback, answered all questions, and have documented the code well and provided a good set of specs. You've distilled the implementation down as far as the design allows. I'm happy to trust your judgement on this, so if you feel satisfied that it doesn't risk significant performance or memory costs, is not likely to be a major source of edge-case bugs, and you judge that the value to developers outweighs any ongoing maintenance costs, then please go ahead and merge this to version-3.0.0-development whenever convenient (or tell me and I'll do it).

Thanks again for your flexibility and patience in reviewing and modifying this change with me!

[mbest]

Thanks again, Steve. I've merged this code in now.