Fusejs FuseJS - Core

Comments and changes to this ticket

• You flagged this item as spam.
  

  Kit Goncharov August 10th, 2009 @ 12:09 PM

  • State changed from “new” to “open”

  Definitely a great idea. The only problem with that, though, is that Element#insertBefore is already a DOM method, and totally different than what Element#insert({bottom:...}) does (plus, it's used by the insertion translations, so overwriting it wouldn't work and would definitely break a lot of scripts). Maybe renaming them to something like Element#insertAtBottom, insertAtTop, etc. would make more sense, since that's what Element#insert really does.

  FWIW, here's how my Element#insert looks in Miniatures (adapted for Fuse). It's definitely faster than Prototype's, but performance could considerably be improved by splitting it into individual methods:

  var toString = Object.prototype.toString, hasKey = Fuse.Object.hasKey;
  function insert(insertions){
          var content, index, length, position;
          for(position in insertions){
              if(hasKey(insertions, position)){
                  content = ((toString.call(insertions[position]) === '[object Array]') ? insertions[position] : [insertions[position]]);
                  switch(position.toLowerCase()){
                      case 'after':
                          for(index = content.length - 1; index >= 0; index--){
                              //Loop in reverse.
                              this.parentNode.insertBefore(content[index], this.nextSibling);
                          }
                          break;
                      case 'top':
                          for(index = content.length - 1; index >= 0; index--){
                              //Loop in reverse.
                              this.insertBefore(content[index], this.firstChild);
                          }
                          break;
                      case 'before':
                          for(index = 0, length = content.length; index < length; index++){
                              this.parentNode.insertBefore(content[index], this);
                          }
                          break;
                      default:
                          //Insert at the bottom of the element if no insertion point is specified (or if it's not `top`, `after`, or `before`).
                          for(index = 0, length = content.length; index < length; index++){
                              this.appendChild(content[index]);
                          }
                          break;
                  }
              }
          }
          return this;
      }
  

• 

  John-David Dalton August 10th, 2009 @ 12:32 PM

  Quick notes:

  Kit keep in mind within a week or so Fuse will be completely off of the native Element objects and instead use Fuse.Dom.Node. so naming conflicts will not be a concern.

  Also the use of Fuse.Object.hasKey is expensive esp when the keys we are looking for aren't know to cause problems so a simple for loop without a check should be fine.

  Fuse has a Fuse.Object.isArray().. so we don't need to duplicate that usage here.

• 

  John-David Dalton August 10th, 2009 @ 12:36 PM

  For reference FuseJS's current insert method:
  https://github.com/jdalton/fusejs/blob/36dfcde01016b45f02544b130e750fa7190f964c/src/dom/element/element.js#L563

• You flagged this item as spam.
  

  Juriy Zaytsev August 10th, 2009 @ 12:40 PM

  Kit,

  you see, most of complexity in Prototype's insert lies in its overload-ish nature:

  1) It can take an object of insertions or a single insertion.
  2) It can take up to 4 different kind of insertions.
  3) For any of 4 insertions, it can take something that has `toElement` and delegate to that method.
  4) For any of 4 insertions, it can take something that has `toHTML` and delegate to that method.
  5) For any of 4 insertions, it can take string value and turn it into a set of elements.
  6) For any of 4 insertions, if value is a string it can normalize troublesome insertions (such as table-related ones)
  7) For any of 4 insertions, if value is a string it will strip scripts and evaluate them.
  8) For any of 4 insertions, if value is not a string it will turn it into a string before proceeding.
  9) And finally, it considers something to be an element only when that something has `nodeType == 1`
  

  It's pretty clear how prototype's insert gets burried under all these convenience "layers" and is noticeable slower than something as simple as a wrapper around insertBefore, appendChild and other native methods.

  I was just experimenting with breaking insert up into smaller chunks, but it looks like it all leads to some serious repetition :/ I'll play with it a bit more.

• You flagged this item as spam.
  

  Kit Goncharov August 10th, 2009 @ 04:34 PM

  Wow, fast responses, guys! :)

  @JDD: I'm using Fuse.Object.hasKey as a replacement for Object#hasOwnProperty. I agree it's not really needed, but, in case someone decides to extend Object.prototype, this would be a lifesaver. On an unrelated side note, I have used Fuse.Object.hasKey/Object.definesOwnProperty to unintentionally make Prototype's Object.toQueryString compatible with arrays and other objects (see https://prototype.lighthouseapp.com/projects/8886/tickets/126-toque... ), so it's a handy tool to have to filter unwanted properties from the prototype.

  I dig the fact that Fuse will have its own DOM namespace, but I'm still a bit hesitant about using insertBefore, insertAfter, etc. as the method names -- it's nonintuitive to those who are familiar with the W3C method (also, the heavily-simulated W3C-style insertAfter follows the insertBefore scheme, so that creates an opportunity for confusion). Element#insert inserts code at the bottom, top, before, or after the element, so maybe something like insertBefore/AfterElement, and insertAtTop/BottomOf would be more descriptive.

  @Juriy: So that explains why I had such a hard time understanding Prototype's code! :D I think we should go ahead and just allow unlimited arguments for our individual methods -- if there's only one argument passed, we should just perform the insertion without looping for performance. For the sake of consistency, I also think we should go ahead and make the insertion element-only -- essentially, we defer to the W3C DOM methods to see if the insertion is valid or not (for example, if a string is passed, the DOM methods will automatically throw a NOT_FOUND error).

