6.0 build target with comment nodes

[justinbmeyer]

To make building templates faster, make sure to render with comment nodes.

Currently, can-stache creates placeholder text nodes for anything "live" using can-view-target.

presentation

For stache like start{{#if(foo)}}BAR{{/if}}end instead of placeholder textNodes, create two comment nodes. Resulting DOM would look like:

start<!-- Observe<if(foo)> -->BAR<!-- Observe<if(foo)> -->end

Before:

hydrate([
  "start",
  function(){ this //-> text node 
  },
  "end"
])

hydrate([
  "start",
  {comment: "{{ if(foo) }}", callbacks: [ function(){} ]},
  {comment: "{{ /if }}"
  "end"
])

can-view-target has already been changed to "force" comments instead of textNodes:

https://github.com/canjs/can-view-live/blob/d541c95d04d6db7f6ec2988bc319c98c049328ea/lib/helpers.js#L63

If you have something like {{foo}} ... this should create a placecholder textNode, however, there are times where you can opt out of this, like safeString or components, with those, we'd need to "force" into comment nodes.

Approach

0. Create a performance test
1. Make can-view-live warn if it's forcing a "comment node". You will see a LOT of warnings with can-view-target, ignore those.
2. Make those warnings going away in can-stache.
3. Remove the warnings?

Code you might want to change

instead of just adding a function, add two comment nodes:

can-stache/src/html_section.js

Line 60 in 49fea89

this.last().add(newSection.targetCallback);

should we be forcing comment nodes here:

can-stache/can-stache.js

Line 113 in 49fea89

section.add(mustacheCore.makeLiveBindingPartialRenderer(stache, copyState({ filename: section.filename, lineNo: lineNo })));

... or in the .add() function?

[bmomberger-bitovi]

Check out these tests in can-stache for places where comment nodes are forced outside of can-view-target:

• "{{#each}} handles an undefined list changing to a defined list (Make and() and or() lazy #629)"
• "#each with null or undefined and then a list"
• 'using #each when toggling between list and null'
• "#each(asyncGetter) should only call async getter once (Using stache's each helper causes computes to be called more than once #402)"

Seems like a pattern.

[justinbmeyer]

0. fix can-view-target so :

   (bug is fixed)

   target([{comment: "foo", callbacks: [function(){}]])

   2 hrs

1. create two comment nodes for every range
   WHERE: can-stache/can-stache.js (or HTMLSection)

   [
       {comment: "#if(foo)", callbacks: [
           function(startComment){
               
           }
       ]},
       {comment: "can-end-placeholder"}
   ]
   

   1 hr

2. Makes sure if the next node is not a "can-end-placeholder", we are warning:
   https://github.com/canjs/can-view-live/blob/487f9d90caeef351bc508f8cb3494017a19091bc/lib/helpers.js#L71

   1 hr

3. Run Stache Tests ... are there new weird cases?

   1 hr - 2 days

4. Make it so functions returned by helpers are called with a placeholder.
   For {{#each}}, this should fix:

    1.  Double placeholders.  Change:
   
        <!-- #for(name in people) -->
            <!-- people patcher -->
                brad||justin
            <!-- end patcher -->
        <!-- /for -->
   
        to 
   
        <!-- #for(name in people) -->
           brad||justin
        <!-- /for -->
    2. Eliminate the warnings from #3  
   

   We will have to change:

    can-view-live.html - 
    can-stache/core - https://github.com/canjs/can-stache/blob/bd5c98979f1cc770b2726f2cc932c0652fe2f8e6/src/mustache_core.js#L379
   

   4 hrs

5. Check can-component / can-stache-bindings

6. Instead of an ending comment node like: {comment: "can-end-placeholder"}
   We will need to change that to: {comment: "/for"}

   At min remove: next.nodeValue === "can-end-placeholder"

   Possibly:

   range.create(el){
   return {start: el, end: el.nextSibling};
   }

   Can we change can-view-live's tests to always make sure this is the case?
   What about can-component?

[justinbmeyer]

@bmomberger-bitovi can you give an update on where you got on this?

[bmomberger-bitovi]

What's complete by package:

• can-view-target has a new version out (4.1.6) that fixes comment node callbacks (point 0 above)
• can-stache has all of its tests re-enabled except for those surrounding nodelists. The test battery passes green on Travis (point 3). It also is using comments for section callbacks (point 1). These updates are published to the onNodeRemoved branch
• can-view-live is now warning when the range is being created without an end placeholder already existing (point 2). these updates are published to the onNodeRemoved branch.

Still to do:

• point 4: no progress yet. Changes to can-view-live and can-stache still needed
• point 5: Blocked by point 4.
• point 6: made a concession to renaming the can-end-placeholder in range.create() so it has "/" and the expression, but this still needs to be refined after comment tags are readied elsewhere (blocked by point 4)

[phillipskevin]

For, point 4, running the can-stache tests no longer produces any of these warnings:

can-view-live: creating an end comment for ...

can-view-live: forcing a comment range for ...

There are still a ton of these warnings: https://github.com/canjs/can-view-live/blob/805b4757a6e77db46737daf9ae2093c28338a434/lib/helpers.js#L20

Can you explain what this case is @justinbmeyer ?