Skip to content

Commit

Permalink
Make sure all descendant bindings of if, with, etc. have a depend…
Browse files Browse the repository at this point in the history
…ency on the parent condition so that updates happen in the right order when using deferred updates.
  • Loading branch information
mbest committed May 2, 2017
1 parent 89617f7 commit 554022b
Show file tree
Hide file tree
Showing 5 changed files with 87 additions and 47 deletions.
45 changes: 36 additions & 9 deletions spec/asyncBindingBehaviors.js
Original file line number Diff line number Diff line change
Expand Up @@ -152,23 +152,50 @@ describe("Deferred bindings", function() {
});

it('Should update "if" binding before descendant bindings', function() {
// Based on example at http://stackoverflow.com/q/43341484/1287183
testNode.innerHTML = '<div data-bind="if: hasAddress()"><div data-bind="text: address().street"></div></div>';
// Based on example at https://github.com/knockout/knockout/pull/2226
testNode.innerHTML = '<div data-bind="if: hasAddress()"><span data-bind="text: streetNumber().toLowerCase()"></span> <span data-bind="text: street().toLowerCase()"></span></div>';
var vm = {
address: ko.observable(),
hasAddress: ko.pureComputed(function () { return vm.address() != null; })
street: ko.observable(),
streetNumber: ko.observable(),
hasAddress: ko.pureComputed(function () { return vm.streetNumber() && vm.street(); })
};

ko.applyBindings(vm, testNode);
jasmine.Clock.tick(1);
expect(testNode.childNodes[0]).toContainHtml('');
expect(testNode.childNodes[0]).toContainText('');

vm.address({street: '123 my street'});
vm.street('my street');
vm.streetNumber('123');
jasmine.Clock.tick(1);
expect(testNode.childNodes[0]).toContainHtml('<div data-bind="text: address().street">123 my street</div>');
expect(testNode.childNodes[0]).toContainText('123 my street');

vm.address(null);
vm.street(null);
vm.streetNumber(null);
jasmine.Clock.tick(1);
expect(testNode.childNodes[0]).toContainHtml('');
expect(testNode.childNodes[0]).toContainText('');
});

it('Should update "with" binding before descendant bindings', function() {
// Based on example at https://github.com/knockout/knockout/pull/2226
testNode.innerHTML = '<div data-bind="with: hasAddress()"><span data-bind="text: $parent.streetNumber().toLowerCase()"></span> <span data-bind="text: $parent.street().toLowerCase()"></span></div>';
var vm = {
street: ko.observable(),
streetNumber: ko.observable(),
hasAddress: ko.pureComputed(function () { return vm.streetNumber() && vm.street(); })
};

ko.applyBindings(vm, testNode);
jasmine.Clock.tick(1);
expect(testNode.childNodes[0]).toContainText('');

vm.street('my street');
vm.streetNumber('123');
jasmine.Clock.tick(1);
expect(testNode.childNodes[0]).toContainText('123 my street');

vm.street(null);
vm.streetNumber(null);
jasmine.Clock.tick(1);
expect(testNode.childNodes[0]).toContainText('');
});
});
10 changes: 8 additions & 2 deletions spec/defaultBindings/withBehaviors.js
Original file line number Diff line number Diff line change
Expand Up @@ -178,19 +178,25 @@ describe('Binding: With', function() {
});

it('Should provide access to an observable viewModel through $rawData', function() {
testNode.innerHTML = "<div data-bind='with: item'><input data-bind='value: $rawData'/></div>";
testNode.innerHTML = "<div data-bind='with: item'><input data-bind='value: $rawData'/><div data-bind='text: $data'></div></div>";
var item = ko.observable('one');
ko.applyBindings({ item: item }, testNode);
expect(item.getSubscriptionsCount('change')).toEqual(2); // only subscriptions are the with and value bindings
expect(item.getSubscriptionsCount('change')).toEqual(3); // subscriptions are the with and value bindings, and the binding context
expect(testNode.childNodes[0]).toHaveValues(['one']);
expect(testNode.childNodes[0]).toContainText('one');

// Should update observable when input is changed
testNode.childNodes[0].childNodes[0].value = 'two';
ko.utils.triggerEvent(testNode.childNodes[0].childNodes[0], "change");
expect(item()).toEqual('two');
expect(testNode.childNodes[0]).toContainText('two');

// Should update the input when the observable changes
item('three');
expect(testNode.childNodes[0]).toHaveValues(['three']);
expect(testNode.childNodes[0]).toContainText('three');

// subscription count is stable
expect(item.getSubscriptionsCount('change')).toEqual(3);
});
});
2 changes: 1 addition & 1 deletion spec/lib/jasmine.extensions.js
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,7 @@ jasmine.Matchers.prototype.toHaveTexts = function (expectedTexts) {
};

