-
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 better way to handle pre-rendered HTML (including: event handler registration) #396
base: master
Are you sure you want to change the base?
Conversation
i think you mean someone else :) |
He meant @ashaffer :) |
Thanks @0a-, I'll take a look at this tonight or tomorrow night. |
@anthonyshort Sure. Thanks! |
(just amended the commit |
I was just looking over #395, #396 and their comments. I noticed @ashaffer pointed out that comparing strings would be quicker. @0a-, I see that you benchmarked this - I thought that maybe the size of the elements html might be a factor (i.e. computing one checksum server where you could use a native c/c++ implementation might be faster for some very large element.innerHTML), so i took a look and it's not;
So far as I can tell, It will never make any sense to include the checksum option or dependancy. Computing two checksums and then comparing them takes 5-10x longer than just comparing the strings. Computing 1 hash will still take ~2.5-5x longer than just comparing the strings - regardless of the size of the strings. Also, #396 traverses the server rendered html and then attaches event listeners where necessary. Is checking the two strings for equality (or hashing them), then attaching all event listeners actually going to be faster than just rendering the whole thing? |
Interesting. I wonder why React decides to use the checksum approach.
Good point. I've given that some thought as well. I'm not so sure which one would be better in terms of speed. However, one thing we can be sure of is that (I'm talking about Blink here which is used by Chrome. Safari's WebCore and Firefox's Gecko should do a similar thing) the first step of rendering the DOM element on screen involves traversing the DOM tree and creating a tree of RenderObjects from it, where each RenderObject is associated with a RenderLayer:
And that is just the start of it. You can learn more about how Blink works on its website. Comparing to the task of displaying graphic on the screen based on a DOM tree, the corresponding style information from the CSS and the dimensions of the browser, all we are doing here is to check for strings equivalence, traverse a tree and attach each eventListener in the tree to the already-rendered elements (and note: to re-render the strings we would need to attach the event listeners as well. In this implementation we are not attaching eventListeners to the un-rendered DOM (from the virtual DOM for string comparison). So the same number of eventListeners would need to be attached if we are going for the re-rendering approach.) A good thing about not re-rendering is that there wouldn't be any possible flicking on the page when a user first arrives. |
This looks good to me. Although I think the rendering options are maybe not necessary. The browser should be smart enough to avoid significantly impacting performance if you are just resetting all the attributes to the same value. |
@ashaffer You mean the re-rendering? I am not sure, because in the previous implementation we are doing But this doesn't seen to prevent the flickering, which is what I want to solve here. |
@0a- Sorry I don't mean caching of the elements themselves. I meant more like re-rendering over the same attributes. Like, say you have pre-rendered: <img src='/thing.jpg' width='40' /> If you just call setAttribute again to set those attributes to the same value, I think the performance impact will be negligible as opposed to adding options to set only the event listeners. |
@ashaffer Oh I see. Yeah that makes sense. |
@0a- It occurred to me that you should probably be comparing the length of the strings before actually comparing the strings. I updated that benchmark; if the strings are different, this:
will be much, much faster in all cases except where the strings coincidentally are the same length. |
Most of the time the strings should be equivalent. Otherwise that means there is something seriously wrong with the approach used by the server to pre-render the HTML. The purpose of this implementation is to to fix things if the unusual occurs i.e., the server-rendered string happens to not match the to-be-rendered strings. The strings shouldn't be the same length "coincidentally". In most cases they should be equivalent and thus the same length. |
Hi @0a-
I'm aware that this is just a check to make sure the server rendered correctly. Here is how this would work; First, check the server rendered html is the same length as what the client would render (comparing the lengths is super trivial, my 2013 MBA can do this at a flat rate of 64 Million times a second, regardless of the length of the strings)? if so, go to number 2, if not (which I understand will generally not be the case) begin the _relatively_ expensive process re-rendering everything. Second, check the server rendered html is equivalent to what the client would render? If so just re-apply the attributes, if not re-render everything. The idea is that in the edge (or at least uncommon) case where Of course, the perf improvement for re-rendering bad server markup is going to be less than a microsecond, so this was just a suggestion. |
Huge thanks to @aeosynth for the feedback on the previous PR ( #395 ). I have now fixed a few things that were overlooked in the previous implementation:
Now the event handler registration works with any pre-rendered HTML.
Turns on this requires reconstructing the
createElement
function slightly so that there is a clear separation of side-effect from the creation of DOM Element (from the Virtual DOM). As a result we can now execute the side-effects on a DOM element of our choice (i.e., in this case, the pre-rendered element) without making a great mess.To prevent pre-rendered content data from "flickering" ( prevent re rendering of server rendered component on the client with same data #190 ) I have also added an option parameter for
.setAttribute
. We can now choose to filter out some side-effects on the DOM elements that we don't. (E.g. When running the side-effects on a pre-rendered DOM element, we are only concerned with those that add eventListener to it and not those that rewrite its attributes. The filtering is now done byoption.onlyEventListeners
).On the client-side, for the self-healing feature to kick in, a user can now choose to set either the attribute
autoFix
for string-comparison orchecksum="{value generated on the server-side}"
on the container.