Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 4 commits into from
Jan 30, 2016

Conversation

archywillhe
Copy link
Contributor

  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.

  1. 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
Copy link
Owner

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
Copy link
Contributor Author

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
Copy link
Contributor Author

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 added a commit that referenced this pull request Jan 30, 2016
a way to handle pre-rendered HTML (including the option to do checksum comparison)
@anthonyshort anthonyshort merged commit 5c2fbef into anthonyshort:master Jan 30, 2016
@anthonyshort
Copy link
Owner

Merged in. Thanks!

@archywillhe
Copy link
Contributor Author

No problem! Glad I was able to help.

@ashaffer
Copy link

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
Copy link
Contributor Author

@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
Copy link
Contributor Author

@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants