anthonyshort / deku

Render interfaces using pure functions and virtual DOM
https://github.com/anthonyshort/deku/tree/master/docs
3.41k stars 130 forks source link

a way to handle pre-rendered HTML (including the option to do checksum comparison) #395

Closed archywillhe closed 8 years ago

archywillhe commented 8 years ago

1) For container with the attribute preRendered:

If the container has attribute checksum:

<div id="container" preRendered checksum>server rendered </div>

In the first call to render, we compute checksum for both the pre-rendered HTML and the to-be-rendered HTML, and destroy & recreate the element if there's difference in the checksums.

If the container doesn't have attribute checksum:

<div id="container" preRendered>server rendered </div>

we do nothing to it in the first call to render, assuming everything is pre-rendered properly.

2) The default behavior for containers without the attribute preRendered is to blast away everything it contains (if it contains something) before appending the HTML element created from the virtual DOM in the first call to render.

Related threads: #390 #190

If this commit is accepted, this feature should be included in doc.

anthonyshort commented 8 years ago

Awesome! This is great. The only thing that I'm not sure about is the way the attributes work. It seems like we could just assume that there is pre-rendered content if there are children within the container. So on the first render it would do something like this:

if (container) {
   if (container.children.length === 0) {
     container.appendChild(node)
   } else {
     let preRendered = adler32(container.innerHTML)
     let toBeRendered = adler32(node.outerHTML)
     if (preRendered != toBeRendered) {
       container.innerHTML = ''
       container.appendChild(node)
     }
   } 
}

That way the user doesn't really need to think about it. They just put content in there any deku takes care of the rest.

archywillhe commented 8 years ago

Thanks!

I wasn't sure whether there would be a situation where there are unwanted stuff in the container (i.e. a situation where the container's got some unrelated elements in it and the user expects that these stuff would be replaced by the vnode). Now that you mention it, I think I've just been overthinking.

I have amended the code to set that as a default behavoir, tested it and did a force-push.

p.s. in this case the emptying container test would need to be omitted.

archywillhe commented 8 years ago

I've just updated the doc & added a page on working with pre-rendered HTML where I mention about the default behavior & how to turn on the self-healing feature.

anthonyshort commented 8 years ago

Merged in. Thanks!

archywillhe commented 8 years ago

No problem! Glad I was able to help.

ashaffer commented 8 years ago

Hey guys, this looks really awesome. I have a couple of questions though:

if (container.attributes.checksum){
  let preRendered = adler32(container.innerHTML)
  let toBeRendered = adler32(node.outerHTML)
  if(preRendered != toBeRendered){
    container.innerHTML = ''
    container.appendChild(node)
  }
}

Is there a reason you're not using the existing checksum attribute? It seems like if you're going to compute two checksums, you might as well just compare the strings. But since there's already a checksum, it would make sense to just assume it's correct, no?

Secondly, how does all this work with event handler registration? It seems to me like event handlers won't be registered on the pre-rendered nodes, but maybe i'm missing something.

EDIT: I see now that the checksum attribute doesn't actually contain a checksum. But it's faster to compare two strings directly than to compute the checksum of two strings and compare those, and doesn't require a dependency. But if you do want to use checksums, it'd make sense to compute one on the server and one on the client, and save the server-side one in the checksum attribute.

archywillhe commented 8 years ago

@ashaffer Hi

I see now that the checksum attribute doesn't actually contain a checksum. But it's faster to compare two strings directly than to compute the checksum of two strings and compare those

You are right that it is faster to compare 2 strings directly than comparing their checksums, as confirmed by some benchmarkings I just did with benchmark.js. Here is one of the result:

string equivalence x 10,044 ops/sec ±0.76% (86 runs sampled)
checksum comparsion x 1,862 ops/sec ±0.84% (82 runs sampled)
string equivalence is faster

so in the next PR i'll get this fix.

But if you do want to use checksums, it'd make sense to compute one on the server and one on the client, and save the server-side one in the checksum attribute.

Yes, certainly. That's what I plan to add in later on, perhaps with something along the line:

let isRenderedCorrectly = true
if(container.attributes.checksum){
  isRenderedCorrectly = container.attributes.checksum ===  adler32(node.outerHTML)
}else if(container.attributes.autoFix){
  isRenderedCorrectly = container.innerHTML ===  node.outerHTML
}
if(!isRenderedCorrectly){
  container.innerHTML = ''
  container.appendChild(node)
}

So instead of having checksum as the attribute, for this feature to kick in, we have autoFix (which should make things less confusing).

And for the sever-side, we simply need to modify the function .renderString such that it has the option to return the checksum.

I think it is always good to have a fallback for the string-comparison on the client-side because we should consider the possibility that a user does not go for the isomorphic js approach and the convert-HTML-string-into-checksum-then-append-it-to-the-container-element-attribute function may not come in handy on the server-side.

and doesn't require a dependency.

The source of adler32 contains only a few lines of code. So I don't see what is so bad about such dependency.

archywillhe commented 8 years ago

@ashaffer

Secondly, how does all this work with event handler registration? It seems to me like event handlers won't be registered on the pre-rendered nodes, but maybe i'm missing something.

Thanks for pointing it out. I have certainly overlooked this. I'll get it fixed in the next few hours.