unusual event handling in TextInput "_onClearInput" breaks fully controlled components and requires unsustainable workarounds

[elliot-wilson]

Component/Module

TextInput

Screenshots (if applicable)

Screen.Recording.2024-10-22.at.1.07.05.PM.mov

Reproduction Steps

import { TextInput } from "@narmi/design_system";
import { useState } from "react";

export function SearchBar() {
  const [searchTerm, setSearchTerm] = useState("");

  return (
    <TextInput
      value={searchTerm}
      label="Search"
      startIcon="search"
      onChange={(e: React.ChangeEvent<HTMLInputElement>) => setSearchTerm(e.target.value)}
      showClearButton
    />
  );
}

Expected Result

Clicking the "x" clear button sets the value to "".

Actual Result

Clicking the "x" clear button sets the value to undefined.

Additional Details / Context

Introduced by #953

If you use the showClearButton prop, the TextInput will call your onChange if you click the "x" IconButton:

design_system/src/TextInput/index.js

Lines 42 to 51 in 3136f79

	function _onChange(e) {
	if (onChange) {
	onChange(e);
	}
	setInputValue(e.target.value);
	}
	function _onClearInput(e) {
	_onChange(e);
	setInputValue("");
	}

This is problematic, because an onChange handler should be expecting an HTML change event, whereas an onClick action will trigger an HTML click event. For the click event, event.target.value will be undefined. That is, _onClearInput is passing the wrong kind of event to _onChange; it only looks like it's working because, in some cases, having an event target value of undefined is pretty close to an event target value of `"``. But not always!

There are two patterns that people seem to be using to avoid this:

1. Using an un-controlled component.

https://github.com/narmi/banking/blob/e65066c412f6ffecff011bac153ae8ed55b69ed1/staff/src/components/ach_manager/AchManagerFilters.tsx#L21-L35

This might be fine in simple cases, but we can't assume that every TextInput will be uncontrolled.

2. Guarding against an undefined target value, even though common sense says it should be truthy:

https://github.com/narmi/banking/blob/e24423e28ffd4876aa795af4077ff15aa120ccef/staff/src/pages/marketing/CampaignList.tsx#L98-L108

This code is not sustainable, because it just looks unnecessary from a TypeScript perspective. A ChangeEvent<HTMLInputElmenet> should never have an undefined target value. Someone will eventually "fix" it during a code cleanup.

Also, from a code maintenance perspective, we should not expect developers to write onChange callbacks that can handle either a change event or a click event. Doing so is unconventional in a way that will assuredly trip up future engineers.

Proposed solution

I'm not sure what the best approach is! Somehow synthesizing an onChange event with a target value "" feels wrong. It's probably better to require a clearInput prop where the consumer can supply a callback that -- 99%+ percent of the time -- will explicitly clear the value "".