Warn when changing Combobox between controlled and uncontrolled (#1878)

Author: thecrypticace

.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 }}

packages/@headlessui-react/CHANGELOG.md

@@ -12,6 +12,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 - Fix `<Popover.Button as={Fragment} />` 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

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 (
             <Combobox value={value} onChange={setValue}>
@@ -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 (
             <Combobox value={value} onChange={setValue}>
@@ -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 (
             <Combobox value={value} onChange={setValue}>
@@ -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<string | null>('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<string | undefined>(undefined)
+
+        return (
+          <>
+            <Combobox value={value} onChange={setValue}>
+              <Combobox.Input onChange={NOOP} />
+              <Combobox.Button>Trigger</Combobox.Button>
+              <Combobox.Options>
+                <Combobox.Option value="alice">alice</Combobox.Option>
+                <Combobox.Option value="bob">bob</Combobox.Option>
+                <Combobox.Option value="charlie">charlie</Combobox.Option>
+              </Combobox.Options>
+            </Combobox>
+            <button onClick={() => setValue('bob')}>to controlled</button>
+          </>
+        )
+      }
+
+      // Render a uncontrolled combobox
+      render(<Example />)
+
+      // 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(<Example />)
+
+      // 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<string | undefined>('bob')
+
+        return (
+          <>
+            <Combobox value={value} onChange={setValue}>
+              <Combobox.Input onChange={NOOP} />
+              <Combobox.Button>Trigger</Combobox.Button>
+              <Combobox.Options>
+                <Combobox.Option value="alice">alice</Combobox.Option>
+                <Combobox.Option value="bob">bob</Combobox.Option>
+                <Combobox.Option value="charlie">charlie</Combobox.Option>
+              </Combobox.Options>
+            </Combobox>
+            <button onClick={() => setValue(undefined)}>to uncontrolled</button>
+          </>
+        )
+      }
+
+      // Render a controlled combobox
+      render(<Example />)
+
+      // 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(<Example />)
+
+      // 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 () => {

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<T>(
@@ -7,7 +7,25 @@ export function useControllable<T>(
   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)!,

packages/@headlessui-react/src/test-utils/suppress-console-logs.ts

@@ -15,3 +15,16 @@ export function suppressConsoleLogs<T extends unknown[]>(
     }).finally(() => spy.mockRestore())
   }
 }
+
+export function mockingConsoleLogs<T extends unknown[]>(
+  cb: (spy: jest.SpyInstance, ...args: T) => unknown,
+  type: FunctionPropertyNames<typeof globalThis.console> = 'error'
+) {
+  return (...args: T) => {
+    let spy = jest.spyOn(globalThis.console, type).mockImplementation(jest.fn())
+
+    return new Promise<unknown>((resolve, reject) => {
+      Promise.resolve(cb(spy, ...args)).then(resolve, reject)
+    }).finally(() => spy.mockRestore())
+  }
+}

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`
+          <Combobox v-model="value">
+            <ComboboxInput />
+            <ComboboxButton>Trigger</ComboboxButton>
+            <ComboboxOptions>
+              <ComboboxOption value="alice">alice</ComboboxOption>
+              <ComboboxOption value="bob">bob</ComboboxOption>
+              <ComboboxOption value="charlie">charlie</ComboboxOption>
+            </ComboboxOptions>
+          </Combobox>
+          <button @click="value = undefined">reset</button>
+        `,
+        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 () => {