Merge pull request #350 from jugglinmike/cleanup-manip

Remove nodes from their previous structures
cheeriojs · Feb 28, 2014 · 18bc828 · 18bc828
2 parents f11de45 + 66c4a47
commit 18bc828
Show file tree

Hide file tree

Showing 2 changed files with 87 additions and 6 deletions.
diff --git a/lib/api/manipulation.js b/lib/api/manipulation.js
@@ -31,22 +31,51 @@ var _insert = function(concatenator) {
     if (_.isFunction(elems[0])) {
       return this.each(function(i, el) {
         dom = makeDomArray(elems[0].call(el, i, this.html()));
-        updateDOM(concatenator(dom, el.children || (el.children = [])), el);
+        concatenator(dom, el.children);
+        updateDOM(el.children, el);
       });
     } else {
       return domEach(this, function(i, el) {
-        updateDOM(concatenator(dom, el.children || (el.children = [])), el);
+        concatenator(dom, el.children);
+        updateDOM(el.children, el);
       });
     }
   };
 };
 
+/*
+ * Modify an array in-place, removing some number of elements and adding new
+ * elements directly following them.
+ *
+ * @param {Array} array Target array to splice.
+ * @param {Number} spliceIdx Index at which to begin changing the array.
+ * @param {Number} spliceCount Number of elements to remove from the array.
+ * @param {Array} newElems Elements to insert into the array.
+ *
+ * @api private
+ */
+var uniqueSplice = function(array, spliceIdx, spliceCount, newElems) {
+  var spliceArgs = [spliceIdx, spliceCount].concat(newElems);
+  var idx, len, prevIdx;
+
+  // Before splicing in new elements, ensure they do not already appear in the
+  // current array.
+  for (idx = 0, len = newElems.length; idx < len; ++idx) {
+    prevIdx = array.indexOf(newElems[idx]);
+    if (prevIdx > -1) {
+      array.splice(prevIdx, 1);
+    }
+  }
+
+  return array.splice.apply(array, spliceArgs);
+};
+
 var append = exports.append = _insert(function(dom, children) {
-  return children.concat(dom);
+  uniqueSplice(children, children.length, 0, dom);
 });
 
 var prepend = exports.prepend = _insert(function(dom, children) {
-  return dom.concat(children);
+  uniqueSplice(children, 0, 0, dom);
 });
 
 var after = exports.after = function() {
@@ -66,7 +95,7 @@ var after = exports.after = function() {
     }
 
     // Add element after `this` element
-    siblings.splice.apply(siblings, [++index, 0].concat(dom));
+    uniqueSplice(siblings, ++index, 0, dom);
 
     // Update next, prev, and parent pointers
     updateDOM(siblings, parent);
@@ -92,7 +121,7 @@ var before = exports.before = function() {
     }
 
     // Add element before `el` element
-    siblings.splice.apply(siblings, [index, 0].concat(dom));
+    uniqueSplice(siblings, index, 0, dom);
 
     // Update next, prev, and parent pointers
     updateDOM(siblings, parent);

diff --git a/test/api.manipulation.js b/test/api.manipulation.js
@@ -30,6 +30,20 @@ describe('$(...)', function() {
       expect($fruits.children(3).hasClass('plum')).to.be.ok();
     });
 
+    it('(existing Node) : should remove node from previous location', function() {
+      var $fruits = $(fruits);
+      var apple = $fruits.children()[0];
+      var $children;
+
+      expect($fruits.children()).to.have.length(3);
+      $fruits.append(apple);
+      $children = $fruits.children();
+
+      expect($children).to.have.length(3);
+      expect($children[0]).to.not.equal(apple);
+      expect($children[2]).to.equal(apple);
+    });
+
     it('($(...), html) : should add multiple elements as last children', function() {
       var $fruits = $(fruits);
       var $plum = $('<li class="plum">Plum</li>');
@@ -158,6 +172,20 @@ describe('$(...)', function() {
       expect($fruits.children(0).hasClass('plum')).to.be.ok();
     });
 
+    it('(existing Node) : should remove existing nodes from previous locations', function() {
+      var $fruits = $(fruits);
+      var pear = $fruits.children()[2];
+      var $children;
+
+      expect($fruits.children()).to.have.length(3);
+      $fruits.prepend(pear);
+      $children = $fruits.children();
+
+      expect($children).to.have.length(3);
+      expect($children[2]).to.not.equal(pear);
+      expect($children[0]).to.equal(pear);
+    });
+
     it('(Array) : should add all elements in the array as inital children', function() {
       var $fruits = $(fruits);
       var more = $('<li class="plum">Plum</li><li class="grape">Grape</li>')
@@ -287,6 +315,18 @@ describe('$(...)', function() {
       expect($('.apple', $fruits).next().hasClass('plum')).to.be.ok();
     });
 
+    it('(existing Node) : should remove existing nodes from previous locations', function() {
+      var $fruits = $(fruits);
+      var pear = $fruits.children()[2];
+      var $children;
+
+      $('.apple', $fruits).after(pear);
+
+      $children = $fruits.children();
+      expect($children).to.have.length(3);
+      expect($children[1]).to.be(pear);
+    });
+
     it('($(...), html) : should add multiple elements as next siblings', function() {
       var $fruits = $(fruits);
       var $plum = $('<li class="plum">Plum</li>');
@@ -382,6 +422,18 @@ describe('$(...)', function() {
       expect($('.apple', $fruits).prev().hasClass('plum')).to.be.ok();
     });
 
+    it('(existing Node) : should remove existing nodes from previous locations', function() {
+      var $fruits = $(fruits);
+      var pear = $fruits.children()[2];
+      var $children;
+
+      $('.apple', $fruits).before(pear);
+
+      $children = $fruits.children();
+      expect($children).to.have.length(3);
+      expect($children[0]).to.be(pear);
+    });
+
     it('(Array) : should add all elements in the array as previous sibling', function() {
       var $fruits = $(fruits);
       var more = $('<li class="plum">Plum</li><li class="grape">Grape</li>')