Skip to content

Conversation

@markelog
Copy link
Member

Alternative to #2206

-3 bytes to gzipped size

Copy link
Member Author

Choose a reason for hiding this comment

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

Or

if ( arg && (arg[ 0 ] || arg.nodeType) && typeof arg !== "string" ) {
    return this;
}

// Force removal if there was no new content (e.g., from empty arguments)
return this.remove();

Which i actually like better, since remove would not be called as much

@gibson042
Copy link
Member

I believe this will still fail on $el.replaceWith( function( old ) { return textToBePrefiltered; } ).

@markelog
Copy link
Member Author

@gibson042 could you post an jsfiddle?

@markelog
Copy link
Member Author

Perhaps i didn't get your example, since this -

htmlPrefilter = jQuery.htmlPrefilter

jQuery.htmlPrefilter = function( html ) {
       // Remove <script> and <del> elements
       return htmlPrefilter.apply( this, arguments )
           .replace( /<(script|del)(?=[\s>])[\w\W]*?<\/\1\s*>/ig, "" );
   };

 $("div").replaceWith(function() {
    return "<script>alert(1)<\/script>";
 });

with conjunction with #2203 works for me

@gibson042
Copy link
Member

Your example works only because the replacer function returns non-empty. I put together a jsFiddle, and actually uncovered (and fixed) a bug in #2206: http://jsfiddle.net/cdhxqwk4/. Observe how this PR erroneously removes all the original elements.

I just want to stop being so damn clever with .replaceWith and get to a point where it does exactly what it says, which I think is achieved by #2206.

@markelog
Copy link
Member Author

@gibson042 this is a lot of code, could you provide simplified example? Just like we ask other people to do?

I just want to stop being so damn clever with .replaceWith and get to a point where it does exactly what it says, which I think is achieved by #2206.

Yeah, your way is more robust and this logic is vague at best, but #2206 complicates a lot in order to fix a very-very edge case.

@markelog
Copy link
Member Author

Just a gist or PR with tests would do

@gibson042
Copy link
Member

Sorry; I thought the fiddle was a clear comparison between the two PRs that demonstrated errors in this one. Straightforward tests are already at https://github.com/jquery/jquery/pull/2206/files#diff-328a9410239da61336e3662fdbfe9bf6

As for the rest, .replaceWith is full of sharp edges, but I believe that #2206 actually simplifies the logic now, albeit by adding a new parameter to buildFragment and domManip. Trying to guess at possibly-dynamic arguments is just begging for problems, and requiring a this.remove() to clean up after the domManip callback is indicative of bad implementation.

@markelog
Copy link
Member Author

Okay, although those are really weird test-cases, i fully agree with your point.

@markelog markelog closed this Apr 21, 2015
@lock lock bot locked as resolved and limited conversation to collaborators Jan 19, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Development

Successfully merging this pull request may close these issues.

3 participants