Avoid unnecessary rerenders at init #512
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
✅ Deploy Preview for elm-pages-todos canceled.
|
|
This is fantastic, thank you @lydell! I went ahead and merged it because the changes you've made so far look safe in that they won't make things any worse, even if it doesn't fully complete the work of making it diffable. Thank you for taking the time to work on this! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
When an Elm app boots, it “virtualizes” existing DOM and diffs against that on the first render. If the existing DOM was similar to the return value of the first
view, not much of the DOM needs to be updated.https://github.com/elm/browser/blob/1.0.2/src/Elm/Kernel/Browser.js#L78-L85
Most Elm apps render into empty or near-empty nodes, so that is rarely relevant. But elm-pages server-side renders views, so it can take advantage of this feature!
However, there is currently a mismatch between the server rendered HTML and what the Elm view produces. The diff at init looks like this:
<body>vs<body><div data-url>vs<div data-url><main>vs<div id="elm-pages-announcer"><main>Since
<div id="elm-pages-announcer">is missing in the server-side rendered HTML, it throws off the diffing forcing Elm to basically rerender the entire page.Then, to make things more complicated, whitespace matters too. When hand-writing HTML, we often put newlines and indentation among elements to make things more readable. However, that results in text nodes with only whitespace! Usually they don’t matter at all, but they do matter when virtualizing DOM. Elm can’t know which text nodes are important and which ones aren’t.
<b>one</b> <b>two</b>has a whitespace-only text node which is important – without it the text renders as “onetwo” instead of “one two”. So just adding<div id="elm-pages-announcer">to the server-side rendered HTML isn’t enough – there are some whitespace text nodes that throw things off as well.This PR:
<div id="elm-pages-announcer">to the server-side rendered HTML.<body>.I found this when working on my fork of elm/virtual-dom. Which brings me to … this PR only avoids unnecessary rerenders in theory. Because as soon as you wrap something in
Html.maporHtml.Lazy.lazy, everything inside them is going to be rendered anyway. elm-pages doesList.map (Html.map UserMsg) bodyto the user’s view, so basically the whole page is wrapped inHtml.map. The reason this causes a rerender, is because elm/virtual-dom’s virtualize function cannot know wheremapnodes should be, and the diffing algorithm bails when amapnode is diffed with a regular node. My fork of elm/virtual-dom handles this, though, so when that is finished, this PR will have value for those who choose to use my fork!My fork isn’t done yet, though, but I thought I’d make this PR while I still remember all the details.