Skip to main content

Bug Tracker

Side navigation

#11230 closed bug (fixed)

Opened January 26, 2012 11:21AM UTC

Closed December 13, 2012 02:27PM UTC

Last modified December 13, 2012 02:38PM UTC

.appendTo .prependTo .insertAfter .insertBefore returns incorrect data for $('not_existing_element')

Reported by: yurk Owned by: yurk
Priority: low Milestone: 1.9
Component: manipulation Version: 1.7.1
Keywords: Cc:
Blocked by: Blocking:

expected: return collection with <h3> element

actually: returns an empty collection

Maybe related with

Attachments (0)
Change History (9)

Changed January 26, 2012 11:26AM UTC by sindresorhus comment:1

owner: → yurk
status: newpending

Thanks for taking the time to contribute to the jQuery project! Please provide a complete reduced test case on jsFiddle to help us assess your ticket.

Additionally, be sure to test against the jQuery Edge version to ensure the issue still exists. To get you started, use this boilerplate: Open the link and click to "Fork" (in the top menu) to get started.

Changed January 26, 2012 11:55AM UTC by anonymous comment:2

Changed January 26, 2012 03:11PM UTC by dmethvin comment:3

_comment0: This one seems to have been around since the dawn of jQuery; I agree that it seems wrong for the set to disappear. Most likely it's *very* rare for anyone to want to continue a chain in that situation. Often I do `var x = $("html").appendTo("body")` but of course the body is always there. :)1327590723034546
component: unfiledmanipulation
priority: undecidedlow
status: pendingopen

This one seems to have been around since the dawn of jQuery; I agree that it seems wrong for the set to disappear. Most likely it's *very* rare for anyone to want to continue a chain in that situation. Often I do var x = $("<html stuff>").appendTo("body") but of course the body is always there. :)

Changed November 02, 2012 02:56AM UTC by dmethvin comment:4

milestone: None1.9

Okay, studying the docs for appendTo I am starting to think this is working as documented, even if it seems stupid:

If an element selected this way is inserted elsewhere, it will be moved into the target (not cloned) ... If there is more than one target element, however, cloned copies of the inserted element will be created for each target after the first, and that new set (the original element plus clones) is returned.

Reading between the lines on that, when there are *no* elements, no elements are returned. I would expect the original set to always be returned, but that's not what it's documented to do unless there is only 1 element.

Looking at the current code, it's more distressing that it does a pushStack unless there is just 1 element. That means it's impossible to tell whether to use .end() on an arbitrary set.

Because of the latter issue I could be convinced to change the documented behavior here so that it always returns the original set.

Changed December 12, 2012 08:54PM UTC by dmethvin comment:5

I spent way too much time looking at this, and am convincing myself we need a breaking change. Consider this case, what does it return?


It turns out the answer depends on whether .status selects 0, 1, or N elements, whether the incoming set has 0 or 1 elements, and whether the first element in the incoming set is connected to the document or not.

If .status selects nothing, we get a new set that is empty as this ticket shows. If .status selects one element, and there's only one element in the set, we return the original set and don't .pushStack(). Otherwise we return a new set consisting of the original plus all the clones.

And that's the simplified description ... I think I got that right.

I'm proposing that we always return the original set and document this as a breaking change. At least in my use, I am generally using .appendTo() for appending to one element which will have the same behavior in the new world order.

Changed December 12, 2012 09:00PM UTC by scottgonzalez comment:6

Should we always return the new set? Otherwise, you can't sanely chain methods. I think the only confusing thing would be that single element .appendTo() would need to .pushStack(), but at least it would be consistent and useful.

Changed December 12, 2012 09:18PM UTC by dmethvin comment:7

Scott and I talked in IRC ... I can see the case for stacking. My main concern is that we are consistent here, since we already got rid of the maybe-stack-who-knows behavior of .before(), .after() and .replaceWith().

The benefit of consistently stacking the appended elements is that it's somewhat like .clone() in that it selects the clones, although a case like the one here will get you NOTHING because nothing was appended to anything. It's is probably the least likely to break things because it will return the same elements as before, just stacking the one case it didn't used to stack.

The benefit of returning the current set is that it's smaller and faster, plus it won't create a useless stacked set for a case like var markup = $("<div>").appendTo("body") which we certainly use all over the place in unit tests at least.

I sure wish we knew what people were doing out there. It could turn out that these cases are pretty rare, given how inconsistently they currently behave.

Changed December 13, 2012 02:27PM UTC by Dave Methvin comment:8

resolution: → fixed
status: openclosed

Fix #11230. .appendTo and pals should always stack.

Changeset: 0018f7700bf8004d573c752ecbf66f299769c4de

Changed December 13, 2012 02:38PM UTC by dmethvin comment:9

And just to be clear here, the behavior of returning a new empty set is correct, we need to update the docs to clarify this change which was to return a new set when there is only one element and several other special cases.