jasmine.Matchers.prototype.toHaveValues = function (expectedValues) {
var values = ko.utils.arrayMap(this.actual.childNodes, function (node) { return node.value; });
var values = ko.utils.arrayFilter(ko.utils.arrayMap(this.actual.childNodes, function (node) { return node.value; }), function (value) { return value !== undefined; });
this.actual = values; // Fix explanatory message
return this.env.equals_(values, expectedValues);
};
Expand Down
24 changes: 16 additions & 8 deletions src/binding/bindingAttributeSyntax.js
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,8 @@
return ko.bindingHandlers[bindingKey];
};

var inheritParentVm = {};

// The ko.bindingContext constructor is only called directly to create the root context. For child
// contexts, use bindingContext.createChildContext or bindingContext.extend.
ko.bindingContext = function(dataItemOrAccessor, parentContext, dataItemAlias, extendCallback, options) {
Expand All @@ -30,7 +32,7 @@
// we call the function to retrieve the view model. If the function accesses any observables or returns
// an observable, the dependency is tracked, and those observables can later cause the binding
// context to be updated.
var dataItemOrObservable = isFunc ? dataItemOrAccessor() : dataItemOrAccessor,
var dataItemOrObservable = isFunc ? realDataItemOrAccessor() : realDataItemOrAccessor,
dataItem = ko.utils.unwrapObservable(dataItemOrObservable);

if (parentContext) {
Expand All @@ -53,8 +55,14 @@
// See https://github.com/SteveSanderson/knockout/issues/490
self['ko'] = ko;
}
self['$rawData'] = dataItemOrObservable;
self['$data'] = dataItem;

if (shouldInheritData) {
dataItem = self['$data'];
} else {
self['$rawData'] = dataItemOrObservable;
self['$data'] = dataItem;
}

if (dataItemAlias)
self[dataItemAlias] = dataItem;

Expand All @@ -71,7 +79,9 @@
}

var self = this,
isFunc = typeof(dataItemOrAccessor) == "function" && !ko.isObservable(dataItemOrAccessor),
shouldInheritData = dataItemOrAccessor === inheritParentVm,
realDataItemOrAccessor = shouldInheritData ? undefined : dataItemOrAccessor,
isFunc = typeof(realDataItemOrAccessor) == "function" && !ko.isObservable(realDataItemOrAccessor),
nodes,
subscribable;

Expand Down Expand Up @@ -112,6 +122,7 @@
}
}
}
ko.bindingContext.inheritParentVm = inheritParentVm;

