From 0220f9238b54e14036309586a9caac4f2e7be614 Mon Sep 17 00:00:00 2001 From: dan Date: Thu, 12 May 2022 17:58:18 +0100 Subject: [PATCH] Fix ignored setState in Safari when iframe is touched (#24459) --- .../src/__tests__/utils-test.js | 21 ++++++++++ .../src/backend/utils.js | 5 +-- packages/react-devtools-shared/src/hook.js | 6 +-- .../ReactDOMSafariMicrotaskBug-test.js | 41 ++++++++++++++++--- .../src/ReactFiberWorkLoop.new.js | 9 ++-- .../src/ReactFiberWorkLoop.old.js | 9 ++-- 6 files changed, 73 insertions(+), 18 deletions(-) diff --git a/packages/react-devtools-shared/src/__tests__/utils-test.js b/packages/react-devtools-shared/src/__tests__/utils-test.js index 02bc6be4657a7..ed89202958375 100644 --- a/packages/react-devtools-shared/src/__tests__/utils-test.js +++ b/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', + ]); + }); }); }); diff --git a/packages/react-devtools-shared/src/backend/utils.js b/packages/react-devtools-shared/src/backend/utils.js index 169623a1633a9..5c4b1289855b2 100644 --- a/packages/react-devtools-shared/src/backend/utils.js +++ b/packages/react-devtools-shared/src/backend/utils.js @@ -181,9 +181,8 @@ 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; @@ -191,7 +190,7 @@ export function formatWithStyles( // 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) => { diff --git a/packages/react-devtools-shared/src/hook.js b/packages/react-devtools-shared/src/hook.js index d41ea4559159a..8adbb7534a254 100644 --- a/packages/react-devtools-shared/src/hook.js +++ b/packages/react-devtools-shared/src/hook.js @@ -180,9 +180,9 @@ 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; @@ -190,7 +190,7 @@ export function installHook(target: any): DevToolsHook | null { // 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) => { diff --git a/packages/react-dom/src/__tests__/ReactDOMSafariMicrotaskBug-test.js b/packages/react-dom/src/__tests__/ReactDOMSafariMicrotaskBug-test.js index 0ba0848d8e3cd..ef820ac77c6c3 100644 --- a/packages/react-dom/src/__tests__/ReactDOMSafariMicrotaskBug-test.js +++ b/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 ( + + ); + } + const root = ReactDOMClient.createRoot(container); + await act(async () => { + root.render(); + }); + expect(container.textContent).toBe('0'); + await act(async () => { + container.firstChild.dispatchEvent( + new MouseEvent('click', {bubbles: true}), + ); + }); + expect(container.textContent).toBe('1'); + }); }); diff --git a/packages/react-reconciler/src/ReactFiberWorkLoop.new.js b/packages/react-reconciler/src/ReactFiberWorkLoop.new.js index b4343485259ae..6e438aaa7bb60 100644 --- a/packages/react-reconciler/src/ReactFiberWorkLoop.new.js +++ b/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(); } }); diff --git a/packages/react-reconciler/src/ReactFiberWorkLoop.old.js b/packages/react-reconciler/src/ReactFiberWorkLoop.old.js index 58757a37367ce..3e02a4fe37697 100644 --- a/packages/react-reconciler/src/ReactFiberWorkLoop.old.js +++ b/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(); } });