Fast Refresh: Don't block DevTools commit hook (#20129)

In some scenarios (either timing dependent, or pre-FR compatible React versions) FR blocked calling the React DevTools commit hook. This PR adds a test and a fix for that.

Author: Brian Vaughn

packages/react-refresh/package.json

@@ -25,5 +25,9 @@
   },
   "engines": {
     "node": ">=0.10.0"
+  },
+  "devDependencies": {
+    "react-16-8": "npm:react@16.8.0",
+    "react-dom-16-8": "npm:react-dom@16.8.0"
   }
-}
+}

packages/react-refresh/src/ReactFreshRuntime.js

@@ -517,53 +517,53 @@ export function injectIntoGlobalHook(globalObject: any): void {
       didError: boolean,
     ) {
       const helpers = helpersByRendererID.get(id);
-      if (helpers === undefined) {
-        return;
-      }
-      helpersByRoot.set(root, helpers);
-
-      const current = root.current;
-      const alternate = current.alternate;
-
-      // We need to determine whether this root has just (un)mounted.
-      // This logic is copy-pasted from similar logic in the DevTools backend.
-      // If this breaks with some refactoring, you'll want to update DevTools too.
-
-      if (alternate !== null) {
-        const wasMounted =
-          alternate.memoizedState != null &&
-          alternate.memoizedState.element != null;
-        const isMounted =
-          current.memoizedState != null &&
-          current.memoizedState.element != null;
-
-        if (!wasMounted && isMounted) {
+      if (helpers !== undefined) {
+        helpersByRoot.set(root, helpers);
+
+        const current = root.current;
+        const alternate = current.alternate;
+
+        // We need to determine whether this root has just (un)mounted.
+        // This logic is copy-pasted from similar logic in the DevTools backend.
+        // If this breaks with some refactoring, you'll want to update DevTools too.
+
+        if (alternate !== null) {
+          const wasMounted =
+            alternate.memoizedState != null &&
+            alternate.memoizedState.element != null;
+          const isMounted =
+            current.memoizedState != null &&
+            current.memoizedState.element != null;
+
+          if (!wasMounted && isMounted) {
+            // Mount a new root.
+            mountedRoots.add(root);
+            failedRoots.delete(root);
+          } else if (wasMounted && isMounted) {
+            // Update an existing root.
+            // This doesn't affect our mounted root Set.
+          } else if (wasMounted && !isMounted) {
+            // Unmount an existing root.
+            mountedRoots.delete(root);
+            if (didError) {
+              // We'll remount it on future edits.
+              failedRoots.add(root);
+            } else {
+              helpersByRoot.delete(root);
+            }
+          } else if (!wasMounted && !isMounted) {
+            if (didError) {
+              // We'll remount it on future edits.
+              failedRoots.add(root);
+            }
+          }
+        } else {
           // Mount a new root.
           mountedRoots.add(root);
-          failedRoots.delete(root);
-        } else if (wasMounted && isMounted) {
-          // Update an existing root.
-          // This doesn't affect our mounted root Set.
-        } else if (wasMounted && !isMounted) {
-          // Unmount an existing root.
-          mountedRoots.delete(root);
-          if (didError) {
-            // We'll remount it on future edits.
-            failedRoots.add(root);
-          } else {
-            helpersByRoot.delete(root);
-          }
-        } else if (!wasMounted && !isMounted) {
-          if (didError) {
-            // We'll remount it on future edits.
-            failedRoots.add(root);
-          }
         }
-      } else {
-        // Mount a new root.
-        mountedRoots.add(root);
       }
 
+      // Always call the decorated DevTools hook.
       return oldOnCommitFiberRoot.apply(this, arguments);
     };
   } else {

packages/react-refresh/src/__tests__/ReactFresh-test.js

@@ -3745,24 +3745,32 @@ describe('ReactFresh', () => {
     }
   });
 
-  // This simulates the scenario in https://github.com/facebook/react/issues/17626.
+  function initFauxDevToolsHook() {
+    const onCommitFiberRoot = jest.fn();
+    const onCommitFiberUnmount = jest.fn();
+
+    let idCounter = 0;
+    const renderers = new Map();
+
+    // This is a minimal shim for the global hook installed by DevTools.
+    // The real one is in packages/react-devtools-shared/src/hook.js.
+    global.__REACT_DEVTOOLS_GLOBAL_HOOK__ = {
+      renderers,
+      supportsFiber: true,
+      inject(renderer) {
+        const id = ++idCounter;
+        renderers.set(id, renderer);
+        return id;
+      },
+      onCommitFiberRoot,
+      onCommitFiberUnmount,
+    };
+  }
+
+  // This simulates the scenario in https://github.com/facebook/react/issues/17626
   it('can inject the runtime after the renderer executes', () => {
     if (__DEV__) {
-      // This is a minimal shim for the global hook installed by DevTools.
-      // The real one is in packages/react-devtools-shared/src/hook.js.
-      let idCounter = 0;
-      const renderers = new Map();
-      global.__REACT_DEVTOOLS_GLOBAL_HOOK__ = {
-        renderers,
-        supportsFiber: true,
-        inject(renderer) {
-          const id = ++idCounter;
-          renderers.set(id, renderer);
-          return id;
-        },
-        onCommitFiberRoot() {},
-        onCommitFiberUnmount() {},
-      };
+      initFauxDevToolsHook();
 
       // Load these first, as if they're coming from a CDN.
       jest.resetModules();
@@ -3820,4 +3828,39 @@ describe('ReactFresh', () => {
       expect(el.style.color).toBe('red');
     }
   });
+
+  // This simulates the scenario in https://github.com/facebook/react/issues/20100
+  it('does not block DevTools when an unsupported renderer is injected', () => {
+    if (__DEV__) {
+      initFauxDevToolsHook();
+
+      const onCommitFiberRoot =
+        global.__REACT_DEVTOOLS_GLOBAL_HOOK__.onCommitFiberRoot;
+
+      // Redirect all React/ReactDOM requires to v16.8.0
+      // This version predates Fast Refresh support.
+      jest.mock('react', () => jest.requireActual('react-16-8'));
+      jest.mock('react-dom', () => jest.requireActual('react-dom-16-8'));
+
+      // Load React and company.
+      jest.resetModules();
+      React = require('react');
+      ReactDOM = require('react-dom');
+      Scheduler = require('scheduler');
+
+      // Important! Inject into the global hook *after* ReactDOM runs:
+      ReactFreshRuntime = require('react-refresh/runtime');
+      ReactFreshRuntime.injectIntoGlobalHook(global);
+
+      render(() => {
+        function Hello() {
+          return <div>Hi!</div>;
+        }
+        $RefreshReg$(Hello, 'Hello');
+        return Hello;
+      });
+
+      expect(onCommitFiberRoot).toHaveBeenCalled();
+    }
+  });
 });

yarn.lock

@@ -11499,6 +11499,16 @@ rc@^1.0.1, rc@^1.1.6, rc@^1.2.8:
     object-assign "^4.1.0"
     prop-types "^15.5.10"
 
+"react-16-8@npm:react@16.8.0":
+  version "16.8.0"
+  resolved "https://registry.yarnpkg.com/react/-/react-16.8.0.tgz#8533f0e4af818f448a276eae71681d09e8dd970a"
+  integrity sha512-g+nikW2D48kqgWSPwNo0NH9tIGG3DsQFlrtrQ1kj6W77z5ahyIHG0w8kPpz4Sdj6gyLnz0lEd/xsjOoGge2MYQ==
+  dependencies:
+    loose-envify "^1.1.0"
+    object-assign "^4.1.1"
+    prop-types "^15.6.2"
+    scheduler "^0.13.0"
+
 "react-dom-15@npm:react-dom@^15":
   version "15.6.2"
   resolved "https://registry.yarnpkg.com/react-dom/-/react-dom-15.6.2.tgz#41cfadf693b757faf2708443a1d1fd5a02bef730"
@@ -11509,6 +11519,16 @@ rc@^1.0.1, rc@^1.1.6, rc@^1.2.8:
     object-assign "^4.1.0"
     prop-types "^15.5.10"
 
+"react-dom-16-8@npm:react-dom@16.8.0":
+  version "16.8.0"
+  resolved "https://registry.yarnpkg.com/react-dom/-/react-dom-16.8.0.tgz#18f28d4be3571ed206672a267c66dd083145a9c4"
+  integrity sha512-dBzoAGYZpW9Yggp+CzBPC7q1HmWSeRc93DWrwbskmG1eHJWznZB/p0l/Sm+69leIGUS91AXPB/qB3WcPnKx8Sw==
+  dependencies:
+    loose-envify "^1.1.0"
+    object-assign "^4.1.1"
+    prop-types "^15.6.2"
+    scheduler "^0.13.0"
+
 react-is@^16.12.0, react-is@^16.8.1:
   version "16.13.1"
   resolved "https://registry.yarnpkg.com/react-is/-/react-is-16.13.1.tgz#789729a4dc36de2999dc156dd6c1d9c18cea56a4"
@@ -12315,6 +12335,14 @@ scheduler@^0.11.0:
     loose-envify "^1.1.0"
     object-assign "^4.1.1"
 
+scheduler@^0.13.0:
+  version "0.13.6"
+  resolved "https://registry.yarnpkg.com/scheduler/-/scheduler-0.13.6.tgz#466a4ec332467b31a91b9bf74e5347072e4cd889"
+  integrity sha512-IWnObHt413ucAYKsD9J1QShUKkbKLQQHdxRyw73sw4FN26iWr3DY/H34xGPe4nmL1DwXyWmSWmMrA9TfQbE/XQ==
+  dependencies:
+    loose-envify "^1.1.0"
+    object-assign "^4.1.1"
+
 schema-utils@^1.0.0:
   version "1.0.0"
   resolved "https://registry.yarnpkg.com/schema-utils/-/schema-utils-1.0.0.tgz#0b79a93204d7b600d4b2850d1f66c2a34951c770"