[Fizz] External runtime: fix bug in processing existing elements (#26303

) ## Summary Fix bug in how the Fizz external runtime processes existing template elements. Bug: - getElementsByTagName returns a HTMLCollection, which is live. - while iterating over an HTMLCollection, we call handleNode which removes nodes Fix: - Call Array.from to copy children of `document.body` before processing. - We could use `querySelectorAll` instead, but that is likely slower due to reading more nodes. ## How did you test this change? Did ad-hoc testing on Facebook home page by commenting out the mutation observer and adding the following. ```javascript window.addEventListener('DOMContentLoaded', function () { handleExistingNodes(document.body); }); ```
facebook · Mar 4, 2023 · 03462cf · 03462cf
1 parent faacefb
commit 03462cf
Showing 1 changed file with 29 additions and 27 deletions.
diff --git a/packages/react-dom-bindings/src/server/ReactDOMServerExternalRuntime.js b/packages/react-dom-bindings/src/server/ReactDOMServerExternalRuntime.js
@@ -21,35 +21,37 @@ if (!window.$RC) {
   window.$RM = new Map();
 }
 
-if (document.readyState === 'loading') {
-  if (document.body != null) {
+if (document.body != null) {
+  if (document.readyState === 'loading') {
     installFizzInstrObserver(document.body);
-  } else {
-    // body may not exist yet if the fizz runtime is sent in <head>
-    // (e.g. as a preinit resource)
-    // $FlowFixMe[recursive-definition]
-    const domBodyObserver = new MutationObserver(() => {
-      // We expect the body node to be stable once parsed / created
-      if (document.body) {
-        if (document.readyState === 'loading') {
-          installFizzInstrObserver(document.body);
-        }
-        handleExistingNodes();
-        // We can call disconnect without takeRecord here,
-        // since we only expect a single document.body
-        domBodyObserver.disconnect();
-      }
-    });
-    // documentElement must already exist at this point
-    // $FlowFixMe[incompatible-call]
-    domBodyObserver.observe(document.documentElement, {childList: true});
   }
-}
+  // $FlowFixMe[incompatible-cast]
+  handleExistingNodes((document.body /*: HTMLElement */));
+} else {
+  // Document must be loading -- body may not exist yet if the fizz external
+  // runtime is sent in <head> (e.g. as a preinit resource)
+  // $FlowFixMe[recursive-definition]
+  const domBodyObserver = new MutationObserver(() => {
+    // We expect the body node to be stable once parsed / created
+    if (document.body != null) {
+      if (document.readyState === 'loading') {
+        installFizzInstrObserver(document.body);
+      }
+      // $FlowFixMe[incompatible-cast]
+      handleExistingNodes((document.body /*: HTMLElement */));
 
-handleExistingNodes();
+      // We can call disconnect without takeRecord here,
+      // since we only expect a single document.body
+      domBodyObserver.disconnect();
+    }
+  });
+  // documentElement must already exist at this point
+  // $FlowFixMe[incompatible-call]
+  domBodyObserver.observe(document.documentElement, {childList: true});
+}
 
-function handleExistingNodes() {
-  const existingNodes = document.getElementsByTagName('template');
+function handleExistingNodes(target /*: HTMLElement */) {
+  const existingNodes = target.querySelectorAll('template');
   for (let i = 0; i < existingNodes.length; i++) {
     handleNode(existingNodes[i]);
   }
@@ -60,8 +62,8 @@ function installFizzInstrObserver(target /*: Node */) {
     for (let i = 0; i < mutations.length; i++) {
       const addedNodes = mutations[i].addedNodes;
       for (let j = 0; j < addedNodes.length; j++) {
-        if (addedNodes.item(j).parentNode) {
-          handleNode(addedNodes.item(j));
+        if (addedNodes[j].parentNode) {
+          handleNode(addedNodes[j]);
         }
       }
     }