From 753ba1647ef67fcefd63d79b11ce3ee52ee2a5d3 Mon Sep 17 00:00:00 2001 From: Adam Thompson Date: Thu, 24 Aug 2023 12:49:00 -0400 Subject: [PATCH 01/16] Extends `useControlledValue` to accept any type --- .changeset/soft-berries-obey.md | 5 +++++ .../src/useControlledValue/useControlledValue.ts | 11 +++++++---- 2 files changed, 12 insertions(+), 4 deletions(-) create mode 100644 .changeset/soft-berries-obey.md diff --git a/.changeset/soft-berries-obey.md b/.changeset/soft-berries-obey.md new file mode 100644 index 0000000000..f169b748be --- /dev/null +++ b/.changeset/soft-berries-obey.md @@ -0,0 +1,5 @@ +--- +'@leafygreen-ui/hooks': minor +--- + +Extends `useControlledValue` to accept any type diff --git a/packages/hooks/src/useControlledValue/useControlledValue.ts b/packages/hooks/src/useControlledValue/useControlledValue.ts index 008235ee54..c1eacd848d 100644 --- a/packages/hooks/src/useControlledValue/useControlledValue.ts +++ b/packages/hooks/src/useControlledValue/useControlledValue.ts @@ -1,7 +1,7 @@ import { ChangeEventHandler, useEffect, useState } from 'react'; import isUndefined from 'lodash/isUndefined'; -interface ControlledValueReturnObject { +interface ControlledValueReturnObject { /** Whether the value is controlled */ isControlled: boolean; @@ -24,14 +24,17 @@ interface ControlledValueReturnObject { * Returns a {@link ControlledValueReturnObject} with the controlled or uncontrolled `value`, * `onChange` & `onClear` handlers, a `setInternalValue` setter, and a boolean `isControlled` */ -export const useControlledValue = ( +export const useControlledValue = ( controlledValue?: T, changeHandler?: ChangeEventHandler, -): ControlledValueReturnObject => { + defaultValue?: T, +): ControlledValueReturnObject => { const isControlled = !isUndefined(controlledValue); // Keep track of state internally, initializing it to the controlled value - const [value, setInternalValue] = useState(controlledValue ?? ('' as T)); + const [value, setInternalValue] = useState( + controlledValue ?? defaultValue, + ); // If the controlled value changes, update the internal state variable useEffect(() => { From c565f2638902fabe592895c0dcb9a5c1bb61eed6 Mon Sep 17 00:00:00 2001 From: Adam Thompson Date: Thu, 24 Aug 2023 15:16:06 -0400 Subject: [PATCH 02/16] Adds more explicit tests --- .../useControlledValue.spec.ts | 112 +++++++++++++----- 1 file changed, 83 insertions(+), 29 deletions(-) diff --git a/packages/hooks/src/useControlledValue/useControlledValue.spec.ts b/packages/hooks/src/useControlledValue/useControlledValue.spec.ts index 8ebc6f789a..035cee5cb5 100644 --- a/packages/hooks/src/useControlledValue/useControlledValue.spec.ts +++ b/packages/hooks/src/useControlledValue/useControlledValue.spec.ts @@ -1,42 +1,96 @@ -import { ChangeEvent } from 'react'; +import { ChangeEvent, ChangeEventHandler } from 'react'; import { act } from 'react-test-renderer'; import { renderHook } from '@testing-library/react-hooks'; import { useControlledValue } from './useControlledValue'; +const changeEventMock = { + target: { value: 'banana' }, +} as ChangeEvent; + describe('packages/lib/useControlledValue', () => { - test('with controlled component', async () => { - const value = 'apple'; - const handler = jest.fn(); - const { - result: { current }, - } = renderHook(() => useControlledValue(value as string, handler)); - expect(current.isControlled).toBe(true); - expect(current.value).toBe(value); - - act(() => { - current.handleChange({ target: { value: 'banana' } } as ChangeEvent); - current.setUncontrolledValue('banana'); + describe('with controlled component', () => { + test('calling with a value sets value and isControlled', () => { + const handler = jest.fn(); + const { result } = renderHook(v => useControlledValue(v, handler), { + initialProps: 'apple', + }); + expect(result.current.isControlled).toBe(true); + expect(result.current.value).toBe('apple'); + }); + + test('calling with a new value changes the value', () => { + const handler = jest.fn(); + const { result, rerender } = renderHook( + v => useControlledValue(v, handler), + { + initialProps: 'apple', + }, + ); + + expect(result.current.value).toBe('apple'); + + act(() => { + rerender('banana'); + }); + + expect(result.current.value).toBe('banana'); + }); + + test('provided handler should be called', () => { + const handler = jest.fn(); + const { result } = renderHook(v => useControlledValue(v, handler), { + initialProps: 'apple', + }); + + // simulate responding to an event + act(() => { + result.current.handleChange(changeEventMock); + }); + expect(handler).toHaveBeenCalledWith( + expect.objectContaining(changeEventMock), + ); + // value doesn't change unless we explicitly change it + expect(result.current.value).toBe('apple'); }); - expect(handler).toHaveBeenCalledWith( - expect.objectContaining({ target: { value: 'banana' } }), - ); - expect(current.value).toBe('apple'); }); - test('with uncontrolled component', async () => { - const value = undefined; - const handler = jest.fn(); - const { - result: { current }, - } = renderHook(() => useControlledValue(value, handler)); - expect(current.isControlled).toBe(false); - expect(current.value).toBe(''); - - act(() => { - current.handleChange({ target: { value: 'apple' } } as ChangeEvent); + describe('with uncontrolled component', () => { + test('calling without a value sets value and isControlled', () => { + const handler = jest.fn(); + const { result } = renderHook(v => useControlledValue(v, handler), { + initialProps: undefined, + }); + expect(result.current.isControlled).toBe(false); + expect(result.current.value).toBe(undefined); + }); + + test('calling setter updates value', () => { + const handler = jest.fn(); + const { result } = renderHook( + v => useControlledValue(v, handler), + { + initialProps: undefined, + }, + ); + + act(() => { + result.current.setUncontrolledValue('apple'); + }); + expect(result.current.value).toBe('apple'); + }); + + test('provided handler should be called', () => { + const handler = jest.fn(); + const { result } = renderHook(v => useControlledValue(v, handler), { + initialProps: undefined, + }); + + act(() => { + result.current.handleChange(changeEventMock); + }); expect(handler).toHaveBeenCalledWith( - expect.objectContaining({ target: { value: 'apple' } }), + expect.objectContaining(changeEventMock), ); }); }); From b61283585a257987408ccfa6b54b6ce23dd27d45 Mon Sep 17 00:00:00 2001 From: Adam Thompson Date: Thu, 24 Aug 2023 15:28:30 -0400 Subject: [PATCH 03/16] adds test component tests --- ...ue.spec.ts => useControlledValue.spec.tsx} | 80 +++++++++++++++++++ 1 file changed, 80 insertions(+) rename packages/hooks/src/useControlledValue/{useControlledValue.spec.ts => useControlledValue.spec.tsx} (51%) diff --git a/packages/hooks/src/useControlledValue/useControlledValue.spec.ts b/packages/hooks/src/useControlledValue/useControlledValue.spec.tsx similarity index 51% rename from packages/hooks/src/useControlledValue/useControlledValue.spec.ts rename to packages/hooks/src/useControlledValue/useControlledValue.spec.tsx index 035cee5cb5..bfff17fbb2 100644 --- a/packages/hooks/src/useControlledValue/useControlledValue.spec.ts +++ b/packages/hooks/src/useControlledValue/useControlledValue.spec.tsx @@ -1,6 +1,9 @@ +import React from 'react'; import { ChangeEvent, ChangeEventHandler } from 'react'; import { act } from 'react-test-renderer'; +import { render } from '@testing-library/react'; import { renderHook } from '@testing-library/react-hooks'; +import userEvent from '@testing-library/user-event'; import { useControlledValue } from './useControlledValue'; @@ -94,4 +97,81 @@ describe('packages/lib/useControlledValue', () => { ); }); }); + + describe('with test component', () => { + const TestComponent = ({ + valueProp, + handlerProp, + }: { + valueProp?: string; + handlerProp?: ChangeEventHandler; + }) => { + // eslint-disable-next-line react-hooks/rules-of-hooks + const { value, handleChange } = useControlledValue( + valueProp, + handlerProp, + ); + + return ; + }; + + describe('Controlled test component', () => { + test('initially renders with a value', () => { + const result = render(); + const input = result.getByTestId('test'); + expect(input).toHaveValue('apple'); + }); + + test('responds to value changes', () => { + const result = render(); + const input = result.getByTestId('test'); + result.rerender(); + expect(input).toHaveValue('banana'); + }); + + test('user interaction triggers handler', () => { + const handler = jest.fn(); + const result = render( + , + ); + const input = result.getByTestId('test'); + userEvent.type(input, 'b'); + expect(handler).toHaveBeenCalled(); + }); + + test('user interaction does not change the element value', () => { + const handler = jest.fn(); + const result = render( + , + ); + const input = result.getByTestId('test'); + userEvent.type(input, 'b'); + expect(input).toHaveValue('apple'); + }); + }); + + describe('Uncontrolled test component', () => { + test('initially renders without a value', () => { + const result = render(); + const input = result.getByTestId('test'); + expect(input).toHaveValue(''); + }); + + test('user interaction triggers handler', () => { + const handler = jest.fn(); + const result = render(); + const input = result.getByTestId('test'); + userEvent.type(input, 'b'); + expect(handler).toHaveBeenCalled(); + }); + + test('user interaction does not change the element value', () => { + const handler = jest.fn(); + const result = render(); + const input = result.getByTestId('test'); + userEvent.type(input, 'banana'); + expect(input).toHaveValue('banana'); + }); + }); + }); }); From f4e1c613075c85b707533d8831840073fbb7cf87 Mon Sep 17 00:00:00 2001 From: Adam Thompson Date: Thu, 24 Aug 2023 15:32:55 -0400 Subject: [PATCH 04/16] adds value tests --- .../useControlledValue.spec.tsx | 40 +++++++++++++++++++ 1 file changed, 40 insertions(+) diff --git a/packages/hooks/src/useControlledValue/useControlledValue.spec.tsx b/packages/hooks/src/useControlledValue/useControlledValue.spec.tsx index bfff17fbb2..8b2e85a018 100644 --- a/packages/hooks/src/useControlledValue/useControlledValue.spec.tsx +++ b/packages/hooks/src/useControlledValue/useControlledValue.spec.tsx @@ -56,6 +56,46 @@ describe('packages/lib/useControlledValue', () => { // value doesn't change unless we explicitly change it expect(result.current.value).toBe('apple'); }); + + describe('value types', () => { + test('accepts number values', () => { + const { result } = renderHook(v => useControlledValue(v), { + initialProps: 5, + }); + expect(result.current.value).toBe(5); + }); + + test('accepts boolean values', () => { + const { result } = renderHook(v => useControlledValue(v), { + initialProps: false, + }); + expect(result.current.value).toBe(false); + }); + + test('accepts array values', () => { + const arr = ['foo', 'bar']; + const { result } = renderHook(v => useControlledValue(v), { + initialProps: arr, + }); + expect(result.current.value).toBe(arr); + }); + + test('accepts object values', () => { + const obj = { foo: 'foo', bar: 'bar' }; + const { result } = renderHook(v => useControlledValue(v), { + initialProps: obj, + }); + expect(result.current.value).toBe(obj); + }); + + test('accepts date values', () => { + const date = new Date('2023-08-23'); + const { result } = renderHook(v => useControlledValue(v), { + initialProps: date, + }); + expect(result.current.value).toBe(date); + }); + }); }); describe('with uncontrolled component', () => { From bbb1832e65f3661f6f2dbc98706cf26844e2e53a Mon Sep 17 00:00:00 2001 From: Adam Thompson Date: Thu, 24 Aug 2023 15:51:20 -0400 Subject: [PATCH 05/16] isControlled never changes from initial render --- .changeset/soft-berries-obey.md | 3 +- .../useControlledValue.spec.tsx | 63 +++++++++++++------ .../useControlledValue/useControlledValue.ts | 6 +- 3 files changed, 50 insertions(+), 22 deletions(-) diff --git a/.changeset/soft-berries-obey.md b/.changeset/soft-berries-obey.md index f169b748be..8d0236cdfe 100644 --- a/.changeset/soft-berries-obey.md +++ b/.changeset/soft-berries-obey.md @@ -2,4 +2,5 @@ '@leafygreen-ui/hooks': minor --- -Extends `useControlledValue` to accept any type +- Extends `useControlledValue` to accept any type. +- The value of `isControlled` is now immutable after the first render diff --git a/packages/hooks/src/useControlledValue/useControlledValue.spec.tsx b/packages/hooks/src/useControlledValue/useControlledValue.spec.tsx index 8b2e85a018..faf8fc1b6f 100644 --- a/packages/hooks/src/useControlledValue/useControlledValue.spec.tsx +++ b/packages/hooks/src/useControlledValue/useControlledValue.spec.tsx @@ -14,8 +14,7 @@ const changeEventMock = { describe('packages/lib/useControlledValue', () => { describe('with controlled component', () => { test('calling with a value sets value and isControlled', () => { - const handler = jest.fn(); - const { result } = renderHook(v => useControlledValue(v, handler), { + const { result } = renderHook(v => useControlledValue(v), { initialProps: 'apple', }); expect(result.current.isControlled).toBe(true); @@ -23,13 +22,9 @@ describe('packages/lib/useControlledValue', () => { }); test('calling with a new value changes the value', () => { - const handler = jest.fn(); - const { result, rerender } = renderHook( - v => useControlledValue(v, handler), - { - initialProps: 'apple', - }, - ); + const { result, rerender } = renderHook(v => useControlledValue(v), { + initialProps: 'apple', + }); expect(result.current.value).toBe('apple'); @@ -40,7 +35,7 @@ describe('packages/lib/useControlledValue', () => { expect(result.current.value).toBe('banana'); }); - test('provided handler should be called', () => { + test('provided handler is called within returned hook handler', () => { const handler = jest.fn(); const { result } = renderHook(v => useControlledValue(v, handler), { initialProps: 'apple', @@ -57,6 +52,15 @@ describe('packages/lib/useControlledValue', () => { expect(result.current.value).toBe('apple'); }); + test('setting value to undefined should keep the component controlled', () => { + const { result, rerender } = renderHook(v => useControlledValue(v), { + initialProps: 'apple', + }); + expect(result.current.isControlled).toBe(true); + act(() => rerender(undefined)); + expect(result.current.isControlled).toBe(true); + }); + describe('value types', () => { test('accepts number values', () => { const { result } = renderHook(v => useControlledValue(v), { @@ -100,8 +104,7 @@ describe('packages/lib/useControlledValue', () => { describe('with uncontrolled component', () => { test('calling without a value sets value and isControlled', () => { - const handler = jest.fn(); - const { result } = renderHook(v => useControlledValue(v, handler), { + const { result } = renderHook(v => useControlledValue(v), { initialProps: undefined, }); expect(result.current.isControlled).toBe(false); @@ -109,13 +112,9 @@ describe('packages/lib/useControlledValue', () => { }); test('calling setter updates value', () => { - const handler = jest.fn(); - const { result } = renderHook( - v => useControlledValue(v, handler), - { - initialProps: undefined, - }, - ); + const { result } = renderHook(v => useControlledValue(v), { + initialProps: undefined, + }); act(() => { result.current.setUncontrolledValue('apple'); @@ -136,6 +135,32 @@ describe('packages/lib/useControlledValue', () => { expect.objectContaining(changeEventMock), ); }); + + test('calling the returned handler sets the value', () => { + const handler = jest.fn(); + const { result } = renderHook(v => useControlledValue(v, handler), { + initialProps: undefined, + }); + + act(() => { + result.current.handleChange(changeEventMock); + }); + expect(result.current.value).toBe('banana'); + }); + + test('changing value prop from initial undefined is ignored', () => { + const { result, rerender } = renderHook( + v => useControlledValue(v), + { + initialProps: undefined, + }, + ); + expect(result.current.isControlled).toBe(false); + // @ts-ignore - picking up renderHook.options types, not actual hook types + act(() => rerender('apple')); + expect(result.current.isControlled).toBe(false); + expect(result.current.value).toBe(undefined); + }); }); describe('with test component', () => { diff --git a/packages/hooks/src/useControlledValue/useControlledValue.ts b/packages/hooks/src/useControlledValue/useControlledValue.ts index 536879319f..e843f2f551 100644 --- a/packages/hooks/src/useControlledValue/useControlledValue.ts +++ b/packages/hooks/src/useControlledValue/useControlledValue.ts @@ -1,4 +1,4 @@ -import { ChangeEventHandler, useState } from 'react'; +import { ChangeEventHandler, useEffect, useMemo, useState } from 'react'; import isUndefined from 'lodash/isUndefined'; interface ControlledValueReturnObject { @@ -28,7 +28,9 @@ export const useControlledValue = ( controlledValue?: T, changeHandler?: ChangeEventHandler, ): ControlledValueReturnObject => { - const isControlled = !isUndefined(controlledValue); + // isControlled should only be computed once + // eslint-disable-next-line react-hooks/exhaustive-deps + const isControlled = useMemo(() => !isUndefined(controlledValue), []); // Keep track of the uncontrolled value state internally const [uncontrolledValue, setUncontrolledValue] = useState(); From af6cc4867690aeac0cf8f57618e4fb0819a4441c Mon Sep 17 00:00:00 2001 From: Adam Thompson Date: Thu, 24 Aug 2023 16:31:17 -0400 Subject: [PATCH 06/16] adds synthetic events --- .changeset/soft-berries-obey.md | 1 + packages/hooks/package.json | 1 + .../useControlledValue.spec.tsx | 108 +++++++++++++++--- .../useControlledValue/useControlledValue.ts | 30 ++++- packages/hooks/tsconfig.json | 6 +- 5 files changed, 130 insertions(+), 16 deletions(-) diff --git a/.changeset/soft-berries-obey.md b/.changeset/soft-berries-obey.md index 8d0236cdfe..b428fe5f29 100644 --- a/.changeset/soft-berries-obey.md +++ b/.changeset/soft-berries-obey.md @@ -3,4 +3,5 @@ --- - Extends `useControlledValue` to accept any type. +- Adds `updateValue` function in return value. This method triggers a synthetic event to update the value of a controlled or uncontrolled component. - The value of `isControlled` is now immutable after the first render diff --git a/packages/hooks/package.json b/packages/hooks/package.json index 27ca1b683d..e3f73c5d70 100644 --- a/packages/hooks/package.json +++ b/packages/hooks/package.json @@ -22,6 +22,7 @@ "access": "public" }, "dependencies": { + "@leafygreen-ui/lib": "^11.0.0", "lodash": "^4.17.21" }, "gitHead": "dd71a2d404218ccec2e657df9c0263dc1c15b9e0", diff --git a/packages/hooks/src/useControlledValue/useControlledValue.spec.tsx b/packages/hooks/src/useControlledValue/useControlledValue.spec.tsx index faf8fc1b6f..837c94aaa3 100644 --- a/packages/hooks/src/useControlledValue/useControlledValue.spec.tsx +++ b/packages/hooks/src/useControlledValue/useControlledValue.spec.tsx @@ -1,4 +1,4 @@ -import React from 'react'; +import React, { useRef } from 'react'; import { ChangeEvent, ChangeEventHandler } from 'react'; import { act } from 'react-test-renderer'; import { render } from '@testing-library/react'; @@ -11,6 +11,10 @@ const changeEventMock = { target: { value: 'banana' }, } as ChangeEvent; +const mutableRefMock = { + current: document.createElement('div'), +}; + describe('packages/lib/useControlledValue', () => { describe('with controlled component', () => { test('calling with a value sets value and isControlled', () => { @@ -61,6 +65,30 @@ describe('packages/lib/useControlledValue', () => { expect(result.current.isControlled).toBe(true); }); + test('setUncontrolledValue does nothing for controlled components', () => { + const { result } = renderHook(v => useControlledValue(v), { + initialProps: 'apple', + }); + act(() => { + result.current.setUncontrolledValue('banana'); + }); + expect(result.current.value).toBe('apple'); + }); + + test('updateValue triggers the provided handler', async () => { + const handler = jest.fn(); + + const { result } = renderHook(v => useControlledValue(v, handler), { + initialProps: 'apple', + }); + + await act(() => { + result.current.updateValue('banana', mutableRefMock); + }); + + expect(handler).toHaveBeenCalled(); + }); + describe('value types', () => { test('accepts number values', () => { const { result } = renderHook(v => useControlledValue(v), { @@ -111,15 +139,20 @@ describe('packages/lib/useControlledValue', () => { expect(result.current.value).toBe(undefined); }); - test('calling setter updates value', () => { - const { result } = renderHook(v => useControlledValue(v), { - initialProps: undefined, - }); + test('setUncontrolledValue updates value', () => { + const handler = jest.fn(); + const { result } = renderHook( + v => useControlledValue(v, handler), + { + initialProps: undefined, + }, + ); act(() => { result.current.setUncontrolledValue('apple'); }); expect(result.current.value).toBe('apple'); + expect(handler).not.toHaveBeenCalled(); }); test('provided handler should be called', () => { @@ -136,6 +169,23 @@ describe('packages/lib/useControlledValue', () => { ); }); + test('updateValue updates value & calls handler', () => { + const handler = jest.fn(); + const { result } = renderHook( + v => useControlledValue(v, handler), + { + initialProps: undefined, + }, + ); + + act(() => { + result.current.updateValue('banana', mutableRefMock); + }); + + expect(handler).toHaveBeenCalled(); + expect(result.current.value).toBe('banana'); + }); + test('calling the returned handler sets the value', () => { const handler = jest.fn(); const { result } = renderHook(v => useControlledValue(v, handler), { @@ -171,25 +221,39 @@ describe('packages/lib/useControlledValue', () => { valueProp?: string; handlerProp?: ChangeEventHandler; }) => { + const inputRef = useRef(null); // eslint-disable-next-line react-hooks/rules-of-hooks - const { value, handleChange } = useControlledValue( + const { value, handleChange, updateValue } = useControlledValue( valueProp, handlerProp, ); - return ; + return ( + <> + +