Bug Tracker

Opened 5 years ago

Closed 4 years ago

Last modified 4 years ago

#11230 closed bug (fixed)

.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 http://bugs.jquery.com/ticket/11226

Change History (9)

comment:1 Changed 5 years ago by sindresorhus

Owner: set to 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: http://jsfiddle.net/FrKyN/ Open the link and click to "Fork" (in the top menu) to get started.

comment:3 Changed 5 years ago by dmethvin

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. :)

Last edited 5 years ago by dmethvin (previous) (diff)

comment:4 Changed 4 years ago by dmethvin

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.

comment:5 Changed 4 years ago by dmethvin

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.

comment:6 Changed 4 years ago by scottgonzalez

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.

comment:7 Changed 4 years ago by dmethvin

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.

comment:8 Changed 4 years ago by Dave Methvin

Resolution: fixed
Status: openclosed

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

Changeset: 0018f7700bf8004d573c752ecbf66f299769c4de

comment:9 Changed 4 years ago by dmethvin

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.

Note: See TracTickets for help on using tickets.