From 88d13f1862da779bcb08e3fbf9e265ae9d29f8cb Mon Sep 17 00:00:00 2001 From: David Greenspan Date: Wed, 8 Aug 2012 14:39:14 -0700 Subject: [PATCH] constant landmark only gets render() if not preserved --- packages/spark/patch.js | 12 ++++++- packages/spark/spark.js | 22 +++++++++--- packages/spark/spark_tests.js | 63 ++++++++++++++++++++++++++++++++--- 3 files changed, 88 insertions(+), 9 deletions(-) diff --git a/packages/spark/patch.js b/packages/spark/patch.js index e263cc1dc4..a4343bb395 100644 --- a/packages/spark/patch.js +++ b/packages/spark/patch.js @@ -1,5 +1,6 @@ -Spark._patch = function(tgtParent, srcParent, tgtBefore, tgtAfter, preservations) { +Spark._patch = function(tgtParent, srcParent, tgtBefore, tgtAfter, preservations, + results) { var copyFunc = function(t, s) { LiveRange.transplantTag(Spark._TAG, t, s); @@ -18,6 +19,13 @@ Spark._patch = function(tgtParent, srcParent, tgtBefore, tgtAfter, preservations } }; + // results arg is optional; it is mutated if provided; returned either way + results = (results || {}); + // array of LiveRanges that were successfully preserved from + // the region preservations + var regionPreservations = (results.regionPreservations = + results.regionPreservations || []); + var lastTgtMatch = null; visitNodes(srcParent, null, null, function(src) { @@ -47,6 +55,7 @@ Spark._patch = function(tgtParent, srcParent, tgtBefore, tgtAfter, preservations // (including references to enclosing ranges). LiveRange.transplantRange( pres.fromStart, pres.fromEnd, pres.newRange); + regionPreservations.push(pres.newRange); } } else if (pres.type === 'node') { if (patcher.match(tgt, src, copyFunc)) { @@ -71,6 +80,7 @@ Spark._patch = function(tgtParent, srcParent, tgtBefore, tgtAfter, preservations patcher.finish(); + return results; }; diff --git a/packages/spark/spark.js b/packages/spark/spark.js index 72b186f114..e38b608f73 100644 --- a/packages/spark/spark.js +++ b/packages/spark/spark.js @@ -20,6 +20,8 @@ // XXX should functions with an htmlFunc use try/finally inside? +// XXX do render callbacks "bubble up" to enclosing landmarks? + (function() { Spark = {}; @@ -298,7 +300,8 @@ var scheduleOnscreenSetup = function (frag, landmarkRanges) { // XXX should bubble up and notify parent landmarks too? for all // the same reasons we need to do it for node preservation? _.each(landmarkRanges, function (landmarkRange) { - landmarkRange.renderCallback.call(landmarkRange.landmark); + if (! landmarkRange.isPreservedConstant) + landmarkRange.renderCallback.call(landmarkRange.landmark); }); // This code can run several times on the same nodes (if the @@ -488,6 +491,8 @@ Spark.renderToRange = function (range, htmlFunc) { tempRange.destroy(); + var results = {}; + // patch (using preservations) range.operate(function (start, end) { // XXX this will destroy all liveranges, including ones @@ -495,7 +500,16 @@ Spark.renderToRange = function (range, htmlFunc) { // to preserve untouched Spark.finalize(start, end); Spark._patch(start.parentNode, frag, start.previousSibling, - end.nextSibling, preservations); + end.nextSibling, preservations, results); + }); + + _.each(results.regionPreservations, function (landmarkRange) { + // Rely on the fact that computePreservations only emits + // region preservations whose ranges are landmarks. + // This flag means that landmarkRange is a new constant landmark + // range that matched an old one *and* was DOM-preservable by + // the patcher. + landmarkRange.isPreservedConstant = true; }); }; @@ -697,7 +711,7 @@ Spark.isolate = function (htmlFunc) { return; // killed by finalize. range has already been destroyed. ctx = new Meteor.deps.Context; - var frag = Spark.renderToRange(range, function () { + Spark.renderToRange(range, function () { return ctx.run(htmlFunc); }); ctx.on_invalidate(refresh); @@ -726,7 +740,7 @@ var atFlushTime = function (f) { atFlushContext = new Meteor.deps.Context; atFlushContext.on_invalidate(function () { var f; - while (f = atFlushQueue.shift()) { + while ((f = atFlushQueue.shift())) { // Since atFlushContext is truthy, if f() calls atFlushTime // reentrantly, it's guaranteed to append to atFlushQueue and // not contruct a new atFlushContext. diff --git a/packages/spark/spark_tests.js b/packages/spark/spark_tests.js index 2cf5af7079..6491bb436b 100644 --- a/packages/spark/spark_tests.js +++ b/packages/spark/spark_tests.js @@ -1620,8 +1620,7 @@ Tinytest.add("spark - landmark constant", function(test) { test.equal(states.length, 1); R.set(1); Meteor.flush(); - test.equal(states.length, 2); - test.isTrue(states[0] === states[1]); + test.equal(states.length, 1); // no render callback on constant var nodes2 = _.toArray(div.node().childNodes); test.equal(nodes2.length, 3); test.isTrue(nodes[0] === nodes2[0]); @@ -1642,6 +1641,8 @@ Tinytest.add("spark - landmark constant", function(test) { var hasSpan = true; var isConstant = true; + var crd = null; // [createCount, renderCount, destroyCount] + R = ReactiveVar('foo'); div = OnscreenDiv(Meteor.render(function() { R.get(); // create unconditional dependency @@ -1650,7 +1651,17 @@ Tinytest.add("spark - landmark constant", function(test) { Spark.labelBranch( brnch, function () { return Spark.createLandmark( - { constant: isConstant }, + { + constant: isConstant, + create: function () { + this.crd = [0,0,0]; + if (! crd) + crd = this.crd; // capture first landmark's crd + this.crd[0]++; + }, + render: function () { this.crd[1]++; }, + destroy: function () { this.crd[2]++; } + }, function() { return hasSpan ? 'stuff' : 'blah'; });}) + (nodeAfter ? R.get() : ''); @@ -1667,12 +1678,16 @@ Tinytest.add("spark - landmark constant", function(test) { R.set('bar'); Meteor.flush(); - // only absence of branch should cause the constant + // only non-matching landmark should cause the constant // chunk to be re-rendered test.equal(div.text(), (nodeBefore ? 'bar' : '')+ (matchLandmark ? 'stuff' : 'blah')+ (nodeAfter ? 'bar' : '')); + // in non-matching case, first landmark is destroyed. + // otherwise, it is kept (and not re-rendered because + // it is constant) + test.equal(crd, matchLandmark ? [1,1,0] : [1,1,1]); R.set('baz'); Meteor.flush(); @@ -1717,6 +1732,46 @@ Tinytest.add("spark - landmark constant", function(test) { }); }); + // test that constant landmark gets render callback if it + // wasn't preserved. + + var renderCount; + + renderCount = 0; + R = ReactiveVar('div'); + div = OnscreenDiv(Meteor.render(function () { + return '<' + R.get() + '>' + Spark.createLandmark( + {constant: true, render: function () { renderCount++; }, + destroy: function () { + console.log(3, rangeToHtml(this._range.findParent())); + }}, + function () { + if (R.get() === 'div class="hamburger"') + console.log(2); + return "hi"; + }) + + ''; + })); + Meteor.flush(); + test.equal(renderCount, 1); + + R.set('div class="hamburger"'); + console.log(1); + Meteor.flush(); + console.log(4); + // constant patched around, not re-rendered! + test.equal(renderCount, 1); + + R.set('span class="hamburger"'); + Meteor.flush(); + // can't patch parent to a different tag + test.equal(renderCount, 2); + + R.set('span'); + Meteor.flush(); + // can patch here, renderCount stays the same + test.equal(renderCount, 2); + });