should result props rather be specifically typed to avoid type collissions

[AndyOGo]

Im using useKeyboard which returns { keyboardProps }.
These keyboardProps are typed too generically with HTMLAttributes<HTMLElement>, because they only set onKeyDown and onKeyUp and all the other possibly collide if a component allows other types.

react-spectrum/packages/@react-aria/interactions/src/useKeyboard.ts

Lines 22 to 37 in f6c0660

	interface KeyboardResult {
	/** Props to spread onto the target element. */
	keyboardProps: HTMLAttributes<HTMLElement>
	}
	/**
	* Handles keyboard interactions for a focusable element.
	*/
	export function useKeyboard(props: KeyboardProps): KeyboardResult {
	return {
	keyboardProps: props.isDisabled ? {} : {
	onKeyDown: createEventHandler(props.onKeyDown),
	onKeyUp: createEventHandler(props.onKeyUp)
	}
	};
	}

I think it's great to type the input of the hooks generically, but the output should be typed specifically to avoid type collisions with custom made components.

Typescript's utility type Pick<T, Keys> could be utilized to improve these typings, like:

interface KeyboardResult {
  /** Props to spread onto the target element. */
  keyboardProps: Pick<HTMLAttributes<HTMLElement>, "onKeyDown" | "onKeyUp">
}

Otherwise I get this TS error for my custom onChange event:

Type '{ className: string; type: string; value: string; renderLeftSide: (() => Element) | undefined; ref: (node: any) => void; defaultChecked?: boolean | undefined; defaultValue?: string | number | readonly string[] | undefined; ... 280 more ...; labelProps: { ...; }; }' is not assignable to type 'Pick<InputProps, "onChange" | "accept" | "acceptCharset" | "action" | "allowFullScreen" | "allowTransparency" | "alt" | "as" | "async" | "autoComplete" | "autoFocus" | ... 353 more ... | "isInvalid">'.
  Types of property 'onChange' are incompatible.
    Type '(((event: FormEvent<HTMLElement>) => void) & ((event: ChangeEvent<HTMLInputElement & HTMLTextAreaElement>) => void)) | undefined' is not assignable to type '((value: string) => void) | undefined'.
      Type '((event: FormEvent<HTMLElement>) => void) & ((event: ChangeEvent<HTMLInputElement & HTMLTextAreaElement>) => void)' is not assignable to type '((value: string) => void) | undefined'.
        Type '((event: FormEvent<HTMLElement>) => void) & ((event: ChangeEvent<HTMLInputElement & HTMLTextAreaElement>) => void)' is not assignable to type '(value: string) => void'.
          Types of parameters 'event' and 'value' are incompatible.
            Type 'string' is not assignable to type 'FormEvent<HTMLElement>'.ts(2322)

Any hook result with a generic type would need a refactoring:
https://github.com/adobe/react-spectrum/search?q=HTMLAttributes%3CHTMLElement%3E

[AndyOGo]

I started a PR for the integration hooks
https://github.com/adobe/react-spectrum/pull/1619/files

[devongovett]

The returned props are intentionally very broad. You should treat them as opaque implementation details and spread them directly onto DOM elements or pass them around as a whole rather than individually. This allows us to change the implementation in the future to return more or less props without breaking changes. If we typed each prop individually, then we could never remove props, and adding props would also potentially break things if only the previously used props were passed around.

I'm curious where you're seeing TS errors. It makes me think the returned props are being used somewhere other than being spread on a DOM element, which seems like it would be problematic.

[AndyOGo]

Im curios if it would help to apply broad types only if needed?

[AndyOGo]

In fact useTextfield applies the same onChange simplification with the ValueBase type.

react-spectrum/packages/@react-aria/textfield/src/useTextField.ts

Line 80 in fa31439

onChange: (e: ChangeEvent<HTMLInputElement>) => onChange(e.target.value),

react-spectrum/packages/@react-types/shared/src/inputs.d.ts

Lines 32 to 39 in fa31439

	export interface ValueBase<T> {
	/** The current value (controlled). */
	value?: T,
	/** The default value (uncontrolled). */
	defaultValue?: T,
	/** Handler that is called when the value changes. */
	onChange?: (value: T) => void
	}

[devongovett]

Yeah, this can happen sometimes. I see two possibilities, which we use in React Spectrum itself to handle this in some places.

1. Add a prop that accepts DOM props.

<Input domProps={inputProps} />

2. Use a context to pass DOM props:

<DOMPropsProvider value={inputProps}>
  <Input />
</DOMPropsProvider>

And make Input consume from that context.

In your case, the onChange prop returned from useTextField actually doesn't match your Input component's props, so the TS error is probably correct. You need to either find a way to pass the DOM props through, or adapt them to match your component's API.

[AndyOGo]

Thanks for your nice reply @devongovett

Meanwhile I moved on with option 1 and I layered our onChange simplification one level up.

From the viewpoint of a library consumer I still think the typings are too broad, especially if only 2 or 3 out of many dozen DOM props are actually being used.

Anyway this is a great library and I'm sure you considered much more use cases than I have to deal with right now.

[venikx]

Would this be okay, while keeping the types as wide as possible? #3287

[venikx]

@devongovett
I might be able to chime in here, because I ran into the same issues in an application I'm working on, when I pulled down react-spectrum and re-implemented the component using the libs from react-spectrum I did not run into the issue. For me specially there's a type mismatch when using all the hooks from the useDialog example.

I traced it back to the following:
From my app

    interface FocusEvent<T = Element> extends SyntheticEvent<T, NativeFocusEvent> {
        relatedTarget: EventTarget | null;
        target: EventTarget & T;
    }

From react-spectrum

    interface FocusEvent<Target = Element, RelatedTarget = Element> extends SyntheticEvent<Target, NativeFocusEvent> {
        relatedTarget: (EventTarget & RelatedTarget) | null;
        target: EventTarget & Target;
    }

So I wonder, if I run a different version of @types/react than react-spectrum, would typings be slightly different?