Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

Improve replaceWith #1038

Closed
wants to merge 1 commit into from

4 participants

@markelog
Collaborator

It's faster, smaller (-10 bytes) and simpler.

@rwaldron rwaldron commented on the diff
src/manipulation.js
((19 lines not shown))
val = !isFunc ? value : value.call( this, i, this );
if ( isDisconnected( this ) ) {
// for disconnected elements, we replace with the new content in the set. We use
// clone here to ensure that each replaced instance is unique
- self[ i ] = jQuery( val ).clone()[ 0 ];
- return;
+ return self[ i ] = jQuery( val ).clone()[ 0 ];
@rwaldron Collaborator

Why this changed behaviour? There are no tests that support a newly appointed return value...

@markelog Collaborator

This behaviour isn't changed, look closely

@rwaldron Collaborator

Ugh. Github's diff doesn't show enough lines to give context to the change. Sorry for the noise.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@gibson042
Collaborator

I like your implementation of .replaceWith much better than what's in master, however @PaulBRamos is actively working on this code in order to resolve #4087 for 1.9. I recommend either collaborating with him or waiting until a bugfix commit lands before optimizing here.

@dmethvin
Owner

Since @PaulBRamos had a patch in this area, I'm going to close this request. @orkel can you review that code when it arrives?

@dmethvin dmethvin closed this
@markelog
Collaborator

@dmethvin

@PaulBRamos pull, currently, not replacing this pull, but one is affecting the other.

@dmethvin, @PaulBRamos, @gibson042 I suggest to wait untill fix for #4087 will be landed (i hope it will), but remove from 1033 pull fix for replaceWith/replaceAll and deal with #4087 bug for these methods here, because this change will no longer be needed, with this optimization, howerer, in order to fix #4087 for replaceWith/replaceAll methods i would need @PaulBRamos changes in jQuery.buildFragment.

@PaulBRamos who you feel about that?

@dmethvin can i reopen this pull after @PaulBRamos fix will be landed?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Nov 20, 2012
  1. @markelog

    Improve replaceWith

    markelog authored
This page is out of date. Refresh to see the latest.
Showing with 9 additions and 23 deletions.
  1. +9 −23 src/manipulation.js
View
32 src/manipulation.js
@@ -273,38 +273,24 @@ jQuery.fn.extend({
var self = this,
isFunc = jQuery.isFunction( value );
- // Make sure that the elements are removed from the DOM before they are inserted
- // this can help fix replacing a parent with child elements
- if ( !isFunc && typeof value !== "string" ) {
- value = jQuery( value ).detach();
- }
-
- this.each( function( i ) {
- var next = this.nextSibling,
- parent = this.parentNode,
- // HTML argument replaced by "this" element
- // 1. There were no supporting tests
- // 2. There was no internal code relying on this
- // 3. There was no documentation of an html argument
+ return this.each(function( i ) {
+ var parsed,
val = !isFunc ? value : value.call( this, i, this );
if ( isDisconnected( this ) ) {
// for disconnected elements, we replace with the new content in the set. We use
// clone here to ensure that each replaced instance is unique
- self[ i ] = jQuery( val ).clone()[ 0 ];
- return;
+ return self[ i ] = jQuery( val ).clone()[ 0 ];
@rwaldron Collaborator

Why this changed behaviour? There are no tests that support a newly appointed return value...

@markelog Collaborator

This behaviour isn't changed, look closely

@rwaldron Collaborator

Ugh. Github's diff doesn't show enough lines to give context to the change. Sorry for the noise.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
}
- jQuery( this ).remove();
-
- if ( next ) {
- jQuery( next ).before( val );
- } else {
- jQuery( parent ).append( val );
+ if ( !val || !val.nodeType ) {
+ parsed = jQuery.buildFragment( [ val ], this );
+ val = parsed.cacheable ? jQuery.clone( parsed.fragment ) : parsed.fragment;
}
- });
- return this;
+ jQuery.cleanData( getAll( this ) );
+ this.parentNode.replaceChild( val, this );
+ });
},
detach: function( selector ) {
Something went wrong with that request. Please try again.