From 01918d923b9e7e40118d8ae300e55dabf9483bee Mon Sep 17 00:00:00 2001 From: Hu Chen Date: 2020εΉ΄8月17ζ—₯ 21:04:15 +0800 Subject: [PATCH 1/2] feat: add addCleanup --- .all-contributorsrc | 11 +++++++++++ README.md | 4 +++- docs/api-reference.md | 26 ++++++++++++++++++++++++++ src/cleanup.js | 20 +++++++++++++------- src/pure.js | 8 ++++---- test/addCleanup.test.js | 27 +++++++++++++++++++++++++++ 6 files changed, 84 insertions(+), 12 deletions(-) create mode 100644 test/addCleanup.test.js diff --git a/.all-contributorsrc b/.all-contributorsrc index be8f304a..dc53c0ec 100644 --- a/.all-contributorsrc +++ b/.all-contributorsrc @@ -183,6 +183,17 @@ "contributions": [ "test" ] + }, + { + "login": "huchenme", + "name": "Hu Chen", + "avatar_url": "https://avatars3.githubusercontent.com/u/2078389?v=4", + "profile": "https://huchen.dev/", + "contributions": [ + "code", + "doc", + "example" + ] } ], "commitConvention": "none" diff --git a/README.md b/README.md index e40d2ad9..edef6733 100644 --- a/README.md +++ b/README.md @@ -164,11 +164,13 @@ Thanks goes to these wonderful people ([emoji key](https://allcontributors.org/d
Roman Gusev
πŸ“–
Adam Seckel
πŸ’»
keiya sasaki
⚠️ +
Hu Chen
πŸ’» πŸ“– πŸ’‘ - + + This project follows the [all-contributors](https://allcontributors.org/) specification. diff --git a/docs/api-reference.md b/docs/api-reference.md index 53c7623a..16413d8f 100644 --- a/docs/api-reference.md +++ b/docs/api-reference.md @@ -10,6 +10,7 @@ route: '/reference/api' - [`renderHook`](/reference/api#renderhook) - [`act`](/reference/api#act) - [`cleanup`](/reference/api#cleanup) +- [`addCleanup`](/reference/api#addcleanup) --- @@ -147,6 +148,31 @@ variable to `true` before importing `@testing-library/react-hooks` will also dis --- +## `addCleanup` + +```js +function addCleanup( + callback: function(props?: any): any +): void +``` + +Callback to be called after `cleanup`. + +In some cases you might want to run some callback after internal `cleanup` happen, especially after +`unmount` happens in `cleanup`. If the sequence matters to you, you could use `addCleanup`. + +```js +import { addCleanup } from '@testing-library/react-hooks' + +jest.useFakeTimers() + +addCleanup(() => { + jest.runOnlyPendingTimers() +}) +``` + +--- + ## Async Utilities ### `waitForNextUpdate` diff --git a/src/cleanup.js b/src/cleanup.js index c240b5e1..60fd5be8 100644 --- a/src/cleanup.js +++ b/src/cleanup.js @@ -1,19 +1,25 @@ import flushMicroTasks from './flush-microtasks' -let cleanupCallbacks = [] +let internalCleanupCbs = [] +let cleanupCbs = [] async function cleanup() { await flushMicroTasks() - cleanupCallbacks.forEach((cb) => cb()) - cleanupCallbacks = [] + internalCleanupCbs.forEach((cb) => cb()) + internalCleanupCbs = [] + cleanupCbs.forEach((cb) => cb()) +} + +function addInternalCleanup(callback) { + internalCleanupCbs.push(callback) } function addCleanup(callback) { - cleanupCallbacks.push(callback) + cleanupCbs.push(callback) } -function removeCleanup(callback) { - cleanupCallbacks = cleanupCallbacks.filter((cb) => cb !== callback) +function removeInternalCleanup(callback) { + internalCleanupCbs = internalCleanupCbs.filter((cb) => cb !== callback) } -export { cleanup, addCleanup, removeCleanup } +export { cleanup, addCleanup, addInternalCleanup, removeInternalCleanup } diff --git a/src/pure.js b/src/pure.js index 3b4a475d..e2427f64 100644 --- a/src/pure.js +++ b/src/pure.js @@ -1,7 +1,7 @@ import React, { Suspense } from 'react' import { act, create } from 'react-test-renderer' import asyncUtils from './asyncUtils' -import { cleanup, addCleanup, removeCleanup } from './cleanup' +import { cleanup, addCleanup, addInternalCleanup, removeInternalCleanup } from './cleanup' function TestHook({ callback, hookProps, onError, children }) { try { @@ -84,12 +84,12 @@ function renderHook(callback, { initialProps, wrapper } = {}) { function unmountHook() { act(() => { - removeCleanup(unmountHook) + removeInternalCleanup(unmountHook) unmount() }) } - addCleanup(unmountHook) + addInternalCleanup(unmountHook) return { result, @@ -99,4 +99,4 @@ function renderHook(callback, { initialProps, wrapper } = {}) { } } -export { renderHook, cleanup, act } +export { renderHook, cleanup, act, addCleanup } diff --git a/test/addCleanup.test.js b/test/addCleanup.test.js new file mode 100644 index 00000000..8e74e73a --- /dev/null +++ b/test/addCleanup.test.js @@ -0,0 +1,27 @@ +import { useEffect } from 'react' +import { renderHook, addCleanup } from 'src' + +let callSequence = [] +addCleanup(() => { + callSequence.push('cleanup') +}) +addCleanup(() => { + callSequence.push('another cleanup') +}) + +describe('addCleanup tests', () => { + test('first', () => { + const hookWithCleanup = () => { + useEffect(() => { + return () => { + callSequence.push('unmount') + } + }) + } + renderHook(() => hookWithCleanup()) + }) + + test('second', () => { + expect(callSequence).toEqual(['unmount', 'cleanup', 'another cleanup']) + }) +}) From 8d0fc8259e12ae790ec50384741444b37ae4388d Mon Sep 17 00:00:00 2001 From: Michael Peyper Date: Sun, 6 Sep 2020 21:28:18 +1000 Subject: [PATCH 2/2] Refactor addCleanup to not treat unmount differently for external cleanups --- docs/api-reference.md | 32 ++++++++------ src/cleanup.js | 23 +++++----- src/pure.js | 8 ++-- test/addCleanup.test.js | 27 ------------ test/cleanup.test.js | 96 ++++++++++++++++++++++++++++++++++++++++- 5 files changed, 128 insertions(+), 58 deletions(-) delete mode 100644 test/addCleanup.test.js diff --git a/docs/api-reference.md b/docs/api-reference.md index 16413d8f..e98f6e92 100644 --- a/docs/api-reference.md +++ b/docs/api-reference.md @@ -109,7 +109,9 @@ This is the same [`act` function](https://reactjs.org/docs/test-utils.html#act) function cleanup: Promise ``` -Unmounts any rendered hooks rendered with `renderHook`, ensuring all effects have been flushed. +Unmounts any rendered hooks rendered with `renderHook`, ensuring all effects have been flushed. Any +callbacks added with [`addCleanup`](<(/reference/api#addcleanup).>) will also be called when +`cleanup` is run. > Please note that this is done automatically if the testing framework you're using supports the > `afterEach` global (like Jest, mocha and Jasmine). If not, you will need to do manual cleanups @@ -151,26 +153,30 @@ variable to `true` before importing `@testing-library/react-hooks` will also dis ## `addCleanup` ```js -function addCleanup( - callback: function(props?: any): any -): void +function addCleanup(callback: function(): void|Promise): function(): void ``` -Callback to be called after `cleanup`. +Add a callback to be called during [`cleanup`](/reference/api#cleanup), returning a function to +remove the cleanup if is no longer required. Cleanups are called in reverse order to being added. +This is usually only relevant when wanting a cleanup to run after the component has been unmounted. -In some cases you might want to run some callback after internal `cleanup` happen, especially after -`unmount` happens in `cleanup`. If the sequence matters to you, you could use `addCleanup`. +If the provided callback is an `async` function or returns a promise, `cleanup` will wait for it to +be resolved before moving onto the next cleanup callback. -```js -import { addCleanup } from '@testing-library/react-hooks' +> Please note that any cleanups added using `addCleanup` are removed after `cleanup` is called. For +> cleanups that need to run with every test, it is advised to add them in a `beforeEach` block (or +> equivalent for your test runner). -jest.useFakeTimers() +## `removeCleanup` -addCleanup(() => { - jest.runOnlyPendingTimers() -}) +```js +function removeCleanup(callback: function(): void|Promise): void ``` +Removes a cleanup callback previously added with [`addCleanup`](/reference/api#addCleanup). Once +removed, the provided callback will no longer execute as part of running +[`cleanup`](/reference/api#cleanup). + --- ## Async Utilities diff --git a/src/cleanup.js b/src/cleanup.js index 60fd5be8..3699180a 100644 --- a/src/cleanup.js +++ b/src/cleanup.js @@ -1,25 +1,22 @@ import flushMicroTasks from './flush-microtasks' -let internalCleanupCbs = [] -let cleanupCbs = [] +let cleanupCallbacks = [] async function cleanup() { await flushMicroTasks() - internalCleanupCbs.forEach((cb) => cb()) - internalCleanupCbs = [] - cleanupCbs.forEach((cb) => cb()) -} - -function addInternalCleanup(callback) { - internalCleanupCbs.push(callback) + for (const callback of cleanupCallbacks) { + await callback() + } + cleanupCallbacks = [] } function addCleanup(callback) { - cleanupCbs.push(callback) + cleanupCallbacks.unshift(callback) + return () => removeCleanup(callback) } -function removeInternalCleanup(callback) { - internalCleanupCbs = internalCleanupCbs.filter((cb) => cb !== callback) +function removeCleanup(callback) { + cleanupCallbacks = cleanupCallbacks.filter((cb) => cb !== callback) } -export { cleanup, addCleanup, addInternalCleanup, removeInternalCleanup } +export { cleanup, addCleanup, removeCleanup } diff --git a/src/pure.js b/src/pure.js index e2427f64..53ef84c5 100644 --- a/src/pure.js +++ b/src/pure.js @@ -1,7 +1,7 @@ import React, { Suspense } from 'react' import { act, create } from 'react-test-renderer' import asyncUtils from './asyncUtils' -import { cleanup, addCleanup, addInternalCleanup, removeInternalCleanup } from './cleanup' +import { cleanup, addCleanup, removeCleanup } from './cleanup' function TestHook({ callback, hookProps, onError, children }) { try { @@ -84,12 +84,12 @@ function renderHook(callback, { initialProps, wrapper } = {}) { function unmountHook() { act(() => { - removeInternalCleanup(unmountHook) + removeCleanup(unmountHook) unmount() }) } - addInternalCleanup(unmountHook) + addCleanup(unmountHook) return { result, @@ -99,4 +99,4 @@ function renderHook(callback, { initialProps, wrapper } = {}) { } } -export { renderHook, cleanup, act, addCleanup } +export { renderHook, cleanup, addCleanup, removeCleanup, act } diff --git a/test/addCleanup.test.js b/test/addCleanup.test.js deleted file mode 100644 index 8e74e73a..00000000 --- a/test/addCleanup.test.js +++ /dev/null @@ -1,27 +0,0 @@ -import { useEffect } from 'react' -import { renderHook, addCleanup } from 'src' - -let callSequence = [] -addCleanup(() => { - callSequence.push('cleanup') -}) -addCleanup(() => { - callSequence.push('another cleanup') -}) - -describe('addCleanup tests', () => { - test('first', () => { - const hookWithCleanup = () => { - useEffect(() => { - return () => { - callSequence.push('unmount') - } - }) - } - renderHook(() => hookWithCleanup()) - }) - - test('second', () => { - expect(callSequence).toEqual(['unmount', 'cleanup', 'another cleanup']) - }) -}) diff --git a/test/cleanup.test.js b/test/cleanup.test.js index a8c3bbba..8e7a44b0 100644 --- a/test/cleanup.test.js +++ b/test/cleanup.test.js @@ -1,5 +1,5 @@ import { useEffect } from 'react' -import { renderHook, cleanup } from 'src' +import { renderHook, cleanup, addCleanup, removeCleanup } from 'src/pure' describe('cleanup tests', () => { test('should flush effects on cleanup', async () => { @@ -38,4 +38,98 @@ describe('cleanup tests', () => { expect(cleanupCalled[1]).toBe(true) expect(cleanupCalled[2]).toBe(true) }) + + test('should call cleanups in reverse order', async () => { + let callSequence = [] + addCleanup(() => { + callSequence.push('cleanup') + }) + addCleanup(() => { + callSequence.push('another cleanup') + }) + const hookWithCleanup = () => { + useEffect(() => { + return () => { + callSequence.push('unmount') + } + }) + } + renderHook(() => hookWithCleanup()) + + await cleanup() + + expect(callSequence).toEqual(['unmount', 'another cleanup', 'cleanup']) + }) + + test('should wait for async cleanup', async () => { + let callSequence = [] + addCleanup(() => { + callSequence.push('cleanup') + }) + addCleanup(async () => { + await new Promise((resolve) => setTimeout(resolve, 10)) + callSequence.push('another cleanup') + }) + const hookWithCleanup = () => { + useEffect(() => { + return () => { + callSequence.push('unmount') + } + }) + } + renderHook(() => hookWithCleanup()) + + await cleanup() + + expect(callSequence).toEqual(['unmount', 'another cleanup', 'cleanup']) + }) + + test('should remove cleanup using removeCleanup', async () => { + let callSequence = [] + addCleanup(() => { + callSequence.push('cleanup') + }) + const anotherCleanup = () => { + callSequence.push('another cleanup') + } + addCleanup(anotherCleanup) + const hookWithCleanup = () => { + useEffect(() => { + return () => { + callSequence.push('unmount') + } + }) + } + renderHook(() => hookWithCleanup()) + + removeCleanup(anotherCleanup) + + await cleanup() + + expect(callSequence).toEqual(['unmount', 'cleanup']) + }) + + test('should remove cleanup using returned handler', async () => { + let callSequence = [] + addCleanup(() => { + callSequence.push('cleanup') + }) + const remove = addCleanup(() => { + callSequence.push('another cleanup') + }) + const hookWithCleanup = () => { + useEffect(() => { + return () => { + callSequence.push('unmount') + } + }) + } + renderHook(() => hookWithCleanup()) + + remove() + + await cleanup() + + expect(callSequence).toEqual(['unmount', 'cleanup']) + }) })

AltStyle γ«γ‚ˆγ£γ¦ε€‰ζ›γ•γ‚ŒγŸγƒšγƒΌγ‚Έ (->γ‚ͺγƒͺγ‚ΈγƒŠγƒ«) /