Fix ignored setState in Safari when iframe is touched (#24459)

Author: gaearon

packages/react-devtools-shared/src/__tests__/utils-test.js

@@ -189,5 +189,26 @@ describe('utils', () => {
       ]);
       expect(formatWithStyles(['%%c%c'], 'color: gray')).toEqual(['%%c%c']);
     });
+
+    it('should format non string inputs as the first argument', () => {
+      expect(formatWithStyles([{foo: 'bar'}])).toEqual([{foo: 'bar'}]);
+      expect(formatWithStyles([[1, 2, 3]])).toEqual([[1, 2, 3]]);
+      expect(formatWithStyles([{foo: 'bar'}], 'color: gray')).toEqual([
+        '%c%o',
+        'color: gray',
+        {foo: 'bar'},
+      ]);
+      expect(formatWithStyles([[1, 2, 3]], 'color: gray')).toEqual([
+        '%c%o',
+        'color: gray',
+        [1, 2, 3],
+      ]);
+      expect(formatWithStyles([{foo: 'bar'}, 'hi'], 'color: gray')).toEqual([
+        '%c%o %s',
+        'color: gray',
+        {foo: 'bar'},
+        'hi',
+      ]);
+    });
   });
 });

packages/react-devtools-shared/src/backend/utils.js

@@ -181,17 +181,16 @@ export function formatWithStyles(
     inputArgs === undefined ||
     inputArgs === null ||
     inputArgs.length === 0 ||
-    typeof inputArgs[0] !== 'string' ||
     // Matches any of %c but not %%c
-    inputArgs[0].match(/([^%]|^)(%c)/g) ||
+    (typeof inputArgs[0] === 'string' && inputArgs[0].match(/([^%]|^)(%c)/g)) ||
     style === undefined
   ) {
     return inputArgs;
   }
 
   // Matches any of %(o|O|d|i|s|f), but not %%(o|O|d|i|s|f)
   const REGEXP = /([^%]|^)((%%)*)(%([oOdisf]))/g;
-  if (inputArgs[0].match(REGEXP)) {
+  if (typeof inputArgs[0] === 'string' && inputArgs[0].match(REGEXP)) {
     return [`%c${inputArgs[0]}`, style, ...inputArgs.slice(1)];
   } else {
     const firstArg = inputArgs.reduce((formatStr, elem, i) => {

packages/react-devtools-shared/src/hook.js

@@ -180,17 +180,17 @@ export function installHook(target: any): DevToolsHook | null {
       inputArgs === undefined ||
       inputArgs === null ||
       inputArgs.length === 0 ||
-      typeof inputArgs[0] !== 'string' ||
       // Matches any of %c but not %%c
-      inputArgs[0].match(/([^%]|^)(%c)/g) ||
+      (typeof inputArgs[0] === 'string' &&
+        inputArgs[0].match(/([^%]|^)(%c)/g)) ||
       style === undefined
     ) {
       return inputArgs;
     }
 
     // Matches any of %(o|O|d|i|s|f), but not %%(o|O|d|i|s|f)
     const REGEXP = /([^%]|^)((%%)*)(%([oOdisf]))/g;
-    if (inputArgs[0].match(REGEXP)) {
+    if (typeof inputArgs[0] === 'string' && inputArgs[0].match(REGEXP)) {
       return [`%c${inputArgs[0]}`, style, ...inputArgs.slice(1)];
     } else {
       const firstArg = inputArgs.reduce((formatStr, elem, i) => {

packages/react-dom/src/__tests__/ReactDOMSafariMicrotaskBug-test.js

@@ -16,7 +16,7 @@ let act;
 
 describe('ReactDOMSafariMicrotaskBug-test', () => {
   let container;
-  let simulateSafariBug;
+  let flushMicrotasksPrematurely;
 
   beforeEach(() => {
     // In Safari, microtasks don't always run on clean stack.
@@ -27,9 +27,12 @@ describe('ReactDOMSafariMicrotaskBug-test', () => {
     window.queueMicrotask = function(cb) {
       queue.push(cb);
     };
-    simulateSafariBug = function() {
-      queue.forEach(cb => cb());
-      queue = [];
+    flushMicrotasksPrematurely = function() {
+      while (queue.length > 0) {
+        const prevQueue = queue;
+        queue = [];
+        prevQueue.forEach(cb => cb());
+      }
     };
 
     jest.resetModules();
@@ -45,7 +48,7 @@ describe('ReactDOMSafariMicrotaskBug-test', () => {
     document.body.removeChild(container);
   });
 
-  it('should be resilient to buggy queueMicrotask', async () => {
+  it('should deal with premature microtask in commit phase', async () => {
     let ran = false;
     function Foo() {
       const [state, setState] = React.useState(0);
@@ -55,7 +58,7 @@ describe('ReactDOMSafariMicrotaskBug-test', () => {
             if (!ran) {
               ran = true;
               setState(1);
-              simulateSafariBug();
+              flushMicrotasksPrematurely();
             }
           }}>
           {state}
@@ -68,4 +71,30 @@ describe('ReactDOMSafariMicrotaskBug-test', () => {
     });
     expect(container.textContent).toBe('1');
   });
+
+  it('should deal with premature microtask in event handler', async () => {
+    function Foo() {
+      const [state, setState] = React.useState(0);
+      return (
+        <button
+          onClick={() => {
+            setState(1);
+            flushMicrotasksPrematurely();
+          }}>
+          {state}
+        </button>
+      );
+    }
+    const root = ReactDOMClient.createRoot(container);
+    await act(async () => {
+      root.render(<Foo />);
+    });
+    expect(container.textContent).toBe('0');
+    await act(async () => {
+      container.firstChild.dispatchEvent(
+        new MouseEvent('click', {bubbles: true}),
+      );
+    });
+    expect(container.textContent).toBe('1');
+  });
 });

packages/react-reconciler/src/ReactFiberWorkLoop.new.js

@@ -835,9 +835,12 @@ function ensureRootIsScheduled(root: FiberRoot, currentTime: number) {
           // https://github.com/facebook/react/issues/22459
           // We don't support running callbacks in the middle of render
           // or commit so we need to check against that.
-          if (executionContext === NoContext) {
-            // It's only safe to do this conditionally because we always
-            // check for pending work before we exit the task.
+          if (
+            (executionContext & (RenderContext | CommitContext)) ===
+            NoContext
+          ) {
+            // Note that this would still prematurely flush the callbacks
+            // if this happens outside render or commit phase (e.g. in an event).
             flushSyncCallbacks();
           }
         });

packages/react-reconciler/src/ReactFiberWorkLoop.old.js

@@ -835,9 +835,12 @@ function ensureRootIsScheduled(root: FiberRoot, currentTime: number) {
           // https://github.com/facebook/react/issues/22459
           // We don't support running callbacks in the middle of render
           // or commit so we need to check against that.
-          if (executionContext === NoContext) {
-            // It's only safe to do this conditionally because we always
-            // check for pending work before we exit the task.
+          if (
+            (executionContext & (RenderContext | CommitContext)) ===
+            NoContext
+          ) {
+            // Note that this would still prematurely flush the callbacks
+            // if this happens outside render or commit phase (e.g. in an event).
             flushSyncCallbacks();
           }
         });