constant landmark only gets render() if not preserved

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;
 };
 
 

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.

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 ?
                                '<span>stuff</span>' : '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";
+      }) +
+      '</' + R.get().split(' ')[0] + '>';
+  }));
+  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);
+
 });