Skip to content

Commit

Permalink
Browse files Browse the repository at this point in the history
Fixes #13779. Remove nodes in document order (uses for loop matching … 
…empty()).

Signed-off-by: Rick Waldron <waldron.rick@gmail.com>
  • Loading branch information
@rwaldron
rwaldron committed Apr 16, 2013
1 parent 562ca75 commit 332ebeb
Showing 1 changed file with 3 additions and 2 deletions.
5 changes: 3 additions & 2 deletions src/manipulation.js
View file
Expand Up @@ -73,9 +73,10 @@ jQuery.fn.extend({
remove: function( selector, keepData ) {
var elem,
elems = selector ? jQuery.filter( selector, this ) : this,
i = elems.length;
i = 0,
l = elems.length;

while ( i-- ) {
for ( ; i < l; i++ ) {
elem = elems[ i ];

if ( !keepData && elem.nodeType === 1 ) {
Expand Down

18 comments on commit 332ebeb

@gibson042
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No unit test? What did the old code break?

@rwaldron
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See follow up commit, I forgot to "tick" the the test file when staging from Tower. Sorry about that

@scottgonzalez
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't explain why removal needs to happen in document order. @BorisMoore said "This can break client code quite easily," but didn't provide any examples. @gibson042 asked for a use case, and then this was changed without any use case being presented. So @gibson042 has asked this question twice now, but still hasn't received an answer: What did the old code break?

@rwaldron
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@scottgonzalez I've sent a direct request to @BorisMoore to share a reduction or example.

@markelog
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, did we not decide to use while( --i ) idiom instead of for loop? Or at least for ( ; (elem = elems[i]) != null; i++ ) {?

See PR and ticket associated with it.

@BorisMoore
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any app that does its own disposal when elements are removed will need to override cleanData, and may need to remove elements in doc order. An example is https://github.com/BorisMoore/jsviews/blob/master/jquery.views.js.

Here is a jsfiddle using 1.9.1: http://jsfiddle.net/BorisMoore/xzGrj/
Click on Meet Jo Black and then on Eyes Wide Shut. The master detail selection works correctly.

Here is the same fiddle but using 2.x (edge) (Same would apply to beta3, which is not correctly provided in jsfiddle): http://jsfiddle.net/BorisMoore/WG2av/
Click on Meet Jo Black - it works.
Click on Eyes Wide Shut. The deleted nodes on the detail are deleted in reverse order, so disposal fails, and there is a script error.

@BorisMoore
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that in current code, .empty() does for ( ; i < l; i++ ) { and .remove does while ( i-- ) {. So (currently) even within a single version of jQuery, code that does disposal needs to deal with both document order and reverse document order semantics.

@markelog
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@BorisMoore

So (currently)

Such behaviour was introduced only couple of weeks ago, of course, either jQuery#empty or jQuery#remove should be changed, i suppose you already notice that there is a PR for it.

Here is the same fiddle

Can you reduce this test-case? It's hard to digest it, without deep understanding of jquery.views.

@BorisMoore
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here is a really simple test case: http://jsfiddle.net/BorisMoore/YFNbC/

In the case of JsViews, data-binding is achieved in some scenarios by insertion of script nodes as marker, (with type !== "javascript" of course), before and after the data-bound HTML content. Some frameworks insert comment nodes, though browsers sometimes move those nodes.

Anyway, the framework may need to handle removal of the preceding marker node differently from the following marker node, as part of the process of disposing the binding, and may need to handle the preceding node before the follow one, know which is which, etc...

Up to now jQuery has always used document order semantics, so code could easily have relied on that. Different users of the framework could be using any number of different version of jQuery. So changing the ordering semantics can create problems...

@markelog
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@BorisMoore thank you! It's explains a lot.

@rwaldron
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, the issue is clearer now, but less sympathetic to your position. Redefining jQuery APIs (especially undocumented methods) voids the warranty. Removal in document order was never guaranteed and is an implementation detail (that's only observable when you rewire the library) which is completely at the source's discretion. If jQuery wanted to "parallelize" the operation by removing the nodes 2 at a time starting at the beginning and middle, then nothing would prevent that, because there is no documented guarantee of execution order.

@BorisMoore
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand that point of view. However, this is a place where jQuery does provide real value. Until we have real usable DOM Mutation events on all browsers :), it is not possible for a platform to manage DOM element disposal effectively other than forcing users to go through special DOM Manip APIs .

Thank goodness at least that it is possible with jQuery, (without re-inventing another parallel set of DOM manip APIs) provided users of the platform use only jQuery APIs for their DOM manipulation...

Of course one can override all the public jQuery DOM manip methods, instead of cleanData, but that would be even more complex and would require the override semantics to track jQuery underlying semantics (as to doc order for example) so it is equally vulnerable...

If you search in code on GitHub for $.cleanData = function(... you get 52210 hits!

@scottgonzalez
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you search in code on GitHub for $.cleanData = function(... you get 52210 hits!

That's because you get a result for every fork and every repo that includes a dependency that does this.

I looked through the first 12 pages and every single result was just triggering an event, e.g., https://github.com/jquery/jquery-ui/blob/71a332e8b83a1657521e04388f5592997e81bbcc/ui/jquery.ui.widget.js#L16-L24

I stopped on page 12 because I saw a result in zepto.js and knew that this wasn't going to lead anywhere good ;-)

@timmywil
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't care that much about the while loop. For loops are fine with me. Since this was caught by someone in the beta, I'm inclined to say let's change it back for the sake of playing nicely with existing code. While I don't think we need to guarantee document-order traversal, it's certainly at a low cost in this case.

@dmethvin
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we are going to change the code back for what we consider to be a regression, we're going to put in a unit test and not be able to change traversal order in the future. It seems as if we would need to guarantee first-to-last order for consistency across the API. We can't just change the code and not have a unit test, or we'll be back here discussing the issue again soon when we break it again.

Although I can live with it either way, it seems like we're overconstraining ourselves by saying we have to use a specific order.

@BorisMoore
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If there is a reason why you need to change ordering semantics in the future, then frameworks like JsViews will just have to add extra code or provide alternative DOM manip APIs to replace those of jQuery. But hey, if they have to they have, since I agree it might be too constraining for you to consider that an absolute contract.

But I still feel strongly that it is better not to switch semantics if there is no major advantage for you to do so in terms of implementation, perf or such like.

So I hope you will wait until you 'have to' to make a change, and above all, not randomly switch, by considering it an internal 'implementation detail'...

So of course it is a vote from me (fwiw :) ) to stay with the first to last ordering for now, and to include that unit test(s), so it only changes if there is a real 'need' to do so in the future...

@jaubourg
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tend to agree with Timmy, Dave & Boris here. This is non critical, so let's add a unit test and keep it in document order for now. This is a unit test that'll test an implementation detail but that wouldn't be a first and that wouldn't be the first of its kind to be removed if an alternative, faster/better/whateverer rewrite comes in.

Hopefully, by the time we rewrite this part again, the DOM will provide the events that would make Boris's hack unnecessary ;)

@rwaldron
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dmethvin here is the patch from earlier #1243

Please sign in to comment.