2.0: Refactor jQuery.clean #1117

Closed
wants to merge 4 commits into
from

2 participants

@markelog
jQuery Foundation member

Now, when manipulation module was reduced, maybe it's time to refactor some stuff?

You removed jQuery.buildFragment, but it seems jQuery.clean is one that should be removed. If this changes is acceptable i could ported it to 1.9 too.

@markelog
jQuery Foundation member

In migrate plugin, jQuery.clean could be implemented like that –

jQuery.clean = function( elems, context, fragment, scripts, selection ) {
    var newFragment = jQuery.buildFragment( elems, context || document, scripts, selection );

    if ( fragment ) {
        fragment.appendChild( newFragment );

    } else {
        fragment = newFragment;
    }

    return jQuery.merge( [], fragment.childNodes );
};

But i did not test this code thoroughly

@gibson042 gibson042 and 1 other commented on an outdated diff Jan 7, 2013
src/manipulation.js
- if ( rscriptType.test( elem.type || "" ) ) {
- scripts.push( elem );
- }
+ // Capture executables
+ if ( scripts ) {
+ for ( j = 0, ll = tmp.length; j < ll; j++ ) {
+ elem = tmp[ j ];
@gibson042
jQuery Foundation member
gibson042 added a line comment Jan 7, 2013

Both this and L471 are prime candidates for our i = 0; while ( (elem = array[i++]) ) { pattern, but failing that I'd still check the compressibility of putting initial assignments before the for.

@markelog
jQuery Foundation member
markelog added a line comment Jan 7, 2013

done, it saves 15 bytes or so and it definitely looks more elegant.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@gibson042 gibson042 and 1 other commented on an outdated diff Jan 7, 2013
test/unit/core.js
@@ -1237,6 +1237,14 @@ test("jQuery.parseHTML", function() {
equal( jQuery.parseHTML( "\t<div></div>" )[0].nodeValue, "\t", "Preserve leading whitespace" );
equal( jQuery.parseHTML(" <div/> ")[0].nodeType, 3, "Leading spaces are treated as text nodes (#11290)" );
+
+ html = jQuery.parseHTML( "<div>test div</div>" );
+
+ equal( html[ 0 ].parentNode.nodeType, 11, "parentNode should be documentFragment" );
+ equal( html[ 0 ].innerHTML, "test div", "Content should be preserved" );
+
+ equal( jQuery.parseHTML("<span><span>").length, 1, "Incorrect html-strings should not break anything" );
+ equal( jQuery.parseHTML("<td><td>")[ 1 ].parentNode.nodeType, 11, "parentNode should be documentFragment" );
@gibson042
jQuery Foundation member
gibson042 added a line comment Jan 7, 2013

What is this assertion supposed to accomplish?

@markelog
jQuery Foundation member
markelog added a line comment Jan 7, 2013

td is a special element that defined in wrapMap, in jQuery.clean/jQuery.buildFragment it should descend to the right depth, parent would not be div like with most nodes, but tr element, so td should be removed though it.

I wanted to test that...

@gibson042
jQuery Foundation member
gibson042 added a line comment Jan 7, 2013

👍
Could you mention that in the assertion text as something like "parentNode should be documentFragment for wrapMap elements"?

@markelog
jQuery Foundation member
markelog added a line comment Jan 7, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@staabm staabm and 1 other commented on an outdated diff Jan 7, 2013
src/manipulation.js
// Convert non-html into a text node
} else if ( !rhtml.test( elem ) ) {
- ret.push( context.createTextNode( elem ) );
+ nodes.push( context.createTextNode( elem ) );
@staabm
staabm added a line comment Jan 7, 2013

Could you use also core_push here? Or maybe push instead of core_push in L446?

@markelog
jQuery Foundation member
markelog added a line comment Jan 7, 2013

good catch!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@staabm staabm commented on the diff Jan 7, 2013
src/manipulation.js
}
- core_push.apply( ret, tmp.childNodes );
+ core_push.apply( nodes, tmp.childNodes );
@staabm
staabm added a line comment Jan 7, 2013

Same here

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@dmethvin
jQuery Foundation member

2.0: Landed in unsquashed commits ending at 57d9dcd.

1.9: Similar edits landed at 0ed497b -- @orkel could you take a look and see if I got that right? It can probably use some cleanup but I wanted to land it asap.

@dmethvin dmethvin closed this Jan 8, 2013
@gibson042 gibson042 added a commit that referenced this pull request Jan 9, 2013
@markelog markelog Ref gh-1117: Don't stop on a falsy value in buildFragment.
(cherry picked from commit 8e6c1ba)
46bbda8
@mescoda mescoda pushed a commit to mescoda/jquery that referenced this pull request Nov 4, 2014
@dmethvin dmethvin Resurrect buildFragment and sacrifice jQuery.clean. See gh-1117. a456b18
@mescoda mescoda pushed a commit to mescoda/jquery that referenced this pull request Nov 4, 2014
@markelog markelog Ref gh-1117: Don't stop on a falsy value in buildFragment.
(cherry picked from commit 8e6c1ba)
ff784cd
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment