Implement mutation observer in DOMChildrenOperations

[j0lvera]

This pull request (WIP) tries to resolve #3218

I have done some basic implementation and tried to add a devtool hook, but have some questions:

• What's the goal of the hooks in ReactDebugTool.js, how can I trigger an existing hook to see how it works?
• I'm not sure I'd add tests to this implementation, any suggestion is welcome
• Also, I'd like suggestion on where to add the .disconnect(); function

[j0lvera]

Working on the tests

[edvinerikson]

guess this will break if there is multiple react trees on the page

[j0lvera]

yeah, i should look for all react trees and run the observer on each of them; will fix.

[j0lvera]

hey @edvinerikson, when you have a chance could you take a look on the questions above?

[LeZuse]

Are you still working on this @thinkxl? I think it would be helpful to write some tests first and get them approved from the React team.

[syranide]

Modifying nodes inside empty React leaf DOM-nodes is allowed and a feature. This PR does not consider this and warns regardless. Preferably it should also not rely on querySelector but rather put hooks in the correct internal React methods which knows when React roots are mounted and unmounted. As mentioned below, unless there's some deduping, modifications inside nested React roots will emit multiple warnings.

More info #3218 (comment)

[j0lvera]

@syranide thanks for the feedback. I will do the changes to meet the requirements.

[j0lvera]

Modifying nodes inside empty React leaf DOM-nodes

You mean when a React component inside a container doesn't have children?

<div id="app">
  <div data-reactroot></div> <!-- empty react root node -->
</div> <!-- #app -->

Or when the tree of React nodes are empty like <p></p> or <p /> and someone wants to mutate that?

<div id="app">
  <div data-reactroot>
    <div class="headerr">
      <img src="logo.svg" alt="logo">
    </div> <!-- non-empty node -->
    <p></p> <!-- empty node, user may want to mutate this externally -->
    <div>foo</div> <!-- non-empty node -->
  </div>
</div> <!-- #app -->

[syranide]

@thinkxl Yeah, anything inside your "non-empty nodes" is free game and mutations should not emit warnings.

@spicyj I'm curious, when it comes to MutationObservers, how do you feel about listeners per React root vs a single global for the document (or per document in the case of iframes)? Having it per React root is a tighter fit but nested React roots will multiply the overhead and nested React roots updating will most likely cause significant overhead due to one or more parent MutationObservers unless you explicitly disconnect parent MutationsObservers but that can also get somewhat expensive. So IMHO a global MutationObserver seems like the simplest and best of these two approaches, but it obviously comes with the downside of being global.

The best option would naturally be to have it per React root but not if it's nested, but if you move a nested React root it might not have a MutationObserver covering it anymore (acceptable?).

[Hypnosphi]

Looks like some flag is needed on empty leafs which would somehow (using Element.closest() maybe?) block warnings for all the potential subtree. Given that, the same flag could be used for nested react trees, which would remove warnings from parent tree, leaving only those from the children.

Another option can be not to use deep observation with {subtree: true}, but to attach plain shallow observer to each dom node which needs it.

[j0lvera]

Another option can be not to use deep observation with {subtree: true}, but to attach plain shallow observer to each dom node which needs it.

That might be the way to go, having an observer per node, but, not sure about the impact on performance.

[gaearon]

As per above comments, this is too strict and will warn for valid cases. I think we might not want to do this after all, as I don't see a way that would be both fast and loose enough. Thanks for trying though!