search

ded / bonzo

library agnostic, extensible DOM utility
Other
1.32k stars 137 forks source link

Closed #125: .replaceWith() now works when replacing a node with itself. #126

Closed ColemanGariety closed 11 years ago

ded commented 11 years ago

yep. this works

rvagg commented 11 years ago

is there any reason we might want to check for this[0].parentNode before assuming it exists?

ColemanGariety commented 11 years ago

@rvagg @ded only if we want to support replacing of the document element itself. The <html> element's parentNode is the document itself.

However, since document instanceof Element === false, I'm not sure what's best here.

Should bonzo be working on things that aren't elements?

rvagg commented 11 years ago

what about detached elements, is there a use-case for that? does the original version even cover that use-case? does it even make sense to do a replaceWith() on the root of a detached element??

just trying to be thorough about this.

ColemanGariety commented 11 years ago

@rvagg using jQuery source as a reference, you should be able to do replaceWith() on a detached element, but not on the document object.

That said, should bonzo throw an error at the first moment you try and use it to grab the document?

Something like, "Error: 'document' isn't a valid element node."?

rvagg commented 11 years ago

so a parentNode isn't going to work in that case then?

mind you, if you're needing to do a replaceWith() on a detached element then what on earth are you doing??

ColemanGariety commented 11 years ago

@rvagg keep in mind that's only using jQuery as a reference, which doesn't always make the most sound decisions.

I think the only sane option here is to throw an error if you try and use replaceWith on an element without a parentNode. The only two instances I can think of are detached elements and the document node.

rvagg commented 11 years ago

well given that it'll already throw an error then perhaps that's enough; @ded opinion on this? is this enough of an edge-case to not care about?

ColemanGariety commented 11 years ago

@rvagg The detached element error that's already being thrown makes sense, but I think one at the bonzo() level is necessary too: "Error: 'document' is not a valid element node."

ded commented 11 years ago

these are the annoying things that perhaps a DOM library should in fact solve for. afterall, why use a library. the html element feels less of a usecase (i can't say i've ever had to do that). but detached nodes should work given we have a few other things that work on detached nodes.

ColemanGariety commented 11 years ago

@ded @rvagg I've written a new version of replaceWith() and submitted a pull. it seems to work fine, however, a couple of the tests are failing.

Maybe one of you can figure out why?

replaceWith: function (node) {
  var that = this

  return this.each(function (el, i) {
    each(normalize(node, that, i), function (node) {
      if (el.parentNode) el.parentNode.replaceChild(node, el) // Replace the element if it exists in the DOM
      that[i] = node // Replace it in the Bonzo instance in case it's detached
    }, null, 1)
  })
}

It works in the two cases we described:

$('div').detach().replaceWith('foo') //=> { 0: #textNode }
$('html').replaceWith($('html')) //=> { 0: #divNode }
rvagg commented 11 years ago

yeah, those tests are probably my domain, I'll have a look when I have a spare moment.