Handling external changes
nbianca opened this issue · 11 comments
Hello,
I am changing the contents of an element using both diffhtml's innerHTML and element's innerHTML field.
In the example below you can see what happens when you use only innerHTML to set the content ('#example1') and diffhtml's innerHTML ('#example2').
Steps to reproduce:
- Use the script below.
- Press the first button to do an initial render, using innerHTML and diffHTML's innerHTML.
- Press the second button to simulate an external change, using innerHTML (it will not actually change the content).
- Press the third button to do another rendering, similar to the first one, but with a different content. Instead of seeing the expected text and image, you will see two images when using diffHTML.
I suspect that some internal state is messed in step 3. Is there any chance to make the two methods for updating content play nice together?
<html>
<head></head>
<body>
<input id="step1" type="button" value="Step #1: Initial render" />
<input id="step2" type="button" value="Step #2: External change" />
<input id="step3" type="button" value="Step #3: Second render" />
<h1>innerHTML</h1>
<div id="example1"></div>
<h1>diffHTML</h1>
<div id="example2"></div>
<script src="https://unpkg.com/diffhtml/dist/diffhtml.min.js"></script>
<script type="text/javascript">
const { innerHTML } = window.diff;
const step1 = `
<p>
<a href="https://via.placeholder.com/150.jpg">
<img src="https://via.placeholder.com/150.jpg">
</a>
</p>
`;
document.getElementById('step1').addEventListener('click', () => {
document.getElementById('example1').innerHTML = step1;
innerHTML(document.getElementById('example2'), step1);
});
document.getElementById('step2').addEventListener('click', () => {
document.getElementById('example1').innerHTML = step1;
document.getElementById('example2').innerHTML = step1;
});
const step3 = `
<p>Hello world!</p>
<p>
<a href="https://via.placeholder.com/150.jpg">
<img src="https://via.placeholder.com/150.jpg">
</a>
</p>
`;
document.getElementById('step3').addEventListener('click', () => {
document.getElementById('example1').innerHTML = step3;
innerHTML(document.getElementById('example2'), step3);
});
</script>
</body>
</html>Very nice reduced test case. The problem is clear now. diffHTML currently only respects external changes if they are different from its previous markup. This is not good. Because as you have found, when the markup is replaced, but identical, the engine believes the markup has not changed. This means it doesn't know to rescan the DOM and learn about the replaced image.
The fix here is sort-of straightforward. Whenever innerHTML/outerHTML call is made with diffHTML, we must first loop the childNodes and ensure they are accounted for internally, otherwise we rebuild the tree. This will account for top level changes, as well as nested changes without too much of a performance hit (as we are only looping direct top-level elements).
I have the fix implemented and will add some tests later. Hopefully can release it later today or tomorrow. In the meanwhile, if you want to be unblocked, you can do this:
const { release, innerHTML } = window.diff;
/* ... */
document.getElementById('step3').addEventListener('click', () => {
document.getElementById('example1').innerHTML = step3;
// Call release before modifying again so that diffHTML will rescan the entire node.
// Remove this once diffHTML new version comes out.
release(document.getElementById('example2'));
innerHTML(document.getElementById('example2'), step3);
});Thanks again, @nbianca!
I ended up going with a MutationObserver. This ensures that all changes are accounted for in the mounted element. Not the most ideal, but it does work perfectly to ensure we do not operate on a DOM that has been modified and not rectified. Maybe in the future this could be done while syncing, but for now this solves the problem in an elegant way.
Just a heads up @nbianca, this was released today in 1.0.0-beta.19. Let me know if you encounter any issues with the upgrade.
@nbianca Hrm that would be strange since there were tests added to validate the behavior: https://github.com/tbranyen/diffhtml/pull/219/files#diff-bd6bb9c981381ec90b4931ef2d0338fa1cc2d1de20af4fe8e85141e1522584fb
Is the reduced test case in this thread breaking for you?
Edit: I'm seeing it break. Glad you pointed to the latest version in there. Will figure out what caused the regression and will fix promptly. Thanks for letting me know!
Okay I think I see what's happening. I implemented more aggressive garbage collection with the latest release and when it fires after a rendering has completed, this takes out the mutation observer. I think I should only fire the garbage collection when mount points are not actively in the DOM.
Doesn't seem to be it... will keep poking at the changeset and see what sticks out.
What's weird is that the mutation observer is not producing any records after the second button is pressed. I grabbed a reference to the observer, pressed the second button, and nothing... I wonder if there was a browser change, however unlikely that would be.
Opened up a potential fix here: #309, still needs unit/int tests, but wanted to get some eyes on this first.
@nbianca yup that was the problem. I found in my original patch for this issue, I had set isDirty to true in the mutationobserver callback. That was then removed when I added better support for component top level rendering.
To fix this, I'm simply purging the mutationobserver records after every render since we only care about mutations when we aren't rendering (due to its synchronous nature).
Once my PR passes, I will merge and then work on the next release and let you know.