From 1127a55a7696157e276bf0d167f18ee1df53c270 Mon Sep 17 00:00:00 2001 From: Jordan Pittman Date: Mon, 10 Oct 2022 10:41:15 -0400 Subject: [PATCH] Warn when changing `Combobox` between controlled and uncontrolled (#1878) --- .github/workflows/main.yml | 5 +- packages/@headlessui-react/CHANGELOG.md | 4 + .../src/components/combobox/combobox.test.tsx | 100 +++++++++++++++++- .../src/hooks/use-controllable.ts | 20 +++- .../src/test-utils/suppress-console-logs.ts | 13 +++ .../src/components/combobox/combobox.test.ts | 44 +++++++- 6 files changed, 177 insertions(+), 9 deletions(-) diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml index 365db64357..efac0d7cf0 100644 --- a/.github/workflows/main.yml +++ b/.github/workflows/main.yml @@ -1,8 +1,9 @@ name: CI on: - - push - - pull_request + push: + branches: [main] + pull_request: concurrency: group: ${{ github.workflow }}-${{ github.head_ref || github.run_id }} diff --git a/packages/@headlessui-react/CHANGELOG.md b/packages/@headlessui-react/CHANGELOG.md index 7de5d6d7db..6331be9bf6 100644 --- a/packages/@headlessui-react/CHANGELOG.md +++ b/packages/@headlessui-react/CHANGELOG.md @@ -12,6 +12,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Fix `` crash ([#1889](https://github.com/tailwindlabs/headlessui/pull/1889)) - Expose `close` function for `Menu` and `Menu.Item` components ([#1897](https://github.com/tailwindlabs/headlessui/pull/1897)) +### Added + +- Warn when changing components between controlled and uncontrolled ([#1878](https://github.com/tailwindlabs/headlessui/issues/1878)) + ## [1.7.3] - 2022-09-30 ### Fixed diff --git a/packages/@headlessui-react/src/components/combobox/combobox.test.tsx b/packages/@headlessui-react/src/components/combobox/combobox.test.tsx index 54c679482f..5f21d3553d 100644 --- a/packages/@headlessui-react/src/components/combobox/combobox.test.tsx +++ b/packages/@headlessui-react/src/components/combobox/combobox.test.tsx @@ -2,7 +2,7 @@ import React, { createElement, useState, useEffect } from 'react' import { render } from '@testing-library/react' import { Combobox } from './combobox' -import { suppressConsoleLogs } from '../../test-utils/suppress-console-logs' +import { mockingConsoleLogs, suppressConsoleLogs } from '../../test-utils/suppress-console-logs' import { click, focus, @@ -396,7 +396,7 @@ describe('Rendering', () => { 'selecting an option puts the value into Combobox.Input when displayValue is not provided', suppressConsoleLogs(async () => { function Example() { - let [value, setValue] = useState(undefined) + let [value, setValue] = useState(null) return ( @@ -430,7 +430,7 @@ describe('Rendering', () => { 'selecting an option puts the display value into Combobox.Input when displayValue is provided', suppressConsoleLogs(async () => { function Example() { - let [value, setValue] = useState(undefined) + let [value, setValue] = useState(null) return ( @@ -558,7 +558,7 @@ describe('Rendering', () => { 'should be possible to override the `type` on the input', suppressConsoleLogs(async () => { function Example() { - let [value, setValue] = useState(undefined) + let [value, setValue] = useState(null) return ( @@ -5155,7 +5155,7 @@ describe('Mouse interactions', () => { ) it( - 'should sync the input field correctly and reset it when resetting the value from outside', + 'should sync the input field correctly and reset it when resetting the value from outside (to null)', suppressConsoleLogs(async () => { function Example() { let [value, setValue] = useState('bob') @@ -5196,6 +5196,96 @@ describe('Mouse interactions', () => { }) ) + it( + 'should warn when changing the combobox from uncontrolled to controlled', + mockingConsoleLogs(async (spy) => { + function Example() { + let [value, setValue] = useState(undefined) + + return ( + <> + + + Trigger + + alice + bob + charlie + + + + + ) + } + + // Render a uncontrolled combobox + render() + + // Change to an controlled combobox + await click(getByText('to controlled')) + + // Make sure we get a warning + expect(spy).toBeCalledTimes(1) + expect(spy.mock.calls.map((args) => args[0])).toEqual([ + 'A component is changing from uncontrolled to controlled. This may be caused by the value changing from undefined to a defined value, which should not happen.', + ]) + + // Render a fresh uncontrolled combobox + render() + + // Change to an controlled combobox + await click(getByText('to controlled')) + + // We shouldn't have gotten another warning as we do not want to warn on every render + expect(spy).toBeCalledTimes(1) + }) + ) + + it( + 'should warn when changing the combobox from controlled to uncontrolled', + mockingConsoleLogs(async (spy) => { + function Example() { + let [value, setValue] = useState('bob') + + return ( + <> + + + Trigger + + alice + bob + charlie + + + + + ) + } + + // Render a controlled combobox + render() + + // Change to an uncontrolled combobox + await click(getByText('to uncontrolled')) + + // Make sure we get a warning + expect(spy).toBeCalledTimes(1) + expect(spy.mock.calls.map((args) => args[0])).toEqual([ + 'A component is changing from controlled to uncontrolled. This may be caused by the value changing from a defined value to undefined, which should not happen.', + ]) + + // Render a fresh controlled combobox + render() + + // Change to an uncontrolled combobox + await click(getByText('to uncontrolled')) + + // We shouldn't have gotten another warning as we do not want to warn on every render + expect(spy).toBeCalledTimes(1) + }) + ) + it( 'should sync the input field correctly and reset it when resetting the value from outside (when using displayValue)', suppressConsoleLogs(async () => { diff --git a/packages/@headlessui-react/src/hooks/use-controllable.ts b/packages/@headlessui-react/src/hooks/use-controllable.ts index 17da50670d..d3dfd56f9c 100644 --- a/packages/@headlessui-react/src/hooks/use-controllable.ts +++ b/packages/@headlessui-react/src/hooks/use-controllable.ts @@ -1,4 +1,4 @@ -import { useState } from 'react' +import { useRef, useState } from 'react' import { useEvent } from './use-event' export function useControllable( @@ -7,7 +7,25 @@ export function useControllable( defaultValue?: T ) { let [internalValue, setInternalValue] = useState(defaultValue) + let isControlled = controlledValue !== undefined + let wasControlled = useRef(isControlled) + let didWarnOnUncontrolledToControlled = useRef(false) + let didWarnOnControlledToUncontrolled = useRef(false) + + if (isControlled && !wasControlled.current && !didWarnOnUncontrolledToControlled.current) { + didWarnOnUncontrolledToControlled.current = true + wasControlled.current = isControlled + console.error( + 'A component is changing from uncontrolled to controlled. This may be caused by the value changing from undefined to a defined value, which should not happen.' + ) + } else if (!isControlled && wasControlled.current && !didWarnOnControlledToUncontrolled.current) { + didWarnOnControlledToUncontrolled.current = true + wasControlled.current = isControlled + console.error( + 'A component is changing from controlled to uncontrolled. This may be caused by the value changing from a defined value to undefined, which should not happen.' + ) + } return [ (isControlled ? controlledValue : internalValue)!, diff --git a/packages/@headlessui-react/src/test-utils/suppress-console-logs.ts b/packages/@headlessui-react/src/test-utils/suppress-console-logs.ts index 0de4a0b4e3..de4d7b2fe0 100644 --- a/packages/@headlessui-react/src/test-utils/suppress-console-logs.ts +++ b/packages/@headlessui-react/src/test-utils/suppress-console-logs.ts @@ -15,3 +15,16 @@ export function suppressConsoleLogs( }).finally(() => spy.mockRestore()) } } + +export function mockingConsoleLogs( + cb: (spy: jest.SpyInstance, ...args: T) => unknown, + type: FunctionPropertyNames = 'error' +) { + return (...args: T) => { + let spy = jest.spyOn(globalThis.console, type).mockImplementation(jest.fn()) + + return new Promise((resolve, reject) => { + Promise.resolve(cb(spy, ...args)).then(resolve, reject) + }).finally(() => spy.mockRestore()) + } +} diff --git a/packages/@headlessui-vue/src/components/combobox/combobox.test.ts b/packages/@headlessui-vue/src/components/combobox/combobox.test.ts index 0bbf63e00e..9c8968d9d4 100644 --- a/packages/@headlessui-vue/src/components/combobox/combobox.test.ts +++ b/packages/@headlessui-vue/src/components/combobox/combobox.test.ts @@ -5381,7 +5381,7 @@ describe('Mouse interactions', () => { ) it( - 'should sync the input field correctly and reset it when resetting the value from outside', + 'should sync the input field correctly and reset it when resetting the value from outside (to null)', suppressConsoleLogs(async () => { renderTemplate({ template: html` @@ -5417,6 +5417,48 @@ describe('Mouse interactions', () => { }) ) + it( + 'should sync the input field correctly and reset it when resetting the value from outside (to undefined)', + suppressConsoleLogs(async () => { + renderTemplate({ + template: html` + + + Trigger + + alice + bob + charlie + + + + `, + setup: () => ({ value: ref('bob') }), + }) + + // Open combobox + await click(getComboboxButton()) + + // Verify the input has the selected value + expect(getComboboxInput()?.value).toBe('bob') + + // Override the input by typing something + await type(word('alice'), getComboboxInput()) + expect(getComboboxInput()?.value).toBe('alice') + + // Select the option + await press(Keys.ArrowUp) + await press(Keys.Enter) + expect(getComboboxInput()?.value).toBe('alice') + + // Reset from outside + await click(getByText('reset')) + + // Verify the input is reset correctly + expect(getComboboxInput()?.value).toBe('') + }) + ) + it( 'should sync the input field correctly and reset it when resetting the value from outside (when using displayValue)', suppressConsoleLogs(async () => {