Skip to content

Commit

Permalink
fix: fixed potential error when hook suspends and current result is a…
Browse files Browse the repository at this point in the history
…ccessed

BREAKING CHANGE: Adjust types so that react renderer exports don't required extra generic parameter
  • Loading branch information
mpeyper committed Jan 11, 2021
1 parent 43891e1 commit dc21e59
Show file tree
Hide file tree
Showing 14 changed files with 110 additions and 73 deletions.
2 changes: 1 addition & 1 deletion package.json
Expand Up @@ -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",
Expand Down
41 changes: 41 additions & 0 deletions 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))
})
})
8 changes: 5 additions & 3 deletions src/core/cleanup.ts
@@ -1,4 +1,6 @@
let cleanupCallbacks: (() => Promise<void> | void)[] = []
import { CleanupCallback } from '../types'

let cleanupCallbacks: Array<CleanupCallback> = []

async function cleanup() {
for (const callback of cleanupCallbacks) {
Expand All @@ -7,12 +9,12 @@ async function cleanup() {
cleanupCallbacks = []
}

function addCleanup(callback: () => Promise<void> | void) {
function addCleanup(callback: CleanupCallback) {
cleanupCallbacks = [callback, ...cleanupCallbacks]
return () => removeCleanup(callback)
}

function removeCleanup(callback: () => Promise<void> | void) {
function removeCleanup(callback: CleanupCallback) {
cleanupCallbacks = cleanupCallbacks.filter((cb) => cb !== callback)
}

Expand Down
24 changes: 9 additions & 15 deletions 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<TValue>(): ResultContainer<TValue> {
function resultContainer<TValue>() {
const results: Array<{ value?: TValue; error?: Error }> = []
const resolvers: Array<() => void> = []

const result: RenderResult<TValue> = {
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
}
Expand Down Expand Up @@ -43,12 +42,12 @@ function resultContainer<TValue>(): ResultContainer<TValue> {
function createRenderHook<
TProps,
TResult,
TOptions extends object,
TRendererOptions extends object,
TRenderer extends Renderer<TProps>
>(createRenderer: CreateRenderer<TProps, TResult, TOptions, TRenderer>) {
const renderHook: RenderHook<TProps, TResult, TOptions, TRenderer> = (
callback,
options = {} as RenderHookOptions<TProps, TOptions>
>(createRenderer: CreateRenderer<TProps, TResult, TRendererOptions, TRenderer>) {
const renderHook = (
callback: (props: TProps) => TResult,
options = {} as RenderHookOptions<TProps> & TRendererOptions
) => {
const { result, setValue, setError, addResolver } = resultContainer<TResult>()
const renderProps = { callback, setValue, setError }
Expand Down Expand Up @@ -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
}

Expand Down
6 changes: 6 additions & 0 deletions src/dom/__tests__/suspenseHook.test.ts
Expand Up @@ -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)

This comment has been minimized.

Copy link
@ntucker

ntucker Jan 24, 2021

Contributor
  • This is a breaking change (84 tests broken)
  • Undocumented in this release
  • Broken since 3.6
  • 3.6 release notes doesn't even mention this change

This comment has been minimized.

Copy link
@ntucker

ntucker Jan 24, 2021

Contributor

    TypeError: Cannot destructure property 'error' of 'results[(results.length - 1)]' as it is undefined.

      407 |     );
      408 |     expect(result.current).toBeUndefined();
    > 409 |     expect(result.error).toBe(null);```
    
    Hmm, never fixed error
})
})
10 changes: 5 additions & 5 deletions 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<TProps, TResult>(
Expand Down Expand Up @@ -39,7 +38,8 @@ function createDomRenderer<TProps, TResult>(

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'
8 changes: 1 addition & 7 deletions 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'

Expand Down Expand Up @@ -40,11 +39,6 @@ function createTestHarness<TProps, TResult>(
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
}

Expand Down
6 changes: 6 additions & 0 deletions src/native/__tests__/suspenseHook.test.ts
Expand Up @@ -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)
})
})
10 changes: 5 additions & 5 deletions 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<TProps, TResult>(
Expand Down Expand Up @@ -36,7 +35,8 @@ function createNativeRenderer<TProps, TResult>(

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'
1 change: 0 additions & 1 deletion src/pure.ts
Expand Up @@ -36,5 +36,4 @@ const { renderHook, act, cleanup, addCleanup, removeCleanup } = getRenderer()

export { renderHook, act, cleanup, addCleanup, removeCleanup }

export * from './types'
export * from './types/react'
10 changes: 5 additions & 5 deletions src/server/pure.ts
Expand Up @@ -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<TProps, TResult>(
Expand Down Expand Up @@ -60,7 +59,8 @@ function createServerRenderer<TProps, TResult>(

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'
30 changes: 11 additions & 19 deletions src/types/index.ts
Expand Up @@ -11,13 +11,15 @@ export type RendererProps<TProps, TResult> = {
setValue: (value: TResult) => void
}

export type CreateRenderer<TProps, TResult, TOptions, TRenderer extends Renderer<TProps>> = (
props: RendererProps<TProps, TResult>,
options: TOptions
) => TRenderer
export type CreateRenderer<
TProps,
TResult,
TRendererOptions extends object,
TRenderer extends Renderer<TProps>
> = (props: RendererProps<TProps, TResult>, options: TRendererOptions) => TRenderer

export type RenderResult<TValue> = {
readonly all: (TValue | Error | undefined)[]
readonly all: Array<TValue | Error>
readonly current: TValue
readonly error?: Error
}
Expand Down Expand Up @@ -47,23 +49,13 @@ export type RenderHookResult<
Omit<TRenderer, keyof Renderer<TProps>> &
AsyncUtils

export type RenderHookOptions<TProps, TOptions extends object> = TOptions & {
export type RenderHookOptions<TProps> = {
initialProps?: TProps
}

export interface RenderHook<
TProps,
TResult,
TOptions extends object,
TRenderer extends Renderer<TProps> = Renderer<TProps>
> {
(
callback: (props: TProps) => TResult,
options?: RenderHookOptions<TProps, TOptions>
): RenderHookResult<TProps, TResult, TRenderer>
}

export interface Act {
export type Act = {
(callback: () => void | undefined): void
(callback: () => Promise<void | undefined>): Promise<undefined>
}

export type CleanupCallback = () => Promise<void> | void
8 changes: 0 additions & 8 deletions src/types/internal.ts

This file was deleted.

19 changes: 15 additions & 4 deletions src/types/react.ts
@@ -1,20 +1,31 @@
import { ComponentType } from 'react'

import { RenderHookOptions, RenderHookResult, Act } from '.'
import {
RenderHookOptions as BaseRenderHookOptions,
RenderHookResult,
Act,
CleanupCallback
} from '.'

export type WrapperComponent<TProps> = ComponentType<TProps>

export type RendererOptions<TProps> = {
wrapper?: WrapperComponent<TProps>
}

export type RenderHookOptions<TProps> = BaseRenderHookOptions<TProps> & {
wrapper?: WrapperComponent<TProps>
}

export type ReactHooksRenderer = {
renderHook: <TProps, TResult>(
callback: (props: TProps) => TResult,
options?: RenderHookOptions<TProps, RendererOptions<TProps>>
options?: RenderHookOptions<TProps>
) => RenderHookResult<TProps, TResult>
act: Act
cleanup: () => void
addCleanup: (callback: () => Promise<void> | void) => () => void
removeCleanup: (callback: () => Promise<void> | void) => void
addCleanup: (callback: CleanupCallback) => () => void
removeCleanup: (callback: CleanupCallback) => void
}

export * from '.'

0 comments on commit dc21e59

Please sign in to comment.