• 

  John-David Dalton August 10th, 2009 @ 05:18 PM

  • State changed from “open” to “new”

  @kit: Bonus! You helped uncover a bug in Fuse :D. Fuse uses Fuse.Object._each() inside Fuse.Object.toQueryString() which is like a corrected form of for-in so it helps find shadowed props and fixes quirks with for-in but we don't check Fuse.Object.hasKey there... a fix is coming soon.

  Update: Resolved the Fuse.Object.toQueryString() bug here https://github.com/jdalton/fusejs/commit/2e52e54cec26fd3150522efa82e6c9db3ace4231

• 

  John-David Dalton August 11th, 2009 @ 10:44 AM

  Fuse currently uses document fragments when converting strings to html elements.
  It does so by:
  

   1) creating a dummy node for the context (current doc or iframe)
  
  
  
  
  
  
  
   2) creating a document fragment for the context (current doc or iframe)
  
  
  
  
  
  
  

  fragments don't play nice with elements from other frames thats why we must have a
  fragment for each context. All dummy nodes and fragments are cached and recycled for
  future calls
  

  
  
  
  
   3) Using the translation table to work around issues with tables, select elements and others
  
  

  we write the html to the dummy div
  

  
  
  
  
   4) We pass the dummy node to the getContentAsFragment(cache, node) internal method that then
  
  

  grabs the child nodes by either
  a) IE inserting the dummy node into the fragment and calling removeNode() (which leaves the child nodes)
  b) Or if document range is supported create a range and selectNodeContents of the dummy node and append the extracted contents to the fragment
  c) Or manually iterate over the dummy node inserting the children into the fragment
  

  
  
  
  
   5) return the fragment and insert into target dom
  
  
  
   Note: when fragments are inserted into elements they auto-clear their contents and are ready to be used again.
   There is no need to clone document fragments we can simply re-use the same fragment over and over.
  

• You flagged this item as spam.
  

  Kit Goncharov August 11th, 2009 @ 10:59 AM

  If document.implementation.createHTMLDocument is available, we could
  use that instead of a document fragment. AFAIK, WebKit is the only one
  that supports it -- Firefox definitely doesn't; I don't know about IE.

• 

  John-David Dalton August 11th, 2009 @ 11:11 AM

  what are the benefits of using createHTMLDocument ?

• You flagged this item as spam.
  

  Kit Goncharov August 11th, 2009 @ 11:52 AM

  @Diego: Nope, I meant createHTMLDocument. :) Try it in Safari:

  var htmlDoc = document.implementation.createHTMLDocument('');
  //Safari 2.x doesn't create a body for this document, so we have to create our own content element
  var htmlEl = htmlDoc.createElement('content');
  //Now create our fragment
  htmlEl.innerHTML = '<p>I am a <span>series</span> of HTML nodes!</p>';
  //And import the content element into the current document
  var importedElement = document.importNode(htmlEl, true);
  

  The problem with using createDocument is that it doesn't create an HTML document, so it doesn't support innerHTML (which really isn't part of the spec to begin with). Your code snippet will create an innerHTML property on the documentElement, but it'll do nothing (i.e., there's no setter defined for it). If you inspect base.documentElement after setting the innerHTML, you'll see that it's still just a single node. Also, before you set the innerHTML of the element, 'innerHTML' in base.documentElement === false.

  @JDD: I guess there really isn't a significant advantage. I just think it'd be more efficient for browsers that support it, since you don't have to worry about cross-frame issues, etc (you can just use ownerDocument and essentially convert an HTML string to a series of nodes in 4 lines). I definitely like that we're using document fragments instead of Prototype's approach, though (creating a DIV in the context of the current document, then setting its innerHTML, which is quirky on XHTML, etc.)

  On the other hand, if only WebKit supports this, then maybe it's not such a good idea, after all.

• 

  John-David Dalton August 11th, 2009 @ 11:57 AM

  @kitsg: Have you tried appending an element from another iframe to the htmlDoc object of the primary document ?

• You flagged this item as spam.
  

  Kit Goncharov August 11th, 2009 @ 11:58 AM

  No, you have to use htmlDoc.importNode for that.

  I have an errand to run, but I'll whip up some testcases when I get back (including documentFragments, too, á la Fuse).

• 

  John-David Dalton August 21st, 2009 @ 11:08 PM

  • Assigned user changed from “John-David Dalton” to “Joe Gornick”
• 

  Joe Gornick August 26th, 2009 @ 01:39 AM

  So, after reading all the comments, I have a few of my own ...

  1. I agree with Kit that we should not name methods after natives. It could get really confusing especially when working with the raw property on the Fuse.Dom.Node wrapper.

  2. Before I go in an make these changes, I'm assuming we are going to split up into separate methods, but also trim down what arguments are accepted by the insert* methods?

  @Juriy: did you have any luck with splitting up the insert method?

• 

  Joe Gornick October 7th, 2009 @ 01:08 AM

  • State changed from “new” to “patched”
  • Assigned user changed from “Joe Gornick” to “John-David Dalton”

  My commit is here: https://github.com/jgornick/fusejs/commit/b819f42cfc57433620d7ababf...

• 

  John-David Dalton April 15th, 2010 @ 02:36 AM

  • State changed from “patched” to “resolved”

  Resolved http://github.com/jdalton/fusejs/commit/b46f6b957484190a34028205727e5c6f936559a3