From 2d75d523f25b64ba4976c289687f5be7afaa99ab Mon Sep 17 00:00:00 2001 From: Sunil Pai Date: Mon, 22 Jul 2019 15:36:30 +0100 Subject: [PATCH] fix async act detection The previous version of async act detection left an open hanging act scope, which broke tests and expectations. This PR delays the detection until it's been called at least once. --- src/__tests__/new-act.js | 76 +++++++++++++++++++ src/__tests__/no-act.js | 73 ++++++++++++++++++- src/__tests__/old-act.js | 82 ++++++++++++++++++--- src/act-compat.js | 153 +++++++++++++++++++++++++++------------ 4 files changed, 325 insertions(+), 59 deletions(-) create mode 100644 src/__tests__/new-act.js diff --git a/src/__tests__/new-act.js b/src/__tests__/new-act.js new file mode 100644 index 00000000..56ce4970 --- /dev/null +++ b/src/__tests__/new-act.js @@ -0,0 +1,76 @@ +let asyncAct + +jest.mock('react-dom/test-utils', () => ({ + act: cb => { + return cb() + }, +})) + +beforeEach(() => { + jest.resetModules() + asyncAct = require('../act-compat').asyncAct + jest.spyOn(console, 'error').mockImplementation(() => {}) +}) + +afterEach(() => { + console.error.mockRestore() +}) + +test('async act works when it does not exist (older versions of react)', async () => { + const callback = jest.fn() + await asyncAct(async () => { + await Promise.resolve() + await callback() + }) + expect(console.error).toHaveBeenCalledTimes(0) + expect(callback).toHaveBeenCalledTimes(1) + + callback.mockClear() + console.error.mockClear() + + await asyncAct(async () => { + await Promise.resolve() + await callback() + }) + expect(console.error).toHaveBeenCalledTimes(0) + expect(callback).toHaveBeenCalledTimes(1) +}) + +test('async act recovers from errors', async () => { + try { + await asyncAct(async () => { + await null + throw new Error('test error') + }) + } catch (err) { + console.error('call console.error') + } + expect(console.error).toHaveBeenCalledTimes(1) + expect(console.error.mock.calls).toMatchInlineSnapshot(` + Array [ + Array [ + "call console.error", + ], + ] + `) +}) + +test('async act recovers from sync errors', async () => { + try { + await asyncAct(() => { + throw new Error('test error') + }) + } catch (err) { + console.error('call console.error') + } + expect(console.error).toHaveBeenCalledTimes(1) + expect(console.error.mock.calls).toMatchInlineSnapshot(` + Array [ + Array [ + "call console.error", + ], + ] + `) +}) + +/* eslint no-console:0 */ diff --git a/src/__tests__/no-act.js b/src/__tests__/no-act.js index 7bf6cd6a..039a79ae 100644 --- a/src/__tests__/no-act.js +++ b/src/__tests__/no-act.js @@ -1,4 +1,15 @@ -import {act} from '..' +let act, asyncAct + +beforeEach(() => { + jest.resetModules() + act = require('..').act + asyncAct = require('../act-compat').asyncAct + jest.spyOn(console, 'error').mockImplementation(() => {}) +}) + +afterEach(() => { + console.error.mockRestore() +}) jest.mock('react-dom/test-utils', () => ({})) @@ -6,4 +17,64 @@ test('act works even when there is no act from test utils', () => { const callback = jest.fn() act(callback) expect(callback).toHaveBeenCalledTimes(1) + expect(console.error).toHaveBeenCalledTimes(0) +}) + +test('async act works when it does not exist (older versions of react)', async () => { + const callback = jest.fn() + await asyncAct(async () => { + await Promise.resolve() + await callback() + }) + expect(console.error).toHaveBeenCalledTimes(0) + expect(callback).toHaveBeenCalledTimes(1) + + callback.mockClear() + console.error.mockClear() + + await asyncAct(async () => { + await Promise.resolve() + await callback() + }) + expect(console.error).toHaveBeenCalledTimes(0) + expect(callback).toHaveBeenCalledTimes(1) +}) + +test('async act recovers from errors', async () => { + try { + await asyncAct(async () => { + await null + throw new Error('test error') + }) + } catch (err) { + console.error('call console.error') + } + expect(console.error).toHaveBeenCalledTimes(1) + expect(console.error.mock.calls).toMatchInlineSnapshot(` + Array [ + Array [ + "call console.error", + ], + ] + `) +}) + +test('async act recovers from sync errors', async () => { + try { + await asyncAct(() => { + throw new Error('test error') + }) + } catch (err) { + console.error('call console.error') + } + expect(console.error).toHaveBeenCalledTimes(1) + expect(console.error.mock.calls).toMatchInlineSnapshot(` + Array [ + Array [ + "call console.error", + ], + ] + `) }) + +/* eslint no-console:0 */ diff --git a/src/__tests__/old-act.js b/src/__tests__/old-act.js index d21c2bae..1b7760eb 100644 --- a/src/__tests__/old-act.js +++ b/src/__tests__/old-act.js @@ -1,4 +1,14 @@ -import {asyncAct} from '../act-compat' +let asyncAct + +beforeEach(() => { + jest.resetModules() + asyncAct = require('../act-compat').asyncAct + jest.spyOn(console, 'error').mockImplementation(() => {}) +}) + +afterEach(() => { + console.error.mockRestore() +}) jest.mock('../react-dom-16.9.0-is-released', () => ({ reactDomSixteenPointNineIsReleased: true, @@ -6,30 +16,40 @@ jest.mock('../react-dom-16.9.0-is-released', () => ({ jest.mock('react-dom/test-utils', () => ({ act: cb => { - const promise = cb() + cb() return { then() { - console.error('blah, do not do this') - return promise + console.error( + 'Warning: Do not await the result of calling ReactTestUtils.act(...), it is not a Promise.', + ) }, } }, })) test('async act works even when the act is an old one', async () => { - jest.spyOn(console, 'error').mockImplementation(() => {}) const callback = jest.fn() await asyncAct(async () => { + console.error('sigil') await Promise.resolve() await callback() + console.error('sigil') }) expect(console.error.mock.calls).toMatchInlineSnapshot(` -Array [ - Array [ - "It looks like you're using a version of react-dom that supports the \\"act\\" function, but not an awaitable version of \\"act\\" which you will need. Please upgrade to at least react-dom@16.9.0 to remove this warning.", - ], -] -`) + Array [ + Array [ + Array [ + "sigil", + ], + ], + Array [ + "It looks like you're using a version of react-dom that supports the \\"act\\" function, but not an awaitable version of \\"act\\" which you will need. Please upgrade to at least react-dom@16.9.0 to remove this warning.", + ], + Array [ + "sigil", + ], + ] + `) expect(callback).toHaveBeenCalledTimes(1) // and it doesn't warn you twice @@ -42,8 +62,46 @@ Array [ }) expect(console.error).toHaveBeenCalledTimes(0) expect(callback).toHaveBeenCalledTimes(1) +}) - console.error.mockRestore() +test('async act recovers from async errors', async () => { + try { + await asyncAct(async () => { + await null + throw new Error('test error') + }) + } catch (err) { + console.error('call console.error') + } + expect(console.error).toHaveBeenCalledTimes(2) + expect(console.error.mock.calls).toMatchInlineSnapshot(` + Array [ + Array [ + "It looks like you're using a version of react-dom that supports the \\"act\\" function, but not an awaitable version of \\"act\\" which you will need. Please upgrade to at least react-dom@16.9.0 to remove this warning.", + ], + Array [ + "call console.error", + ], + ] + `) +}) + +test('async act recovers from sync errors', async () => { + try { + await asyncAct(() => { + throw new Error('test error') + }) + } catch (err) { + console.error('call console.error') + } + expect(console.error).toHaveBeenCalledTimes(1) + expect(console.error.mock.calls).toMatchInlineSnapshot(` + Array [ + Array [ + "call console.error", + ], + ] + `) }) /* eslint no-console:0 */ diff --git a/src/act-compat.js b/src/act-compat.js index a87ee4d7..2c1a53c5 100644 --- a/src/act-compat.js +++ b/src/act-compat.js @@ -2,29 +2,8 @@ import React from 'react' import ReactDOM from 'react-dom' import {reactDomSixteenPointNineIsReleased} from './react-dom-16.9.0-is-released' -let reactAct -let actSupported = false -let asyncActSupported = false -try { - reactAct = require('react-dom/test-utils').act - actSupported = reactAct !== undefined - - const originalError = console.error - let errorCalled = false - console.error = () => { - errorCalled = true - } - console.error.calls = [] - /* istanbul ignore next */ - reactAct(() => ({then: () => {}})).then(() => {}) - /* istanbul ignore next */ - if (!errorCalled) { - asyncActSupported = true - } - console.error = originalError -} catch (error) { - // ignore, this is to support old versions of react -} +const reactAct = require('react-dom/test-utils').act +const actSupported = reactAct !== undefined // act is supported react-dom@16.8.0 // so for versions that don't have act from test utils @@ -38,32 +17,114 @@ function actPolyfill(cb) { const act = reactAct || actPolyfill let youHaveBeenWarned = false -// this will not avoid warnings that react-dom 16.8.0 logs for triggering -// state updates asynchronously, but at least we can tell people they need -// to upgrade to avoid the warnings. -async function asyncActPolyfill(cb) { - // istanbul-ignore-next - if ( - !youHaveBeenWarned && - actSupported && - reactDomSixteenPointNineIsReleased - ) { - // if act is supported and async act isn't and they're trying to use async - // act, then they need to upgrade from 16.8 to 16.9. - // This is a seemless upgrade, so we'll add a warning - console.error( - `It looks like you're using a version of react-dom that supports the "act" function, but not an awaitable version of "act" which you will need. Please upgrade to at least react-dom@16.9.0 to remove this warning.`, - ) - youHaveBeenWarned = true +let isAsyncActSupported = null + +function asyncAct(cb) { + if (actSupported === true) { + if (isAsyncActSupported === null) { + return new Promise((resolve, reject) => { + // patch console.error here + const originalConsoleError = console.error + console.error = function error(...args) { + /* if console.error fired *with that specific message* */ + if ( + args[0].indexOf( + 'Warning: Do not await the result of calling ReactTestUtils.act', + ) === 0 + ) { + // v16.8.6 + isAsyncActSupported = false + } else if ( + args[0].indexOf( + 'Warning: The callback passed to ReactTestUtils.act(...) function must not return anything', + ) === 0 + ) { + // no-op + } else { + originalConsoleError.call(console, args) + } + } + let cbReturn, result + try { + result = reactAct(() => { + cbReturn = cb() + return cbReturn + }) + } catch (err) { + console.error = originalConsoleError + reject(err) + return + } + + result.then( + () => { + console.error = originalConsoleError + // if it got here, it means async act is supported + isAsyncActSupported = true + resolve() + }, + err => { + console.error = originalConsoleError + isAsyncActSupported = true + reject(err) + }, + ) + + // 16.8.6's act().then() doesn't call a resolve handler, so we need to manually flush here, sigh + + if (isAsyncActSupported === false) { + console.error = originalConsoleError + /* istanbul-ignore-next */ + if (!youHaveBeenWarned && reactDomSixteenPointNineIsReleased) { + // if act is supported and async act isn't and they're trying to use async + // act, then they need to upgrade from 16.8 to 16.9. + // This is a seemless upgrade, so we'll add a warning + console.error( + `It looks like you're using a version of react-dom that supports the "act" function, but not an awaitable version of "act" which you will need. Please upgrade to at least react-dom@16.9.0 to remove this warning.`, + ) + youHaveBeenWarned = true + } + + cbReturn.then(() => { + // a faux-version. + // todo - copy https://github.com/facebook/react/blob/master/packages/shared/enqueueTask.js + Promise.resolve().then(() => { + // use sync act to flush effects + act(() => {}) + resolve() + }) + }, reject) + } + }) + } else if (isAsyncActSupported === false) { + // use the polyfill directly + let result + act(() => { + result = cb() + }) + return result.then(() => { + return Promise.resolve().then(() => { + // use sync act to flush effects + act(() => {}) + }) + }) + } + // all good! regular act + return act(cb) } - await cb() - // make all effects resolve after - act(() => {}) + // use the polyfill + let result + act(() => { + result = cb() + }) + return result.then(() => { + return Promise.resolve().then(() => { + // use sync act to flush effects + act(() => {}) + }) + }) } -// istanbul ignore next -const asyncAct = asyncActSupported ? reactAct : asyncActPolyfill - export default act export {asyncAct}