chore(usecontrolledoruncontrolled): emulate React's uncontrolled warn…

… if value is set without onChange or readOnly
react95-io · Sep 10, 2022 · 2a0d9ba · 2a0d9ba
1 parent 2177c67
commit 2a0d9ba
Show file tree

Hide file tree

Showing 7 changed files with 83 additions and 32 deletions.
diff --git a/src/Checkbox/Checkbox.tsx b/src/Checkbox/Checkbox.tsx
@@ -207,8 +207,10 @@ const Checkbox = forwardRef<HTMLInputElement, CheckboxProps>(
     ref
   ) => {
     const [state, setState] = useControlledOrUncontrolled({
-      value: checked,
-      defaultValue: defaultChecked
+      defaultValue: defaultChecked,
+      onChange,
+      readOnly: otherProps.readOnly ?? disabled,
+      value: checked
     });
 
     const handleChange = useCallback(

diff --git a/src/ColorInput/ColorInput.tsx b/src/ColorInput/ColorInput.tsx
@@ -128,8 +128,10 @@ const ColorInput = forwardRef<HTMLInputElement, ColorInputProps>(
     ref
   ) => {
     const [valueDerived, setValueState] = useControlledOrUncontrolled({
-      value,
-      defaultValue
+      defaultValue,
+      onChange,
+      readOnly: otherProps.readOnly ?? disabled,
+      value
     });
 
     const handleChange = (e: React.ChangeEvent<HTMLInputElement>) => {

diff --git a/src/NumberInput/NumberInput.tsx b/src/NumberInput/NumberInput.tsx
@@ -14,6 +14,7 @@ type NumberInputProps = {
   disabled?: boolean;
   max?: number;
   min?: number;
+  readOnly?: boolean;
   step?: number;
   onChange?: (newValue: number) => void;
   style?: React.CSSProperties;
@@ -105,6 +106,7 @@ const NumberInput = forwardRef<HTMLInputElement, NumberInputProps>(
       max,
       min,
       onChange,
+      readOnly,
       step = 1,
       style,
       value,
@@ -114,8 +116,10 @@ const NumberInput = forwardRef<HTMLInputElement, NumberInputProps>(
     ref
   ) => {
     const [valueDerived, setValueState] = useControlledOrUncontrolled({
-      value,
-      defaultValue
+      defaultValue,
+      onChange,
+      readOnly,
+      value
     });
 
     const handleInputChange = useCallback(
@@ -169,6 +173,7 @@ const NumberInput = forwardRef<HTMLInputElement, NumberInputProps>(
           onChange={handleInputChange}
           disabled={disabled}
           type='number'
+          readOnly={readOnly}
           ref={ref}
           fullWidth
           onBlur={onBlur}
@@ -177,15 +182,15 @@ const NumberInput = forwardRef<HTMLInputElement, NumberInputProps>(
           <StyledButton
             data-testid='increment'
             variant={variant}
-            disabled={disabled}
+            disabled={disabled || readOnly}
             onClick={stepUp}
           >
             <StyledButtonIcon invert />
           </StyledButton>
           <StyledButton
             data-testid='decrement'
             variant={variant}
-            disabled={disabled}
+            disabled={disabled || readOnly}
             onClick={stepDown}
           >
             <StyledButtonIcon />

diff --git a/src/Slider/Slider.spec.tsx b/src/Slider/Slider.spec.tsx
@@ -14,6 +14,20 @@ function createTouches(
 }
 
 describe('<Slider />', () => {
+  beforeAll(() => {
+    jest
+      .spyOn(HTMLElement.prototype, 'getBoundingClientRect')
+      .mockImplementation(
+        () =>
+          ({
+            width: 100,
+            height: 20,
+            bottom: 20,
+            left: 0
+          } as DOMRect)
+      );
+  });
+
   it('should call handlers', () => {
     const handleChange = jest.fn();
     const handleChangeCommitted = jest.fn();
@@ -76,7 +90,7 @@ describe('<Slider />', () => {
   });
 
   it('defaults to horizontal orientation', () => {
-    const { getByRole } = renderWithTheme(<Slider value={0} />);
+    const { getByRole } = renderWithTheme(<Slider defaultValue={0} />);
 
     expect(getByRole('slider')).toHaveAttribute(
       'aria-orientation',
@@ -86,7 +100,7 @@ describe('<Slider />', () => {
   it('should forward mouseDown', () => {
     const handleMouseDown = jest.fn();
     const { container } = renderWithTheme(
-      <Slider onMouseDown={handleMouseDown} value={0} />
+      <Slider onMouseDown={handleMouseDown} defaultValue={0} />
     );
     const slider = container.firstElementChild as HTMLElement;
     fireEvent.mouseDown(slider);
@@ -103,13 +117,6 @@ describe('<Slider />', () => {
       );
       const slider = container.firstElementChild as HTMLElement;
       // mocking containers size
-      slider.getBoundingClientRect = () =>
-        ({
-          width: 100,
-          height: 20,
-          bottom: 20,
-          left: 0
-        } as DOMRect);
       const thumb = getByRole('slider');
 
       fireEvent.touchStart(
@@ -292,7 +299,7 @@ describe('<Slider />', () => {
   describe('prop: orientation', () => {
     it('when vertical, should render with aria-orientation attribute set to "vertical" ', () => {
       const { getByRole } = renderWithTheme(
-        <Slider orientation='vertical' value={0} />
+        <Slider orientation='vertical' defaultValue={0} />
       );
 
       expect(getByRole('slider')).toHaveAttribute(
@@ -314,13 +321,17 @@ describe('<Slider />', () => {
       const slider = container.firstElementChild as HTMLElement;
 
       // mocking containers size
-      slider.getBoundingClientRect = () =>
-        ({
-          width: 20,
-          height: 100,
-          bottom: 100,
-          left: 0
-        } as DOMRect);
+      jest
+        .spyOn(HTMLElement.prototype, 'getBoundingClientRect')
+        .mockImplementation(
+          () =>
+            ({
+              width: 20,
+              height: 100,
+              bottom: 100,
+              left: 0
+            } as DOMRect)
+        );
 
       fireEvent.touchStart(
         slider,

diff --git a/src/Slider/Slider.tsx b/src/Slider/Slider.tsx
@@ -313,9 +313,10 @@ const Slider = forwardRef<HTMLDivElement, SliderProps>(
   ) => {
     const Groove = variant === 'flat' ? StyledFlatGroove : StyledGroove;
     const vertical = orientation === 'vertical';
-    const [valueDerived, setValueState] = useControlledOrUncontrolled({
-      value,
-      defaultValue: defaultValue ?? min
+    const [valueDerived = min, setValueState] = useControlledOrUncontrolled({
+      defaultValue,
+      onChange: onChange ?? onChangeCommitted,
+      value
     });
 
     const {

diff --git a/src/TreeView/TreeView.tsx b/src/TreeView/TreeView.tsx
@@ -321,13 +321,19 @@ function TreeInner<T>(
   ref: React.ForwardedRef<HTMLUListElement>
 ) {
   const [expandedInternal, setExpandedInternal] = useControlledOrUncontrolled({
+    defaultValue: defaultExpanded,
+    onChange: onNodeToggle,
+    onChangePropName: 'onNodeToggle',
     value: expanded,
-    defaultValue: defaultExpanded
+    valuePropName: 'expanded'
   });
 
   const [selectedInternal, setSelectedInternal] = useControlledOrUncontrolled({
+    defaultValue: defaultSelected,
+    onChange: onNodeSelect,
+    onChangePropName: 'onNodeSelect',
     value: selected,
-    defaultValue: defaultSelected
+    valuePropName: 'selected'
   });
 
   const toggleMenu = useCallback(

diff --git a/src/common/hooks/useControlledOrUncontrolled.ts b/src/common/hooks/useControlledOrUncontrolled.ts
@@ -1,11 +1,20 @@
 import React, { useState, useCallback } from 'react';
 
 export default function useControlledOrUncontrolled<T>({
+  defaultValue,
+  onChange,
+  onChangePropName = 'onChange',
+  readOnly,
   value,
-  defaultValue
+  valuePropName = 'value'
 }: {
-  value: T | undefined;
   defaultValue: T;
+  // eslint-disable-next-line @typescript-eslint/no-explicit-any
+  onChange?: (...args: any[]) => void;
+  onChangePropName?: string;
+  readOnly?: boolean;
+  value: T | undefined;
+  valuePropName?: string;
 }): [T, (newValue: React.SetStateAction<T>) => void] {
   const isControlled = value !== undefined;
   const [controlledValue, setControlledValue] = useState(defaultValue);
@@ -17,5 +26,20 @@ export default function useControlledOrUncontrolled<T>({
     },
     [isControlled]
   );
+
+  // Because we provide `onChange` even to uncontrolled components, React's
+  // default uncontrolled warning must be reimplemented. This also deals with
+  // props that are different from `value`.
+  if (isControlled && typeof onChange !== 'function' && !readOnly) {
+    const message = `Warning: You provided a \`${valuePropName}\` prop to a component without an \`${onChangePropName}\` handler.${
+      valuePropName === 'value'
+        ? `This will render a read-only field. If the field should be mutable use \`defaultValue\`. Otherwise, set either \`${onChangePropName}\` or \`readOnly\`.`
+        : `This breaks the component state. You must provide an \`${onChangePropName}\` function that updates \`${valuePropName}\`.`
+    }`;
+
+    // eslint-disable-next-line no-console
+    console.warn(message);
+  }
+
   return [isControlled ? value : controlledValue, handleChangeIfUncontrolled];
 }