From cd59515da1a0493916febfbec2504e452151740c Mon Sep 17 00:00:00 2001 From: eps1lon Date: Sun, 21 Apr 2024 17:26:52 +0200 Subject: [PATCH] Don't retry synchronous render that doesn't go through Scheduler --- .../__tests__/ReactInternalTestUtils-test.js | 6 +- .../src/ReactFiberWorkLoop.js | 10 +-- ...tIncrementalErrorHandling-test.internal.js | 62 ++++++------------- .../ReactIncrementalErrorLogging-test.js | 11 +--- .../ReactCoffeeScriptClass-test.coffee | 15 +++-- .../react/src/__tests__/ReactES6Class-test.js | 23 ++++--- .../__tests__/ReactTypeScriptClass-test.ts | 21 ++++--- .../useSyncExternalStoreShared-test.js | 5 +- 8 files changed, 70 insertions(+), 83 deletions(-) diff --git a/packages/internal-test-utils/__tests__/ReactInternalTestUtils-test.js b/packages/internal-test-utils/__tests__/ReactInternalTestUtils-test.js index 1d5f5226e7d5b..1cdc52072699c 100644 --- a/packages/internal-test-utils/__tests__/ReactInternalTestUtils-test.js +++ b/packages/internal-test-utils/__tests__/ReactInternalTestUtils-test.js @@ -752,7 +752,7 @@ describe('ReactInternalTestUtils console assertions', () => { You must call one of the assertConsoleDev helpers between each act call." `); - await waitForAll(['A', 'B', 'A', 'B']); + await waitForAll(['A', 'B']); }); test('should fail if waitForPaint is called before asserting', async () => { @@ -1684,7 +1684,7 @@ describe('ReactInternalTestUtils console assertions', () => { You must call one of the assertConsoleDev helpers between each act call." `); - await waitForAll(['A', 'B', 'A', 'B']); + await waitForAll(['A', 'B']); }); test('should fail if waitForPaint is called before asserting', async () => { @@ -2634,7 +2634,7 @@ describe('ReactInternalTestUtils console assertions', () => { You must call one of the assertConsoleDev helpers between each act call." `); - await waitForAll(['A', 'B', 'A', 'B']); + await waitForAll(['A', 'B']); }); test('should fail if waitForPaint is called before asserting', async () => { diff --git a/packages/react-reconciler/src/ReactFiberWorkLoop.js b/packages/react-reconciler/src/ReactFiberWorkLoop.js index 2592807cde69a..206dfd8a95985 100644 --- a/packages/react-reconciler/src/ReactFiberWorkLoop.js +++ b/packages/react-reconciler/src/ReactFiberWorkLoop.js @@ -1364,14 +1364,14 @@ export function performSyncWorkOnRoot(root: FiberRoot, lanes: Lanes): null { } let exitStatus = renderRootSync(root, lanes); + + const wasRootDehydrated = supportsHydration && isRootDehydrated(root); if ( (disableLegacyMode || root.tag !== LegacyRoot) && - exitStatus === RootErrored + exitStatus === RootErrored && + wasRootDehydrated ) { - // If something threw an error, try rendering one more time. We'll render - // synchronously to block concurrent data mutations, and we'll includes - // all pending updates are included. If it still fails after the second - // attempt, we'll give up and commit the resulting tree. + // If something threw an error and we have work to do, try rendering one more time. const originallyAttemptedLanes = lanes; const errorRetryLanes = getLanesToRetrySynchronouslyOnError( root, diff --git a/packages/react-reconciler/src/__tests__/ReactIncrementalErrorHandling-test.internal.js b/packages/react-reconciler/src/__tests__/ReactIncrementalErrorHandling-test.internal.js index 834bbeb0e7d02..3f36396adaf1a 100644 --- a/packages/react-reconciler/src/__tests__/ReactIncrementalErrorHandling-test.internal.js +++ b/packages/react-reconciler/src/__tests__/ReactIncrementalErrorHandling-test.internal.js @@ -410,7 +410,7 @@ describe('ReactIncrementalErrorHandling', () => { ReactNoop.render(, () => Scheduler.log('commit')); }); - assertLog(['Parent', 'BadRender', 'Parent', 'BadRender', 'commit']); + assertLog(['Parent', 'BadRender', 'commit']); expect(ReactNoop).toMatchRenderedOutput(null); }); @@ -666,12 +666,6 @@ describe('ReactIncrementalErrorHandling', () => { assertLog([ 'ErrorBoundary render success', 'BrokenRender', - - // React retries one more time - 'ErrorBoundary render success', - 'BrokenRender', - - // Errored again on retry. Now handle it. 'ErrorBoundary componentDidCatch', 'ErrorBoundary render error', ]); @@ -716,12 +710,6 @@ describe('ReactIncrementalErrorHandling', () => { assertLog([ 'ErrorBoundary render success', 'BrokenRender', - - // React retries one more time - 'ErrorBoundary render success', - 'BrokenRender', - - // Errored again on retry. Now handle it. 'ErrorBoundary componentDidCatch', 'ErrorBoundary render error', ]); @@ -840,12 +828,6 @@ describe('ReactIncrementalErrorHandling', () => { assertLog([ 'RethrowErrorBoundary render', 'BrokenRender', - - // React retries one more time - 'RethrowErrorBoundary render', - 'BrokenRender', - - // Errored again on retry. Now handle it. 'RethrowErrorBoundary componentDidCatch', ]); expect(ReactNoop).toMatchRenderedOutput(null); @@ -882,12 +864,6 @@ describe('ReactIncrementalErrorHandling', () => { assertLog([ 'RethrowErrorBoundary render', 'BrokenRender', - - // React retries one more time - 'RethrowErrorBoundary render', - 'BrokenRender', - - // Errored again on retry. Now handle it. 'RethrowErrorBoundary componentDidCatch', ]); expect(ReactNoop).toMatchRenderedOutput(null); @@ -1892,27 +1868,25 @@ describe('ReactIncrementalErrorHandling', () => { root.render(); }); - await act(async () => { - // Schedule a default pri and a low pri update on the root. - root.render(); - React.startTransition(() => { - root.render(); - }); + await expect( + act(async () => { + // Schedule a default pri and a low pri update on the root. + root.render(); + React.startTransition(() => { + root.render(); + }); - // Render through just the default pri update. The low pri update remains on - // the queue. - await waitFor(['Everything is fine.']); + // Render through the default pri and low pri update. + await waitFor(['Everything is fine.']); - // Schedule a discrete update on a child that triggers an error. - // The root should capture this error. But since there's still a pending - // update on the root, the error should be suppressed. - ReactNoop.discreteUpdates(() => { - setShouldThrow(true); - }); - }); - // Should render the final state without throwing the error. - assertLog(['Everything is fine.']); - expect(root).toMatchRenderedOutput('Everything is fine.'); + // Schedule a discrete update on a child that triggers an error. + ReactNoop.discreteUpdates(() => { + setShouldThrow(true); + }); + }), + ).rejects.toThrow('Oops'); + assertLog([]); + expect(root).toMatchRenderedOutput(null); }); it("does not infinite loop if there's a render phase update in the same render as an error", async () => { diff --git a/packages/react-reconciler/src/__tests__/ReactIncrementalErrorLogging-test.js b/packages/react-reconciler/src/__tests__/ReactIncrementalErrorLogging-test.js index d4c7949580c97..77f764566d4ae 100644 --- a/packages/react-reconciler/src/__tests__/ReactIncrementalErrorLogging-test.js +++ b/packages/react-reconciler/src/__tests__/ReactIncrementalErrorLogging-test.js @@ -250,16 +250,7 @@ describe('ReactIncrementalErrorLogging', () => { , ); await waitForAll( - [ - 'render: 0', - - 'render: 1', - - // Retry one more time before handling error - 'render: 1', - - 'componentWillUnmount: 0', - ].filter(Boolean), + ['render: 0', 'render: 1', 'componentWillUnmount: 0'].filter(Boolean), ); expect(console.error).toHaveBeenCalledTimes(1); diff --git a/packages/react/src/__tests__/ReactCoffeeScriptClass-test.coffee b/packages/react/src/__tests__/ReactCoffeeScriptClass-test.coffee index 033156d25f0fb..38652a3dabf19 100644 --- a/packages/react/src/__tests__/ReactCoffeeScriptClass-test.coffee +++ b/packages/react/src/__tests__/ReactCoffeeScriptClass-test.coffee @@ -56,11 +56,16 @@ describe 'ReactCoffeeScriptClass', -> expect(-> ReactDOM.flushSync -> root.render React.createElement(Foo) - ).toErrorDev([ - # A failed component renders twice in DEV in concurrent mode - 'No `render` method found on the Foo instance', - 'No `render` method found on the Foo instance', - ]) + ).toErrorDev( + if featureFlags.enableUnifiedSyncLane + ['No `render` method found on the Foo instance'] + else + [ + 'No `render` method found on the Foo instance', + # Retry in non-blocking updates + 'No `render` method found on the Foo instance', + ] + ) window.removeEventListener 'error', errorHandler; expect(caughtErrors).toEqual([ expect.objectContaining( diff --git a/packages/react/src/__tests__/ReactES6Class-test.js b/packages/react/src/__tests__/ReactES6Class-test.js index e7226dc0bee4e..8ffe69062318f 100644 --- a/packages/react/src/__tests__/ReactES6Class-test.js +++ b/packages/react/src/__tests__/ReactES6Class-test.js @@ -13,6 +13,7 @@ let PropTypes; let React; let ReactDOM; let ReactDOMClient; +let ReactFeatureFlags; describe('ReactES6Class', () => { let container; @@ -30,6 +31,7 @@ describe('ReactES6Class', () => { React = require('react'); ReactDOM = require('react-dom'); ReactDOMClient = require('react-dom/client'); + ReactFeatureFlags = require('shared/ReactFeatureFlags'); container = document.createElement('div'); root = ReactDOMClient.createRoot(container); attachedListener = null; @@ -69,13 +71,20 @@ describe('ReactES6Class', () => { try { expect(() => { ReactDOM.flushSync(() => root.render()); - }).toErrorDev([ - // A failed component renders twice in DEV in concurrent mode - 'Warning: No `render` method found on the Foo instance: ' + - 'you may have forgotten to define `render`.', - 'Warning: No `render` method found on the Foo instance: ' + - 'you may have forgotten to define `render`.', - ]); + }).toErrorDev( + ReactFeatureFlags.enableUnifiedSyncLane + ? [ + 'Warning: No `render` method found on the Foo instance: ' + + 'you may have forgotten to define `render`.', + ] + : [ + 'Warning: No `render` method found on the Foo instance: ' + + 'you may have forgotten to define `render`.', + // Retry in non-blocking updates + 'Warning: No `render` method found on the Foo instance: ' + + 'you may have forgotten to define `render`.', + ], + ); } finally { window.removeEventListener('error', errorHandler); } diff --git a/packages/react/src/__tests__/ReactTypeScriptClass-test.ts b/packages/react/src/__tests__/ReactTypeScriptClass-test.ts index 5a6e81bccf329..2d54441bde95e 100644 --- a/packages/react/src/__tests__/ReactTypeScriptClass-test.ts +++ b/packages/react/src/__tests__/ReactTypeScriptClass-test.ts @@ -337,13 +337,20 @@ describe('ReactTypeScriptClass', function() { try { expect(() => { ReactDOM.flushSync(() => root.render(React.createElement(Empty))) - }).toErrorDev([ - // A failed component renders twice in DEV in concurrent mode - 'Warning: No `render` method found on the Empty instance: ' + - 'you may have forgotten to define `render`.', - 'Warning: No `render` method found on the Empty instance: ' + - 'you may have forgotten to define `render`.', - ]); + }).toErrorDev( + ReactFeatureFlags.enableUnifiedSyncLane + ? [ + 'Warning: No `render` method found on the Empty instance: ' + + 'you may have forgotten to define `render`.' + ] + : [ + 'Warning: No `render` method found on the Empty instance: ' + + 'you may have forgotten to define `render`.', + // Retry in non-blocking updates + 'Warning: No `render` method found on the Empty instance: ' + + 'you may have forgotten to define `render`.', + ] + ); } finally { window.removeEventListener('error', errorHandler); } diff --git a/packages/use-sync-external-store/src/__tests__/useSyncExternalStoreShared-test.js b/packages/use-sync-external-store/src/__tests__/useSyncExternalStoreShared-test.js index e01ef57c10a64..815b9af4153be 100644 --- a/packages/use-sync-external-store/src/__tests__/useSyncExternalStoreShared-test.js +++ b/packages/use-sync-external-store/src/__tests__/useSyncExternalStoreShared-test.js @@ -588,11 +588,12 @@ describe('Shared useSyncExternalStore behavior (shim and built-in)', () => { } assertLog( - gate(flags => flags.enableUseSyncExternalStoreShim) + gate(flags => flags.enableUseSyncExternalStoreShim) || + gate(flags => flags.enableUnifiedSyncLane) ? ['Error in getSnapshot'] : [ 'Error in getSnapshot', - // In a concurrent root, React renders a second time to attempt to + // In a non-blocking update, React renders a second time to attempt to // recover from the error. 'Error in getSnapshot', ],