6.0 build target with comment nodes

justinbmeyer commented 5 years ago

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.

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

Create a performance test
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.
Make those warnings going away in can-stache.
Remove the warnings?

Code you might want to change

instead of just adding a function, add two comment nodes: https://github.com/canjs/can-stache/blob/49fea89e3f919e21c941cb8c23090c61857a46ce/src/html_section.js#L60

should we be forcing comment nodes here: https://github.com/canjs/can-stache/blob/49fea89e3f919e21c941cb8c23090c61857a46ce/can-stache.js#L113

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

bmomberger-bitovi commented 5 years ago

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 (#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 (#402)"

Seems like a pattern.

justinbmeyer commented 5 years ago

fix can-view-target so :

(bug is fixed)

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

2 hrs

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

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
Run Stache Tests ... are there new weird cases?

1 hr - 2 days

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

Check can-component / can-stache-bindings
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 commented 5 years ago

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

bmomberger-bitovi commented 5 years ago

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 commented 5 years ago

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 ?

canjs / can-stache

6.0 build target with comment nodes #701

Approach

Code you might want to change