From 3074aa979ad71139aa3045655ba27af1257828c5 Mon Sep 17 00:00:00 2001 From: Brian M Hunt Date: Wed, 7 Jun 2017 17:13:57 -0400 Subject: [PATCH] tko.bind) Fix issue with the dependency chain and huge performance problem This addresses the on-again/off-again performance problem experienced and noted here: https://stackoverflow.com/questions/44414554 The synchronous `applyBindings` that was being called in various bindings (e.g. `template`, `if`, `with`) was being repeated for all child bindings, causing large refreshes. The problem was caused when those bindings (`template`, `if`, etc) were changed to be descendants of `BindingHandler`, and were no longer converted via the `LegacyBindingHandler`. What was the `init` function of those bindings became the `constructor` method of the new handler, but with the important difference that the `constructor` was not being called inside an `dependencyDetection.ignore`. --- dist/ko.js | 4 +-- dist/ko.js.map | 4 +-- dist/ko.min.js | 4 +-- dist/ko.min.js.map | 4 +-- karma.conf.js | 2 +- packages/tko.bind/src/LegacyBindingHandler.js | 8 ++--- packages/tko.bind/src/applyBindings.js | 16 +++++----- packages/tko.binding.foreach/src/foreach.js | 6 ++-- packages/tko.binding.if/src/ifIfnotWith.js | 13 ++++---- .../spec/nativeTemplateEngineBehaviors.js | 30 ++++++++++++++++++- .../tko.binding.template/src/templating.js | 27 ++++++++--------- 11 files changed, 72 insertions(+), 46 deletions(-) diff --git a/dist/ko.js b/dist/ko.js index db6b3591..c75aada6 100644 --- a/dist/ko.js +++ b/dist/ko.js @@ -1,3 +1,3 @@ version https://git-lfs.github.com/spec/v1 -oid sha256:d6afaed79a518cb4d1c61bb760c6803b85650bcd5c0f2c7a93eae2e130ae71b6 -size 324794 +oid sha256:0d1f6554b7dc1800374e23b0aca4bb80724886ff8ce21b685085d2d5c25b034d +size 324788 diff --git a/dist/ko.js.map b/dist/ko.js.map index 33e5022a..987d1675 100644 --- a/dist/ko.js.map +++ b/dist/ko.js.map @@ -1,3 +1,3 @@ version https://git-lfs.github.com/spec/v1 -oid sha256:9ce7ddaa01e1c7ea8904008daa3ad3b5d2119739fe71ae64bfdff80d542cd455 -size 576051 +oid sha256:4d1d441b91af7579b6bdd955efa428ac2b9db43618c15fb3c8fd4455866ac891 +size 576059 diff --git a/dist/ko.min.js b/dist/ko.min.js index 03b65f21..5258ec81 100644 --- a/dist/ko.min.js +++ b/dist/ko.min.js @@ -1,3 +1,3 @@ version https://git-lfs.github.com/spec/v1 -oid sha256:3cf0e8f30c5ba3cacae450b90ddd291b5ae208d8f444b1413cc5f6b41c73aaf7 -size 166970 +oid sha256:4c61cdec8b30374f88aa07943b2970cb38455b74dfa5fcb17660174e5e313ef9 +size 167128 diff --git a/dist/ko.min.js.map b/dist/ko.min.js.map index 7472a637..e6a02799 100644 --- a/dist/ko.min.js.map +++ b/dist/ko.min.js.map @@ -1,3 +1,3 @@ version https://git-lfs.github.com/spec/v1 -oid sha256:ad52c825932fcdd37a9cedfd027e3adee837c009ed163938101f1e695692bc28 -size 536653 +oid sha256:850b4c2007f4c9e34ee37370512e14811299a05b05e81ed5a9626b7f2790eb86 +size 536645 diff --git a/karma.conf.js b/karma.conf.js index 452f20ae..08ae9032 100644 --- a/karma.conf.js +++ b/karma.conf.js @@ -12,7 +12,7 @@ const pkg = JSON.parse(fs.readFileSync('package.json')) const root = path.join(process.cwd(), 'spec') -console.log(` 🏕 Karma being loaded at ${process.cwd()}.`) +console.log(` 🏕 Karma being loaded at ${process.cwd()}`) if (!pkg.karma || !pkg.karma.frameworks) { console.warn(` diff --git a/packages/tko.bind/src/LegacyBindingHandler.js b/packages/tko.bind/src/LegacyBindingHandler.js index 99bc3d23..1c91d11f 100644 --- a/packages/tko.bind/src/LegacyBindingHandler.js +++ b/packages/tko.bind/src/LegacyBindingHandler.js @@ -32,12 +32,8 @@ export class LegacyBindingHandler extends BindingHandler { } try { - dependencyDetection.ignore(() => { - this.initReturn = handler.init && handler.init(...this.legacyArgs) - }) - } catch (e) { - params.onError('init', e) - } + this.initReturn = handler.init && handler.init(...this.legacyArgs) + } catch (e) { params.onError('init', e) } if (typeof handler.update === 'function') { this.computed(() => { diff --git a/packages/tko.bind/src/applyBindings.js b/packages/tko.bind/src/applyBindings.js index 88ea80b4..bd6409bf 100644 --- a/packages/tko.bind/src/applyBindings.js +++ b/packages/tko.bind/src/applyBindings.js @@ -267,13 +267,15 @@ function applyBindingsToNodeInternal (node, sourceBindings, bindingContext, bind } try { - const bindingHandler = new BindingHandlerClass({ - allBindings, - $element: node, - $context: bindingContext, - onError: reportBindingError, - valueAccessor (...v) { return getValueAccessor(key)(...v) } - }) + const bindingHandler = dependencyDetection.ignore(() => + new BindingHandlerClass({ + allBindings, + $element: node, + $context: bindingContext, + onError: reportBindingError, + valueAccessor (...v) { return getValueAccessor(key)(...v) } + }) + ) // Expose the bindings via domData. allBindingHandlers[key] = bindingHandler diff --git a/packages/tko.binding.foreach/src/foreach.js b/packages/tko.binding.foreach/src/foreach.js index 273da4c9..7ffcf6fb 100644 --- a/packages/tko.binding.foreach/src/foreach.js +++ b/packages/tko.binding.foreach/src/foreach.js @@ -73,15 +73,15 @@ function extend$context (include$index, $context) { } export class ForEachBinding extends AsyncBindingHandler { - // Valid valueAccessors: + // NOTE: valid valueAccessors include: // [] // observable([]) // observableArray([]) // computed // {data: array, name: string, as: string} + constructor (params) { super(params) - const settings = {} if (isPlainObject(this.value)) { Object.assign(settings, this.value) @@ -123,7 +123,7 @@ export class ForEachBinding extends AsyncBindingHandler { virtualElements.emptyNode(this.$element) // Prime content - var primeData = unwrap(this.data) + const primeData = unwrap(this.data) if (primeData.map) { this.onArrayChange(primeData.map(valueToChangeAddItem), true) } else { diff --git a/packages/tko.binding.if/src/ifIfnotWith.js b/packages/tko.binding.if/src/ifIfnotWith.js index 60c80c0e..37f9afcf 100644 --- a/packages/tko.binding.if/src/ifIfnotWith.js +++ b/packages/tko.binding.if/src/ifIfnotWith.js @@ -52,10 +52,7 @@ function cloneIfElseNodes (element, hasElse) { } } - return { - ifNodes: ifNodes, - elseNodes: elseNodes - } + return { ifNodes, elseNodes } } /** @@ -150,6 +147,11 @@ export class ElseBindingHandler extends ConditionalBindingHandler { * @return {object} { elseChainSatisfied: observable } */ get elseChainIsAlreadySatisfied () { + if (!this._elseChain) { this._elseChain = this.readElseChain() } + return unwrap(this._elseChain.elseChainSatisfied) + } + + readElseChain () { let node = this.$element do { node = node.previousSibling @@ -161,8 +163,7 @@ export class ElseBindingHandler extends ConditionalBindingHandler { node = virtualElements.previousSibling(node) } - const conditionObject = domData.get(node, 'conditional') || {} - return unwrap(conditionObject.elseChainSatisfied) + return domData.get(node, 'conditional') || {} } } diff --git a/packages/tko.binding.template/spec/nativeTemplateEngineBehaviors.js b/packages/tko.binding.template/spec/nativeTemplateEngineBehaviors.js index 89c64e0c..241858f3 100644 --- a/packages/tko.binding.template/spec/nativeTemplateEngineBehaviors.js +++ b/packages/tko.binding.template/spec/nativeTemplateEngineBehaviors.js @@ -3,7 +3,7 @@ import { } from 'tko.bind'; import { - observableArray + observable, observableArray } from 'tko.observable'; import { @@ -189,6 +189,34 @@ describe('Native template engine', function() { expect(testNode.childNodes[0].childNodes[0]).toContainText("(Val: A1, Invocations: 1, Parents: 2)(Val: A2, Invocations: 2, Parents: 2)(Val: A3, Invocations: 3, Parents: 2)"); expect(testNode.childNodes[0].childNodes[1]).toContainText("(Val: ANew, Invocations: 6, Parents: 2)(Val: B1, Invocations: 4, Parents: 2)(Val: B2, Invocations: 5, Parents: 2)"); }); + + it('re-renders nested templates', function () { + var node = document.createElement('div') + let inner = 0 + let outer = 0 + node.innerHTML = ` +
+ +
+ +
+
+ ` + options.bindingProviderInstance.bindingHandlers.set('incrementOuter', () => outer++) + options.bindingProviderInstance.bindingHandlers.set('incrementInner', () => inner++) + const x = observable() + const y = observable() + applyBindings({ x, y }, node) + expect(inner).toEqual(1) + expect(outer).toEqual(1) + x('a') + expect(inner).toEqual(2) + expect(outer).toEqual(2) + y('a') + expect(inner).toEqual(2) + expect(outer).toEqual(3) + }) + }); describe('Data-bind syntax', function() { diff --git a/packages/tko.binding.template/src/templating.js b/packages/tko.binding.template/src/templating.js index a93474b2..226ba253 100644 --- a/packages/tko.binding.template/src/templating.js +++ b/packages/tko.binding.template/src/templating.js @@ -131,7 +131,7 @@ function executeTemplate (targetNodeOrNodeArray, renderMode, template, bindingCo if (haveAddedNodesToParent) { activateBindingsOnContinuousNodeArray(renderedNodesArray, bindingContext, afterBindingCallback) - if (options['afterRender']) { dependencyDetection.ignore(options['afterRender'], null, [renderedNodesArray, bindingContext['$data']]) } + if (options.afterRender) { dependencyDetection.ignore(options.afterRender, null, [renderedNodesArray, bindingContext['$data']]) } } return renderedNodesArray @@ -140,13 +140,13 @@ function executeTemplate (targetNodeOrNodeArray, renderMode, template, bindingCo function resolveTemplateName (template, data, context) { // The template can be specified as: if (isObservable(template)) { - // 1. An observable, with string value + // 1. An observable, with string value return template() } else if (typeof template === 'function') { - // 2. A function of (data, context) returning a string + // 2. A function of (data, context) returning a string return template(data, context) } else { - // 3. A string + // 3. A string return template } } @@ -164,7 +164,7 @@ export function renderTemplate (template, dataOrBindingContext, options, targetN return computed( // So the DOM is automatically updated when any dependency changes function () { - // Ensure we've got a proper binding context to work with + // Ensure we've got a proper binding context to work with var bindingContext = (dataOrBindingContext && (dataOrBindingContext instanceof BindingContextConstructor)) ? dataOrBindingContext : new BindingContextConstructor(dataOrBindingContext, null, null, null, { 'exportDependencies': true }) @@ -229,17 +229,11 @@ export default function renderTemplateForEach (template, arrayOrObservableArray, }, null, { disposeWhenNodeIsRemoved: targetNode }) } -var templateComputedDomDataKey = domData.nextKey() -function disposeOldComputedAndStoreNewOne (element, newComputed) { - var oldComputed = domData.get(element, templateComputedDomDataKey) - if (oldComputed && (typeof (oldComputed.dispose) === 'function')) { oldComputed.dispose() } - domData.set(element, templateComputedDomDataKey, (newComputed && newComputed.isActive()) ? newComputed : undefined) -} +let templateComputedDomDataKey = domData.nextKey() export class TemplateBindingHandler extends AsyncBindingHandler { constructor (params) { super(params) - const element = this.$element const bindingValue = unwrap(this.value) let container @@ -270,7 +264,6 @@ export class TemplateBindingHandler extends AsyncBindingHandler { container = moveCleanedNodesToContainerElement(templateNodes) // This also removes the nodes from their current parent new AnonymousTemplate(element).nodes(container) } - this.computed(this.onValueChange.bind(this)) } @@ -319,7 +312,13 @@ export class TemplateBindingHandler extends AsyncBindingHandler { } // It only makes sense to have a single template computed per element (otherwise which one should have its output displayed?) - disposeOldComputedAndStoreNewOne(element, templateComputed) + this.disposeOldComputedAndStoreNewOne(element, templateComputed) + } + + disposeOldComputedAndStoreNewOne (element, newComputed) { + let oldComputed = domData.get(element, templateComputedDomDataKey) + if (oldComputed && (typeof oldComputed.dispose === 'function')) { oldComputed.dispose() } + domData.set(element, templateComputedDomDataKey, (newComputed && newComputed.isActive()) ? newComputed : undefined) } get controlsDescendants () { return true }