Handle the case of an empty string. Fixes #13401 #1162

Closed
wants to merge 2 commits into
from

Conversation

Projects
None yet
5 participants
Contributor

markraddatz commented Feb 6, 2013

This will fix .replaceWith() when you pass an empty string.

src/manipulation.js
@@ -433,7 +433,7 @@ jQuery.extend({
for ( ; i < l; i++ ) {
elem = elems[ i ];
- if ( elem || elem === 0 ) {
+ if ( elem || typeof elem === "string" || elem === 0 ) {
@jaubourg

jaubourg Feb 6, 2013

Member

Why not elem === "" instead?

@staabm

staabm Feb 6, 2013

Contributor

the test also handles the case of " ", which wouldn't work with elem === ""

@jaubourg

jaubourg Feb 6, 2013

Member

... but would have been caught by elem since !!" " === true.

@markraddatz

markraddatz Feb 6, 2013

Contributor

elem === "" should work as well.

@@ -433,7 +433,7 @@ jQuery.extend({
for ( ; i < l; i++ ) {
elem = elems[ i ];
- if ( elem || elem === 0 ) {
+ if ( elem || elem === "" || elem === 0 ) {
@gibson042

gibson042 Feb 7, 2013

Member

-3 bytes by changing this condition to elem != null && elem !== false.

Member

gibson042 commented Feb 7, 2013

I approve of this pull, but it changes the behavior of other manipulation methods (append/prepend/before/after and *To/insert* inversions) and thus may not be fit for a patch release. But there is a regression with respect to 1.8 replaceWith, so maybe we should special-case "" or something (I'm open to suggestion on that).

Contributor

markraddatz commented Feb 7, 2013

From this perspective I totally agree that the change causes more harm than good. I prefer to have a lean childNodes list without clutters of empty strings. I opt in for a solution that uses the 1.8 replaceWith way without domManip.

Member

gibson042 commented Feb 7, 2013

@markraddatz Do you want to submit another pull request with that approach? I'd suggest keeping the .domManip dependency but just adding a value === "" check in .replaceWith that either redefines value or (probably better) skips .domManip in favor of .remove.

Contributor

markraddatz commented Feb 7, 2013

I submitted a new pull request that skips .domManip for this special case.

Owner

dmethvin commented Feb 7, 2013

Closing in favor of gh-1163.

@dmethvin dmethvin closed this Feb 7, 2013

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