-
Notifications
You must be signed in to change notification settings - Fork 131
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
Conversation
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. |
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. |
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. |
a way to handle pre-rendered HTML (including the option to do checksum comparison)
Merged in. Thanks! |
No problem! Glad I was able to help. |
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. |
@ashaffer Hi
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:
so in the next PR i'll get this fix.
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 And for the sever-side, we simply need to modify the function 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.
The source of adler32 contains only a few lines of code. So I don't see what is so bad about such dependency. |
Thanks for pointing it out. I have certainly overlooked this. I'll get it fixed in the next few hours. |
preRendered
:If the container has attribute
checksum
: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
:we do nothing to it in the first call to
render
, assuming everything is pre-rendered properly.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 torender
.Related threads: #390 #190
If this commit is accepted, this feature should be included in doc.