Fixed rare issue with components initialization

[a-ignatov-parc]

Found today problem with rendering simple DOM tree in browsers that doesn't support custom-elements natively (all except Google Chrome).

DOM tree example

<component1>
    <section class="some-class"></section>
    <section class="some-class2">
        <component2 id="id1">
            <content>
                Some content
            </content>
        </component2>
        <span></span>
        <component2 id="id2">
            <content>
                Some content
            </content>
        </component2>
        <span></span>
        <component2 id="id3">
            <content>
                Some content
            </content>
        </component2>
    </section>
</component1>

Last <component2> node that was not initialized after appending to document. Problem was with MutationObserver shim that was not firing event on appending child nodes to section.some-class2.

I was trying to recreate issue in tests with simple DOM manipulations but with no luck. MutationObserver shim was working properly.

So here is a list of circumstances that could lead to this problem:

1. <component2> on attached callback moves some child nodes to component2.nextSibling. This behaviour is similar to <label> element that move inappropriate elements away.
2. Around 100+ components and subcomponents are rendering on the page at the same time.
3. Components' content renders with virtual-dom lib.
4. Changes to document are made in batches with using fastdom lib.

P.S. I will continue my attempts to reproduce this problem in tests. But for now i wasn't be able to do this

---

I guess my problem is related with this line of code that can lead to increasing elements length. Need to investigate... maybe there should be another fix...

---

If i am right the problem is in this line of code. Because MutationObserver callback is synchronously fired before cleaning lastBatchedRecord this could lead to adding other nodes exactly like in case i faced today. Native MutationObserver callbacks are fired at the "end of micro-task" (there is a good explanation in this article) and i am afraid can't be achieved properly with setTimeout. Only promises behave same way but that is not an option for browsers that does not have support for them.

Will make another fix tomorrow.

[treshugart]

Yep, fully aware of microtasks etc. The only thing I could recommend trying is removing batching and setTimeout, but that'd have adverse affects on peformance most probably because batching helps to eliminate unnecessary ops iirc.

[treshugart]

My only gripe here is that it's cheaper in IE9 to save elements.length as elementsLen in super massive doms.

[treshugart]

But I guess we can't do that :(. I wonder what the performance would be like now.

[a-ignatov-parc]

Is super massive DOM trees are real world scenario or just synthetic test case?

[treshugart]

Do you think we'll need to do anything in master to fix this? I'm not really sure how to reproduce it so I can't really help you there. Also, the mutation observer polyfill has been split from the core in master. It's now over at https://github.com/skatejs/polyfill-mutation-observer.

[treshugart]

Couple more questions:

1. Does this happen if you remove the virtual-dom and fastdom layers?
2. It's perfectly acceptable to use those and this should be fixed for that reason, but it sounded like you wanted to land another fix? Do you want me to hold off on merging this until then?
3. Since you said "will make another fix tomorrow", does that mean that this doesn't fix the issue?

I'm not overly concerned about it, but it'd be interesting to see if this affects IE performance.

[treshugart]

I think what you're experiencing in not being able to reproduce it might be because of fastdom. I'm not certain, but in order to batch dom mutations it'd have to override internal methods. It's probably delaying those which would mean you're right int that it becomes out of sync. You might be able to reproduce it by delaying an appendChild.

[a-ignatov-parc]

Good morning. Yes i want to put this PR on hold until i finish my investigations. I hope i will complete them in 2-3 hours

[a-ignatov-parc]

I think the problem was not with fastdom but with rendering pipeline. I'll try to explain:

1. DOM tree in my example renders
2. Subcomponentes are created and they moved child nodes to siblings.
3. MutationObserver fires handler and shim starts to collect changes.
4. Because eTargetParent is similar to lastBatchedRecord.target (this) shim appends new nodes to already sent childNodes collection

I should notice that this problem take place because there were no other changes in DOM tree

[a-ignatov-parc]

Since you said "will make another fix tomorrow", does that mean that this doesn't fix the issue?

This PR fix my issue but it is more hack then a proper fix. That is why I want to look into MutationObserver shim

[a-ignatov-parc]

Well... the problem is not with MutationObserver but with traversing through childNodes here. childNodes is live collection and i see only two way to handle this:

1. Freeze childNodes
2. Don't cache length

Second way is more performant then freezing.

[a-ignatov-parc]

Updated comment and i think this fix is the best from performance point of view. But always open to discuss other ways to fix this problem.

[treshugart]

👍 let me know when you're ok for this to be merged. Are you able to check to see if this needs fixing on master? I'm happy if you just spot the part in master that needs changing rather than trying to integrate master to where you're reproducing the issue.

[a-ignatov-parc]

Our QA team is still testing this fix. I'll post results here when they are done. Later will check master branch if it is need to be fixed ;)

[treshugart]

Let me know when I can merge this :)

[a-ignatov-parc]

Our QA team gives green light to this fix. You can merge it ;)

[treshugart]

Created issues for master and the mutation observer polyfill repo.

[a-ignatov-parc]

Checking master for possible changes right now. Going to create PR soon