Fix order of elements after .add

[fb55]

$('<a>').add('<b>').map((i, e) => console.log(e.tagName))

produces A, then B in jQuery, and b and a in cheerio.

This seems like the kind of thing we can fix in a 1.0

Adapted from #952.

[fb55]

@jugglinmike Would you mind having a look at this, as well as #1159?

[jugglinmike]

Doing so as we speak, actually :)

[jugglinmike]

This implementation contains a regression. Today, jQuery and Cheerio agree:

$('<a>').add('<b>').end().length === 1

But because this patch modifies the current selection (instead of creating new one), it interferes with the expected behavior of the end API (along with addBack and addSelf).

I just spent a bunch of time fixing this, but the correction essentially amounts to reverting the original patch. There are no new tests that assert the desired behavior. This may be surprising given how many changes were made to the tests, but all of those changes are concerned with working around the regression itself.

The correction looks like this:

--- a/lib/api/traversing.js
+++ b/lib/api/traversing.js
@@ -413,10 +413,11 @@ exports.add = function(other, context) {
   var contents = uniqueSort(this.get().concat(selection.get()));
 
   for (var i = 0; i < contents.length; ++i) {
-    this[i] = contents[i];
+    selection[i] = contents[i];
   }
-  this.length = contents.length;
-  return this;
+  selection.length = contents.length;
+
+  return selection;
 };

Which, when applied to the current change set, makes the overall patch from this branch look like this:

--- a/lib/api/traversing.js
+++ b/lib/api/traversing.js
@@ -410,7 +410,7 @@ exports.end = function() {
 
 exports.add = function(other, context) {
   var selection = this._make(other, context);
-  var contents = uniqueSort(selection.get().concat(this.get()));
+  var contents = uniqueSort(this.get().concat(selection.get()));
 
   for (var i = 0; i < contents.length; ++i) {
     selection[i] = contents[i];

We don't have a test for this, and that is actually intentional. From the source of our test suite:

    /**
     * Element order is undefined in this case, so it should not be asserted
     * here.
     *
     * > If the collection consists of elements from different documents or
     * > ones not in any document, the sort order is undefined.
     *
     * http://api.jquery.com/add/
     */

We've received feature requests like this in the past, and I stand by my general response. It might be nice for Cheerio to match the observed behavior of jQuery, but this is explicitly documented as unstable behavior. By trying to replicate it in Cheerio, we are doing a disservice to our users (by encouraging them to write code which depends on unstable behavior) and to ourselves as maintainers (by suggesting that we will monitor any changes that jQuery might make between stable releases--as is its right--and update Cheerio accordingly).

So I'd like to close this issue as "won't fix". Even if it makes tonight's coding session a wash. What do you say, @fb55?

[fb55]

Thanks so much Mike for diving into this! Seems very reasonable, closing this then :)

[jugglinmike]

My pleasure!