From dc21e5947a54adfaf348bfeaeab56d37a4e7e2bb Mon Sep 17 00:00:00 2001 From: Michael Peyper Date: Mon, 11 Jan 2021 21:08:15 +1100 Subject: [PATCH] fix: fixed potential error when hook suspends and current result is accessed BREAKING CHANGE: Adjust types so that react renderer exports don't required extra generic parameter --- package.json | 2 +- src/__tests__/defaultRenderer.test.ts | 41 +++++++++++++++++++++++ src/core/cleanup.ts | 8 +++-- src/core/index.ts | 24 +++++-------- src/dom/__tests__/suspenseHook.test.ts | 6 ++++ src/dom/pure.ts | 10 +++--- src/helpers/createTestHarness.tsx | 8 +---- src/native/__tests__/suspenseHook.test.ts | 6 ++++ src/native/pure.ts | 10 +++--- src/pure.ts | 1 - src/server/pure.ts | 10 +++--- src/types/index.ts | 30 ++++++----------- src/types/internal.ts | 8 ----- src/types/react.ts | 19 ++++++++--- 14 files changed, 110 insertions(+), 73 deletions(-) create mode 100644 src/__tests__/defaultRenderer.test.ts delete mode 100644 src/types/internal.ts diff --git a/package.json b/package.json index 3f1ec9d5..86aee639 100644 --- a/package.json +++ b/package.json @@ -56,7 +56,7 @@ "docz-theme-default": "1.2.0", "docz-utils": "2.3.0", "eslint": "7.17.0", - "kcd-scripts": "7.5.3", + "kcd-scripts": "7.5.4", "prettier": "^2.2.1", "react": "17.0.1", "react-dom": "^17.0.1", diff --git a/src/__tests__/defaultRenderer.test.ts b/src/__tests__/defaultRenderer.test.ts new file mode 100644 index 00000000..0352813d --- /dev/null +++ b/src/__tests__/defaultRenderer.test.ts @@ -0,0 +1,41 @@ +/* eslint-disable @typescript-eslint/no-var-requires */ +import { ReactHooksRenderer } from '../types/react' + +describe('default renderer', () => { + beforeEach(() => { + jest.resetModules() + }) + + test('should resolve native renderer as default renderer', () => { + const expectedRenderer = require('../native/pure') as ReactHooksRenderer + const actualRenderer = require('..') as ReactHooksRenderer + + expect(actualRenderer).toEqual(expectedRenderer) + }) + + test('should resolve dom renderer as default renderer', () => { + jest.doMock('react-test-renderer', () => { + throw new Error('missing dependency') + }) + + const expectedRenderer = require('../dom/pure') as ReactHooksRenderer + const actualRenderer = require('..') as ReactHooksRenderer + + expect(actualRenderer).toEqual(expectedRenderer) + }) + + test('should throw error if a default renderer cannot be resolved', () => { + jest.doMock('react-test-renderer', () => { + throw new Error('missing dependency') + }) + + jest.doMock('react-dom', () => { + throw new Error('missing dependency') + }) + + const expectedMessage = + "Could not auto-detect a React renderer. Are you sure you've installed one of the following\n - react-dom\n - react-test-renderer" + + expect(() => require('..')).toThrowError(new Error(expectedMessage)) + }) +}) diff --git a/src/core/cleanup.ts b/src/core/cleanup.ts index 2a56d5b1..e521b1fb 100644 --- a/src/core/cleanup.ts +++ b/src/core/cleanup.ts @@ -1,4 +1,6 @@ -let cleanupCallbacks: (() => Promise | void)[] = [] +import { CleanupCallback } from '../types' + +let cleanupCallbacks: Array = [] async function cleanup() { for (const callback of cleanupCallbacks) { @@ -7,12 +9,12 @@ async function cleanup() { cleanupCallbacks = [] } -function addCleanup(callback: () => Promise | void) { +function addCleanup(callback: CleanupCallback) { cleanupCallbacks = [callback, ...cleanupCallbacks] return () => removeCleanup(callback) } -function removeCleanup(callback: () => Promise | void) { +function removeCleanup(callback: CleanupCallback) { cleanupCallbacks = cleanupCallbacks.filter((cb) => cb !== callback) } diff --git a/src/core/index.ts b/src/core/index.ts index ac9b555a..f3c50981 100644 --- a/src/core/index.ts +++ b/src/core/index.ts @@ -1,19 +1,18 @@ -import { CreateRenderer, Renderer, RenderResult, RenderHook, RenderHookOptions } from '../types' -import { ResultContainer } from '../types/internal' +import { CreateRenderer, Renderer, RenderResult, RenderHookOptions } from '../types' import { asyncUtils } from './asyncUtils' import { cleanup, addCleanup, removeCleanup } from './cleanup' -function resultContainer(): ResultContainer { +function resultContainer() { const results: Array<{ value?: TValue; error?: Error }> = [] const resolvers: Array<() => void> = [] const result: RenderResult = { get all() { - return results.map(({ value, error }) => error ?? value) + return results.map(({ value, error }) => error ?? (value as TValue)) }, get current() { - const { value, error } = results[results.length - 1] + const { value, error } = results[results.length - 1] ?? {} if (error) { throw error } @@ -43,12 +42,12 @@ function resultContainer(): ResultContainer { function createRenderHook< TProps, TResult, - TOptions extends object, + TRendererOptions extends object, TRenderer extends Renderer ->(createRenderer: CreateRenderer) { - const renderHook: RenderHook = ( - callback, - options = {} as RenderHookOptions +>(createRenderer: CreateRenderer) { + const renderHook = ( + callback: (props: TProps) => TResult, + options = {} as RenderHookOptions & TRendererOptions ) => { const { result, setValue, setError, addResolver } = resultContainer() const renderProps = { callback, setValue, setError } @@ -79,11 +78,6 @@ function createRenderHook< } } - // If the function name does not get used before it is returned, - // it's name is removed by babel-plugin-minify-dead-code-elimination. - // This dummy usage works around that. - renderHook.name // eslint-disable-line @typescript-eslint/no-unused-expressions - return renderHook } diff --git a/src/dom/__tests__/suspenseHook.test.ts b/src/dom/__tests__/suspenseHook.test.ts index d3f19a1d..7ee7154c 100644 --- a/src/dom/__tests__/suspenseHook.test.ts +++ b/src/dom/__tests__/suspenseHook.test.ts @@ -46,4 +46,10 @@ describe('suspense hook tests', () => { expect(result.error).toEqual(new Error('Failed to fetch name')) }) + + test('should return undefined if current value is requested before suspension has resolved', async () => { + const { result } = renderHook(() => useFetchName(true)) + + expect(result.current).toBe(undefined) + }) }) diff --git a/src/dom/pure.ts b/src/dom/pure.ts index 2b2d1f32..a3ca74fd 100644 --- a/src/dom/pure.ts +++ b/src/dom/pure.ts @@ -1,10 +1,9 @@ import ReactDOM from 'react-dom' import { act } from 'react-dom/test-utils' -import { RendererProps } from '../types' -import { RendererOptions } from '../types/react' +import { RendererProps, RendererOptions } from '../types/react' -import { createRenderHook, cleanup, addCleanup, removeCleanup } from '../core' +import { createRenderHook } from '../core' import { createTestHarness } from '../helpers/createTestHarness' function createDomRenderer( @@ -39,7 +38,8 @@ function createDomRenderer( const renderHook = createRenderHook(createDomRenderer) -export { renderHook, act, cleanup, addCleanup, removeCleanup } +export { renderHook, act } + +export { cleanup, addCleanup, removeCleanup } from '../core' -export * from '../types' export * from '../types/react' diff --git a/src/helpers/createTestHarness.tsx b/src/helpers/createTestHarness.tsx index d50472d2..2a276107 100644 --- a/src/helpers/createTestHarness.tsx +++ b/src/helpers/createTestHarness.tsx @@ -1,7 +1,6 @@ import React, { Suspense } from 'react' -import { RendererProps } from '../types' -import { WrapperComponent } from '../types/react' +import { RendererProps, WrapperComponent } from '../types/react' import { isPromise } from './promises' @@ -40,11 +39,6 @@ function createTestHarness( return component } - // If the function name does not get used before it is returned, - // it's name is removed by babel-plugin-minify-dead-code-elimination. - // This dummy usage works around that. - testHarness.name // eslint-disable-line @typescript-eslint/no-unused-expressions - return testHarness } diff --git a/src/native/__tests__/suspenseHook.test.ts b/src/native/__tests__/suspenseHook.test.ts index d3f19a1d..7ee7154c 100644 --- a/src/native/__tests__/suspenseHook.test.ts +++ b/src/native/__tests__/suspenseHook.test.ts @@ -46,4 +46,10 @@ describe('suspense hook tests', () => { expect(result.error).toEqual(new Error('Failed to fetch name')) }) + + test('should return undefined if current value is requested before suspension has resolved', async () => { + const { result } = renderHook(() => useFetchName(true)) + + expect(result.current).toBe(undefined) + }) }) diff --git a/src/native/pure.ts b/src/native/pure.ts index b343bb66..959024d6 100644 --- a/src/native/pure.ts +++ b/src/native/pure.ts @@ -1,9 +1,8 @@ import { act, create, ReactTestRenderer } from 'react-test-renderer' -import { RendererProps } from '../types' -import { RendererOptions } from '../types/react' +import { RendererProps, RendererOptions } from '../types/react' -import { createRenderHook, cleanup, addCleanup, removeCleanup } from '../core' +import { createRenderHook } from '../core' import { createTestHarness } from '../helpers/createTestHarness' function createNativeRenderer( @@ -36,7 +35,8 @@ function createNativeRenderer( const renderHook = createRenderHook(createNativeRenderer) -export { renderHook, act, cleanup, addCleanup, removeCleanup } +export { renderHook, act } + +export { cleanup, addCleanup, removeCleanup } from '../core' -export * from '../types' export * from '../types/react' diff --git a/src/pure.ts b/src/pure.ts index d19cd1a7..699c7c10 100644 --- a/src/pure.ts +++ b/src/pure.ts @@ -36,5 +36,4 @@ const { renderHook, act, cleanup, addCleanup, removeCleanup } = getRenderer() export { renderHook, act, cleanup, addCleanup, removeCleanup } -export * from './types' export * from './types/react' diff --git a/src/server/pure.ts b/src/server/pure.ts index ae7f21d0..b58a44a1 100644 --- a/src/server/pure.ts +++ b/src/server/pure.ts @@ -2,10 +2,9 @@ import ReactDOMServer from 'react-dom/server' import ReactDOM from 'react-dom' import { act } from 'react-dom/test-utils' -import { RendererProps } from '../types' -import { RendererOptions } from '../types/react' +import { RendererProps, RendererOptions } from '../types/react' -import { createRenderHook, cleanup, addCleanup, removeCleanup } from '../core' +import { createRenderHook } from '../core' import { createTestHarness } from '../helpers/createTestHarness' function createServerRenderer( @@ -60,7 +59,8 @@ function createServerRenderer( const renderHook = createRenderHook(createServerRenderer) -export { renderHook, act, cleanup, addCleanup, removeCleanup } +export { renderHook, act } + +export { cleanup, addCleanup, removeCleanup } from '../core' -export * from '../types' export * from '../types/react' diff --git a/src/types/index.ts b/src/types/index.ts index e35b64b9..cdfe5aac 100644 --- a/src/types/index.ts +++ b/src/types/index.ts @@ -11,13 +11,15 @@ export type RendererProps = { setValue: (value: TResult) => void } -export type CreateRenderer> = ( - props: RendererProps, - options: TOptions -) => TRenderer +export type CreateRenderer< + TProps, + TResult, + TRendererOptions extends object, + TRenderer extends Renderer +> = (props: RendererProps, options: TRendererOptions) => TRenderer export type RenderResult = { - readonly all: (TValue | Error | undefined)[] + readonly all: Array readonly current: TValue readonly error?: Error } @@ -47,23 +49,13 @@ export type RenderHookResult< Omit> & AsyncUtils -export type RenderHookOptions = TOptions & { +export type RenderHookOptions = { initialProps?: TProps } -export interface RenderHook< - TProps, - TResult, - TOptions extends object, - TRenderer extends Renderer = Renderer -> { - ( - callback: (props: TProps) => TResult, - options?: RenderHookOptions - ): RenderHookResult -} - -export interface Act { +export type Act = { (callback: () => void | undefined): void (callback: () => Promise): Promise } + +export type CleanupCallback = () => Promise | void diff --git a/src/types/internal.ts b/src/types/internal.ts deleted file mode 100644 index 324e8268..00000000 --- a/src/types/internal.ts +++ /dev/null @@ -1,8 +0,0 @@ -import { RenderResult } from '.' - -export type ResultContainer = { - result: RenderResult - addResolver: (resolver: () => void) => void - setValue: (val: TValue) => void - setError: (error: Error) => void -} diff --git a/src/types/react.ts b/src/types/react.ts index 6b387281..56e2798a 100644 --- a/src/types/react.ts +++ b/src/types/react.ts @@ -1,6 +1,11 @@ import { ComponentType } from 'react' -import { RenderHookOptions, RenderHookResult, Act } from '.' +import { + RenderHookOptions as BaseRenderHookOptions, + RenderHookResult, + Act, + CleanupCallback +} from '.' export type WrapperComponent = ComponentType @@ -8,13 +13,19 @@ export type RendererOptions = { wrapper?: WrapperComponent } +export type RenderHookOptions = BaseRenderHookOptions & { + wrapper?: WrapperComponent +} + export type ReactHooksRenderer = { renderHook: ( callback: (props: TProps) => TResult, - options?: RenderHookOptions> + options?: RenderHookOptions ) => RenderHookResult act: Act cleanup: () => void - addCleanup: (callback: () => Promise | void) => () => void - removeCleanup: (callback: () => Promise | void) => void + addCleanup: (callback: CleanupCallback) => () => void + removeCleanup: (callback: CleanupCallback) => void } + +export * from '.'