// Extend the binding context hierarchy with a new view model object. If the parent context is watching
// any observables, the new child context will automatically get a dependency on the parent context.
Expand All @@ -136,10 +147,7 @@
ko.bindingContext.prototype['extend'] = function(properties) {
// If the parent context references an observable view model, "_subscribable" will always be the
// latest view model object. If not, "_subscribable" isn't set, and we can use the static "$data" value.
return new ko.bindingContext(this._subscribable || this['$data'], this, null, function(self, parentContext) {
// This "child" context doesn't directly track a parent observable view model,
// so we need to manually set the $rawData value to match the parent.
self['$rawData'] = parentContext['$rawData'];
return new ko.bindingContext(inheritParentVm, this, null, function(self, parentContext) {
ko.utils.extend(self, typeof(properties) == "function" ? properties() : properties);
});
};
Expand Down
53 changes: 26 additions & 27 deletions src/binding/defaultBindings/ifIfnotWith.js
Original file line number Diff line number Diff line change
@@ -1,34 +1,37 @@
// Makes a binding like with or if
function makeWithIfBinding(bindingKey, isWith, isNot, makeContextCallback) {
function makeWithIfBinding(bindingKey, isWith, isNot) {
ko.bindingHandlers[bindingKey] = {
'init': function(element, valueAccessor, allBindings, viewModel, bindingContext) {
var didDisplayOnLastUpdate,
savedNodes;
var savedNodes;
var ifCondition = !isWith && ko.computed(function() {
return !isNot !== !ko.utils.unwrapObservable(valueAccessor());
}, null, { disposeWhenNodeIsRemoved: element });

ko.computed(function() {
var rawValue = valueAccessor(),
dataValue = ko.utils.unwrapObservable(rawValue),
shouldDisplay = !isNot !== !dataValue, // equivalent to isNot ? !dataValue : !!dataValue
isFirstRender = !savedNodes,
needsRefresh = isFirstRender || isWith || (shouldDisplay !== didDisplayOnLastUpdate);
var shouldDisplay = isWith ? !!ko.utils.unwrapObservable(valueAccessor()) : ifCondition(),
isFirstRender = !savedNodes;

if (needsRefresh) {
// Save a copy of the inner nodes on the initial update, but only if we have dependencies.
if (isFirstRender && ko.computedContext.getDependenciesCount()) {
savedNodes = ko.utils.cloneNodes(ko.virtualElements.childNodes(element), true /* shouldCleanNodes */);
}
// Save a copy of the inner nodes on the initial update, but only if we have dependencies.
if (isFirstRender && ko.computedContext.getDependenciesCount()) {
savedNodes = ko.utils.cloneNodes(ko.virtualElements.childNodes(element), true /* shouldCleanNodes */);
}

if (shouldDisplay) {
if (!isFirstRender) {
ko.virtualElements.setDomNodeChildren(element, ko.utils.cloneNodes(savedNodes));
}
ko.applyBindingsToDescendants(makeContextCallback ? makeContextCallback(bindingContext, rawValue) : bindingContext, element);

This comment has been minimized.

Copy link
@mblarsen

mblarsen Oct 18, 2017

Contributor

I'm rebasing this PR b7e391e#diff-1d8c93790f19c00e4fd280d3a5d13241L24 to this code and trying to get back into it.

Anyway, the PR adds an optional as: binding to with:. What would be the way to add such binding now that makeContextCallback is gone?

This comment has been minimized.

Copy link
@mblarsen

mblarsen Oct 18, 2017

Contributor

.. as easy as adding allBindings().as as the second argument of bindingContext['createChildContext'] I wonder.

(was static child context before)

This comment has been minimized.

Copy link
@mblarsen

mblarsen Oct 19, 2017

Contributor

Seems to do it yes. Tests passes, but my old test cases have not taken the deferred updates into account. I'm not sure if it matters for this binding.

} else {
ko.virtualElements.emptyNode(element);
if (shouldDisplay) {
if (!isFirstRender) {
ko.virtualElements.setDomNodeChildren(element, ko.utils.cloneNodes(savedNodes));
}

didDisplayOnLastUpdate = shouldDisplay;
ko.applyBindingsToDescendants(
isWith ?
bindingContext['createChildContext'](valueAccessor) :
ifCondition.isActive() ?
new ko.bindingContext(ko.bindingContext.inheritParentVm, bindingContext, null, function() { ifCondition(); }) :
bindingContext,
element);
} else {
ko.virtualElements.emptyNode(element);
}
}, null, { disposeWhenNodeIsRemoved: element });

return { 'controlsDescendantBindings': true };
}
};
Expand All @@ -39,8 +42,4 @@ function makeWithIfBinding(bindingKey, isWith, isNot, makeContextCallback) {
// Construct the actual binding handlers
makeWithIfBinding('if');
makeWithIfBinding('ifnot', false /* isWith */, true /* isNot */);
makeWithIfBinding('with', true /* isWith */, false /* isNot */,
function(bindingContext, dataValue) {
return bindingContext.createStaticChildContext(dataValue);
}
);
makeWithIfBinding('with', true /* isWith */);

0 comments on commit 554022b

Please sign in to comment.