Is there something wrong or hard with patching virtual-dom to fix this problem?
A PR is welcome, we tried but it was simply too hard
2 Likes
Very interesting! Moving to a fork of vdom isn’t ideal, but we might be able to consider it as long as we can prove there are no regressions (and we move it to our own GitHub organisation).
Were you able to run the vdom test suite to confirm there are no regressions? How easy would be it be to add a new vdom test for this prepend behaviour?
2 Likes
I’ll first address the second question. This prepend behavior is essentially the same, it just eliminates some extra work. I’ll try to illustrate what vdom does originally and after the patch.
Suppose there are 20 posts and 10 are being loaded on top after scroll.
Originally vdom makes this sequence of transformations:
[20 old] → [20 old, 10 new] // 10 new elements are appended to the end
[20 old, 10 new] → // all 30 elements are removed
→ [10 new, 20 old] // all 30 elements are inserted into their place
After the patch the sequence becomes:
[20 old] → [20 old, 10 new] // 10 new elements are appended to the end
[20 old, 10 new] → [20 old] // 10 new elements are removed from the end
[20 old] → [10 new, 20 old] // 10 new elements are prepended
As you can see there’s still extra work: in principle you can prepend those elements right away, but this way I could just add some code in one place and not bother about rewriting what’s already there.
Each of the transformations becomes a dom operation, so youtube videos start playing because originally old elements are reinserted (which triggers them being rerendered), and after the patch they stay in place.
2 Likes
Here is an excerpt from virtual-dom’s package.json
"scripts": {
"test": "node ./test/index.js | tap-spec",
"dot": "node ./test/index.js | tap-dot",
"start": "node ./index.js",
"cover": "istanbul cover --report html --print detail ./test/index.js",
"view-cover": "istanbul report html && opn ./coverage/index.html",
"browser": "run-browser test/index.js",
"phantom": "run-browser test/index.js -b | tap-spec",
"dist": "browserify --standalone virtual-dom index.js > dist/virtual-dom.js",
"travis-test": "npm run phantom && npm run cover && istanbul report lcov && ((cat coverage/lcov.info | coveralls) || exit 0)",
"release": "npm run release-patch",
"release-patch": "git checkout master && npm version patch && git push origin master --tags && npm publish",
"release-minor": "git checkout master && npm version minor && git push origin master --tags && npm publish",
"release-major": "git checkout master && npm version major && git push origin master --tags && npm publish"
},
Of these ‘test’, ‘dot’, ‘cover’, ‘view-cover’, ‘browser’, ‘phantom’, ‘travis-test’ seem relevant to testing.
‘browser’, ‘phantom’, ‘travis-test’ give a parsing error due to newer javascript constructs in my code. Others pass. If I change this code
var prepend = simulate.every(item => item && item.key)
prepend &&= aChildren.every((item, i) => item.key === bChildren[i + shift].key)
to this code
var prepend = true
for (var i = 0; prepend && i < simulate.length; i++) {
prepend = simulate[i] && simulate[i].key
}
for (var i = 0; prepend && i < aChildren.length; i++) {
prepend = aChildren[i].key === bChildren[i + shift].key
}
then all pass. I can push this change to keep javascript consistently old if satisfying all these scripts is desirable.
2 Likes
please do - let’s keep their existing test suite working
I just forked virtual-dom into GitHub - discourse/virtual-dom: A Virtual DOM and diffing algorithm - please can you make a PR against that repo?
3 Likes
3 Likes
I should also note that my change to virtual-dom is very narrow. It targets prepends specifically and falls back on the original algorithm in all other cases. And that original algorithm is still imperfect if you look at the problem generally (it’s not hard to come up with examples, when it would touch old elements unnecessarily). On the other hand it handles appends, removes, single inserts just fine. And with this you need to get real fancy with the post stream to break out. So practically speaking solving the problem generally might be a bit of an overkill, though of course you can sleep better when you do.
I pushed some new tests. Are you planning to review the PR any time soon?
1 Like
Thanks Aleksey - I will try to get it reviewed within the next week
4 Likes
I’ve got the virtualdom fork set up, and can confirm it solves the iframe issue in manual testing.
@Aleksey_Bogdanov I wonder if you can help me with something though. I’ve been trying to add a test within Discourse. Essentially:
-
Render
<span>ElementOne</span><span>ElementTwo</span> -
Remove ElementTwo, and prepend
PrependedElement -
Check result is
<span>PrependedElement</span><span>ElementOne</span> -
Check that the original ElementOne span is equal to the final ElementOne span (i.e. check it wasn’t re-rendered)
Unfortunately the check in (4) fails, meaning that ElementOne was re-rendered. Any idea why the new logic isn’t working in this case? 
I pushed the new fork configuration and failing test to the existing PR.
3 Likes
Yes, I wrote about this earlier.
I wouldn’t expect this to work generally (and the test as you describe it to pass). Originally, it works for appends, removals, single inserts (perhaps some combination of them). With my change it also works for pure prepends, which should cover the youtube bug. Your test has ‘removal + prepend’. This isn’t covered by my change.
A more robust solution would require a significant rewrite of ‘reorder’ and ‘diffChildren’ in virtual-dom at least. Which I can try, but it begs the question, are you planning to maintain your own fork of virtual-dom long-term? If we’re trying to be robust, it might be a better idea and require less time and effort to try switching to an analogous actively maintained library. My guess is other libraries already solved this problem by now.
2 Likes
I see - that makes sense. However, even if I make it a ‘pure prepend’, I get the same failure. Dropping a breakpoint in the relevant reorder() function shows that it bugs out on the bFree.length === bChildren.length check:
I pushed the updated test to the branch.
This is definitely just a short-term solution. We have already started replacing our use of vdom with Ember/Glimmer rendering. Our aim is to replace the post stream implementation within the next 12 months. Glimmer handles this kind of ‘prepend’ correctly.
So, totally happy to merge an ‘incomplete, but better than before’ change. But, if it’s not too much work, it would be nice to understand why this test isn’t working
3 Likes
Absolutely. I’m not sure yet, but I’ll look into it as soon as I can.
3 Likes
Update.
I added keys for the test to pass. Keys hint to virtual-dom which elements correspond to which. Without them virtual-dom just assumes elements follow the same order as before and can’t figure out it’s dealing with prepends.
Then I changed single prepend to multiple prepends because virtual-dom already covered single inserts (and single prepends are a special case), so they shouldn’t make a difference.
Changes above is what I pushed so far.
But now multiple prepends don’t make a difference, which is surprising. My initial theory of what’s wrong and by extension why my patch works might be flawed. So I’m still trying to figure it out.
I note, David, that you debugged the test in browser. Is it hard to set up? I’m having a hard time setting up a debugger for tests and would appreciate if you could give me some pointers.
3 Likes
Aha I see - thanks for that!
Do you have a working Discourse development setup? If so, start it up, visit /tests in the browser, and then use the filter UI at the top to search for “avoids rerendering on prepend”. Then you can use the browser dev tools to debug. (e.g. go to the sources tab, ctrl+p to open file, search for vdom/diff, and set a breakpoint)
3 Likes
Thank you, I’ll try it.
3 Likes
So, this code
assert.strictEqual(elementOneBefore, elementOneAfter);
doesn’t make a difference because elementOneBefore maintains its identity through all these changes either way.
Here’s a little illustration, if you’d like to see for yourself:
<!DOCTYPE html>
<html>
<head>
<meta charset="utf-8">
<title>Youtube Bug Demo</title>
</head>
<body>
<script>
let iframe = document.createElement("iframe");
iframe.width = 690;
iframe.height = 388;
iframe.src = "https://www.youtube.com/embed/Xc5rB-0ZBcI";
iframe.title = "Strange S.T.A.L.K.E.R. car glitch";
document.body.appendChild(iframe);
let button = document.createElement("button");
button.type = "button";
button.innerHTML = "Reinsert";
button.onclick = function(){
let iframeStart = document.querySelector("iframe");
document.body.removeChild(iframeStart);
document.body.insertBefore(iframeStart, button);
let iframeEnd = document.querySelector("iframe");
alert(iframeStart === iframeEnd);
};
document.body.appendChild(button);
</script>
</body>
</html>
What does make a difference is those ‘removeChild’, ‘insertBefore’ calls, so I added a check for dom mutations. Now the test fails for the previous version and passes for the current one, so hopefully this is sufficient.
6 Likes
That’s great, thanks so much for your work here @Aleksey_Bogdanov. I just merged the PR - it’ll be live on Meta within the next hour.
6 Likes