From 3ba7add608ec8338cbcaea26d5935401853d2959 Mon Sep 17 00:00:00 2001 From: Sebastian Silbermann Date: Thu, 1 Dec 2022 11:26:43 +0100 Subject: [PATCH] Allow async blocks in `to(Error|Warn)Dev` (#25338) --- .../ReactHooksWithNoopRenderer-test.js | 66 +++++++++---------- .../ReactCoffeeScriptClass-test.coffee | 4 ++ .../react/src/__tests__/ReactES6Class-test.js | 16 +++-- .../useSyncExternalStoreShared-test.js | 29 +++++--- scripts/jest/matchers/toWarnDev.js | 50 ++++++++++++-- 5 files changed, 113 insertions(+), 52 deletions(-) diff --git a/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.js b/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.js index d16b07fc0772f..25d19be6b48d3 100644 --- a/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.js +++ b/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.js @@ -1777,7 +1777,7 @@ describe('ReactHooksWithNoopRenderer', () => { }, [props.count]); return ; } - expect(() => + expect(() => { act(() => { ReactNoop.render(, () => Scheduler.unstable_yieldValue('Sync effect'), @@ -1787,8 +1787,8 @@ describe('ReactHooksWithNoopRenderer', () => { 'Sync effect', ]); expect(ReactNoop.getChildren()).toEqual([span('Count: (empty)')]); - }), - ).toErrorDev('flushSync was called from inside a lifecycle method'); + }); + }).toErrorDev('flushSync was called from inside a lifecycle method'); expect(ReactNoop.getChildren()).toEqual([span('Count: 0')]); }); @@ -2648,32 +2648,32 @@ describe('ReactHooksWithNoopRenderer', () => { } const root1 = ReactNoop.createRoot(); - expect(() => + expect(() => { act(() => { root1.render(); - }), - ).toErrorDev([ + }); + }).toErrorDev([ 'Warning: useEffect must not return anything besides a ' + 'function, which is used for clean-up. You returned: 17', ]); const root2 = ReactNoop.createRoot(); - expect(() => + expect(() => { act(() => { root2.render(); - }), - ).toErrorDev([ + }); + }).toErrorDev([ 'Warning: useEffect must not return anything besides a ' + 'function, which is used for clean-up. You returned null. If your ' + 'effect does not require clean up, return undefined (or nothing).', ]); const root3 = ReactNoop.createRoot(); - expect(() => + expect(() => { act(() => { root3.render(); - }), - ).toErrorDev([ + }); + }).toErrorDev([ 'Warning: useEffect must not return anything besides a ' + 'function, which is used for clean-up.\n\n' + 'It looks like you wrote useEffect(async () => ...) or returned a Promise.', @@ -3052,32 +3052,32 @@ describe('ReactHooksWithNoopRenderer', () => { } const root1 = ReactNoop.createRoot(); - expect(() => + expect(() => { act(() => { root1.render(); - }), - ).toErrorDev([ + }); + }).toErrorDev([ 'Warning: useInsertionEffect must not return anything besides a ' + 'function, which is used for clean-up. You returned: 17', ]); const root2 = ReactNoop.createRoot(); - expect(() => + expect(() => { act(() => { root2.render(); - }), - ).toErrorDev([ + }); + }).toErrorDev([ 'Warning: useInsertionEffect must not return anything besides a ' + 'function, which is used for clean-up. You returned null. If your ' + 'effect does not require clean up, return undefined (or nothing).', ]); const root3 = ReactNoop.createRoot(); - expect(() => + expect(() => { act(() => { root3.render(); - }), - ).toErrorDev([ + }); + }).toErrorDev([ 'Warning: useInsertionEffect must not return anything besides a ' + 'function, which is used for clean-up.\n\n' + 'It looks like you wrote useInsertionEffect(async () => ...) or returned a Promise.', @@ -3104,11 +3104,11 @@ describe('ReactHooksWithNoopRenderer', () => { } const root = ReactNoop.createRoot(); - expect(() => + expect(() => { act(() => { root.render(); - }), - ).toErrorDev(['Warning: useInsertionEffect must not schedule updates.']); + }); + }).toErrorDev(['Warning: useInsertionEffect must not schedule updates.']); expect(() => { act(() => { @@ -3359,32 +3359,32 @@ describe('ReactHooksWithNoopRenderer', () => { } const root1 = ReactNoop.createRoot(); - expect(() => + expect(() => { act(() => { root1.render(); - }), - ).toErrorDev([ + }); + }).toErrorDev([ 'Warning: useLayoutEffect must not return anything besides a ' + 'function, which is used for clean-up. You returned: 17', ]); const root2 = ReactNoop.createRoot(); - expect(() => + expect(() => { act(() => { root2.render(); - }), - ).toErrorDev([ + }); + }).toErrorDev([ 'Warning: useLayoutEffect must not return anything besides a ' + 'function, which is used for clean-up. You returned null. If your ' + 'effect does not require clean up, return undefined (or nothing).', ]); const root3 = ReactNoop.createRoot(); - expect(() => + expect(() => { act(() => { root3.render(); - }), - ).toErrorDev([ + }); + }).toErrorDev([ 'Warning: useLayoutEffect must not return anything besides a ' + 'function, which is used for clean-up.\n\n' + 'It looks like you wrote useLayoutEffect(async () => ...) or returned a Promise.', diff --git a/packages/react/src/__tests__/ReactCoffeeScriptClass-test.coffee b/packages/react/src/__tests__/ReactCoffeeScriptClass-test.coffee index c9dee4ac6c7fa..0fd7e0b1e2222 100644 --- a/packages/react/src/__tests__/ReactCoffeeScriptClass-test.coffee +++ b/packages/react/src/__tests__/ReactCoffeeScriptClass-test.coffee @@ -133,6 +133,7 @@ describe 'ReactCoffeeScriptClass', -> expect(-> act -> root.render React.createElement(Foo, foo: 'foo') + return ).toErrorDev 'Foo: getDerivedStateFromProps() is defined as an instance method and will be ignored. Instead, declare it as a static method.' it 'warns if getDerivedStateFromError is not static', -> @@ -144,6 +145,7 @@ describe 'ReactCoffeeScriptClass', -> expect(-> act -> root.render React.createElement(Foo, foo: 'foo') + return ).toErrorDev 'Foo: getDerivedStateFromError() is defined as an instance method and will be ignored. Instead, declare it as a static method.' it 'warns if getSnapshotBeforeUpdate is static', -> @@ -155,6 +157,7 @@ describe 'ReactCoffeeScriptClass', -> expect(-> act -> root.render React.createElement(Foo, foo: 'foo') + return ).toErrorDev 'Foo: getSnapshotBeforeUpdate() is defined as a static method and will be ignored. Instead, declare it as an instance method.' it 'warns if state not initialized before static getDerivedStateFromProps', -> @@ -171,6 +174,7 @@ describe 'ReactCoffeeScriptClass', -> expect(-> act -> root.render React.createElement(Foo, foo: 'foo') + return ).toErrorDev ( '`Foo` uses `getDerivedStateFromProps` but its initial state is ' + 'undefined. This is not recommended. Instead, define the initial state by ' + diff --git a/packages/react/src/__tests__/ReactES6Class-test.js b/packages/react/src/__tests__/ReactES6Class-test.js index 53a44f9fb17df..6c23038205ed7 100644 --- a/packages/react/src/__tests__/ReactES6Class-test.js +++ b/packages/react/src/__tests__/ReactES6Class-test.js @@ -149,7 +149,9 @@ describe('ReactES6Class', () => { return
; } } - expect(() => act(() => root.render())).toErrorDev( + expect(() => { + act(() => root.render()); + }).toErrorDev( 'Foo: getDerivedStateFromProps() is defined as an instance method ' + 'and will be ignored. Instead, declare it as a static method.', ); @@ -164,7 +166,9 @@ describe('ReactES6Class', () => { return
; } } - expect(() => act(() => root.render())).toErrorDev( + expect(() => { + act(() => root.render()); + }).toErrorDev( 'Foo: getDerivedStateFromError() is defined as an instance method ' + 'and will be ignored. Instead, declare it as a static method.', ); @@ -177,7 +181,9 @@ describe('ReactES6Class', () => { return
; } } - expect(() => act(() => root.render())).toErrorDev( + expect(() => { + act(() => root.render()); + }).toErrorDev( 'Foo: getSnapshotBeforeUpdate() is defined as a static method ' + 'and will be ignored. Instead, declare it as an instance method.', ); @@ -195,7 +201,9 @@ describe('ReactES6Class', () => { return
; } } - expect(() => act(() => root.render())).toErrorDev( + expect(() => { + act(() => root.render()); + }).toErrorDev( '`Foo` uses `getDerivedStateFromProps` but its initial state is ' + 'undefined. This is not recommended. Instead, define the initial state by ' + 'assigning an object to `this.state` in the constructor of `Foo`. ' + 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 7251b65819be9..80fb794bfc4d2 100644 --- a/packages/use-sync-external-store/src/__tests__/useSyncExternalStoreShared-test.js +++ b/packages/use-sync-external-store/src/__tests__/useSyncExternalStoreShared-test.js @@ -14,6 +14,7 @@ let useSyncExternalStoreWithSelector; let React; let ReactDOM; let ReactDOMClient; +let ReactFeatureFlags; let Scheduler; let act; let useState; @@ -48,6 +49,7 @@ describe('Shared useSyncExternalStore behavior (shim and built-in)', () => { React = require('react'); ReactDOM = require('react-dom'); ReactDOMClient = require('react-dom/client'); + ReactFeatureFlags = require('shared/ReactFeatureFlags'); Scheduler = require('scheduler'); useState = React.useState; useEffect = React.useEffect; @@ -882,8 +884,7 @@ describe('Shared useSyncExternalStore behavior (shim and built-in)', () => { describe('selector and isEqual error handling in extra', () => { let ErrorBoundary; - beforeAll(() => { - spyOnDev(console, 'warn'); + beforeEach(() => { ErrorBoundary = class extends React.Component { state = {error: null}; static getDerivedStateFromError(error) { @@ -929,9 +930,15 @@ describe('Shared useSyncExternalStore behavior (shim and built-in)', () => { expect(container.textContent).toEqual('A'); - await act(() => { - store.set({}); - }); + await expect(async () => { + await act(async () => { + store.set({}); + }); + }).toWarnDev( + ReactFeatureFlags.enableUseRefAccessWarning + ? ['Warning: App: Unsafe read of a mutable value during render.'] + : [], + ); expect(container.textContent).toEqual('Malformed state'); }); @@ -968,9 +975,15 @@ describe('Shared useSyncExternalStore behavior (shim and built-in)', () => { expect(container.textContent).toEqual('A'); - await act(() => { - store.set({}); - }); + await expect(async () => { + await act(() => { + store.set({}); + }); + }).toWarnDev( + ReactFeatureFlags.enableUseRefAccessWarning + ? ['Warning: App: Unsafe read of a mutable value during render.'] + : [], + ); expect(container.textContent).toEqual('Malformed state'); }); }); diff --git a/scripts/jest/matchers/toWarnDev.js b/scripts/jest/matchers/toWarnDev.js index 5e2a144b4e29c..19d2dd17d8e5a 100644 --- a/scripts/jest/matchers/toWarnDev.js +++ b/scripts/jest/matchers/toWarnDev.js @@ -152,11 +152,7 @@ const createMatcherFor = (consoleMethod, matcherName) => // Avoid using Jest's built-in spy since it can't be removed. console[consoleMethod] = consoleSpy; - try { - callback(); - } catch (error) { - caughtError = error; - } finally { + const onFinally = () => { // Restore the unspied method so that unexpected errors fail tests. console[consoleMethod] = originalMethod; @@ -259,12 +255,52 @@ const createMatcherFor = (consoleMethod, matcherName) => } return {pass: true}; + }; + + let returnPromise = null; + try { + const result = callback(); + + if ( + typeof result === 'object' && + result !== null && + typeof result.then === 'function' + ) { + // `act` returns a thenable that can't be chained. + // Once `act(async () => {}).then(() => {}).then(() => {})` works + // we can just return `result.then(onFinally, error => ...)` + returnPromise = new Promise((resolve, reject) => { + result.then( + () => { + resolve(onFinally()); + }, + error => { + caughtError = error; + return resolve(onFinally()); + } + ); + }); + } + } catch (error) { + caughtError = error; + } finally { + return returnPromise === null ? onFinally() : returnPromise; } } else { // Any uncaught errors or warnings should fail tests in production mode. - callback(); + const result = callback(); - return {pass: true}; + if ( + typeof result === 'object' && + result !== null && + typeof result.then === 'function' + ) { + return result.then(() => { + return {pass: true}; + }); + } else { + return {pass: true}; + } } };