provide more accurate types for useTextField

[chrishoage]

Closes mui/material-ui#1760

This PR uses Function Overloads to support both Textarea and Input elements in useTextField. If the approach is approvied there will need to be a tweak to the eslint config to allow for overloads https://github.com/typescript-eslint/typescript-eslint/blob/master/packages/eslint-plugin/docs/rules/no-redeclare.md (I discovered the precedent of eslint-disable lines in useButton)

I also think this might open up support for closing #1178 since the overload should allow props that only apply to Textarea or Input however I haven't attempted to solve that yet.

In order to achieve fixing that there will need to be a split of AriaTextFieldProps into AriaTextFieldInputProps and AriaTextFieldTextareaProps such that the overload will allow only Textarea props when the signature matches.

I'm happy to tweak this as needed

✅ Pull Request Checklist:

• Included link to corresponding React Spectrum GitHub Issue.
• Added/updated unit tests and storybook for this change (for new code or code which already has tests).
• Filled out test instructions.
• Updated documentation (if it already exists for this component).
• Looked at the Accessibility Practices for this feature - Aria Practices

📝 Test Instructions:

No regressions in useTextField

Case described here works https://codesandbox.io/s/affectionate-bardeen-rtfmw

🧢 Your Project:

[chrishoage]

If this approach is okay I will update useComboBox and useSearchField with the same function overload pattern which will fix the lint errors.

Though I do wonder if it's necessary to have useSearchField and useComboBox to allow for HTMLTextAreaElement considering the use cases of the hooks.

Even so doing SearchInputFieldAria and SearchTextariaFieldAria with function overloads will work just fine

[snowystinger]

It looks similar to what I'd been expecting for the other issue.
Curious about your comment that we need to have useSearchField allow for TextArea, we'd prefer it if it didn't allow for that. As you said, it doesn't make much sense. Or were you just asking if you should do that?

[chrishoage]

Curious about your comment that we need to have useSearchField allow for TextArea, we'd prefer it if it didn't allow for that. As you said, it doesn't make much sense.

The types currently allow for both HTMLInputElement and HTMLTextAreaElement

react-spectrum/packages/@react-aria/searchfield/src/useSearchField.ts

Lines 22 to 40 in e066b67

	interface SearchFieldAria {
	/** Props for the text field's visible label element (if any). */
	labelProps: LabelHTMLAttributes<HTMLLabelElement>,
	/** Props for the input element. */
	inputProps: InputHTMLAttributes<HTMLInputElement | HTMLTextAreaElement>,
	/** Props for the clear button. */
	clearButtonProps: AriaButtonProps
	}
	/**
	* Provides the behavior and accessibility implementation for a search field.
	* @param props - Props for the search field.
	* @param state - State for the search field, as returned by `useSearchFieldState`.
	* @param inputRef - A ref to the input element.
	*/
	export function useSearchField(
	props: AriaSearchFieldProps,
	state: SearchFieldState,
	inputRef: RefObject<HTMLInputElement | HTMLTextAreaElement>

If I remove HTMLTextAreaElement I no longer get type errors in that hook.

Same for useComboBox, the inputRef allows RefObject<HTMLInputElement | HTMLTextAreaElement>. Chaning this to just HTMLInputElement doesn't cause any issue with Typescript applying the correct overload.

[snowystinger]

O interesting, I didn't notice it already had it, that makes it more complicated :( I think we'll have to have the overloads for both so it's not a breaking change.

[chrishoage]

Sure no problem. I'll implement the function overloads for both useComobox and useSerachInput in the next couple of days!

I just wanted to make sure that the approach would be considered for merging before I went to the trouble 🙂

[chrishoage]

@snowystinger I have pushed up the changes for useCombobox and useSearchField.

I will note - it seems better to make this a breaking change and to not allow Textarea for these hooks. It really doesn't make sense and it really complicated the types (for example, I'm unhappy with the use of any for the function implementation but the resulting types are correct due to the overload)

Also of note I had to change ComboBox.tsx and SearchField.tsx as these allowed a ref of HTMLInputElement | HTMLTextAreaElement which seems obviously incorrect to me.

It's also noteworthy that until the parcel build is fixed (#1388) such that overloads are properly included in the types.d.ts file that this change is a little moot - since sometimes the returned inputProps result in any type

I believe that the lint step in the CI is failing due to the missing types from the parcel build (#1388)

Please let me know of any feedback or anything you'd like me to address here

[chrishoage]

I unfortunately need to withdraw my PR. We have begun to unwind our usage of react-aria due to issues with types and reliance on React Stately. Using react-aria outside of React Spectrum is quite difficult when not conforming to the design considerations made for React Spectrum.

[devongovett]

@chrishoage I'm sorry to hear that. I also apologize for not getting back to your PR. If you don't mind, could you expand on the problems you ran into aside from the type changes in this PR? What were your issues with React Stately, and what did you find difficult to use?

[chrishoage]

@devongovett Sure, happy to!

By far the biggest issue is the well-baked in pattern of passing in all props to the hooks, and then getting a bag of unknown props back. A goal for our component library was strict types, and every single hook in react-aria returning HTMLAttributes<HTMLElement> became a huge headache.

I'm going to pick on useOverlayTrigger.ts in particular, but we encountered similar issues with all hooks provided in react-aria which I will give a concrete example later.

react-spectrum/packages/@react-aria/overlays/src/useOverlayTrigger.ts

Lines 60 to 68 in 9aab12e

	return {
	triggerProps: {
	'aria-haspopup': ariaHasPopup,
	'aria-expanded': isOpen,
	'aria-controls': isOpen ? overlayId : null
	},
	overlayProps: {
	id: overlayId
	}

The return value of overlayProps inside useOverlayTrigger is static and well known, it's a single attribute: id. However the return type for this is

react-spectrum/packages/@react-aria/overlays/src/useOverlayTrigger.ts

Lines 28 to 29 in 9aab12e

	/** Props for the overlay container element. */
	overlayProps: HTMLAttributes<HTMLElement>

Which means that we can't know what types could possibly come from the hook with out inspecting the source.

This became particularly problematic when trying to use useOverlay on a <motion.div /> element from Framer Motion, which has incompatible types with HTMLAttributes<HTMLElement>. Due to this we weren't able to spread the props from overlayProps onto the framer motion element to preform our animations. Again, this mean inspecting the source and picking off individual props - or not using useOverlay and rolling our own (which we ultimately did).

As for React Stately, the issue was our company ESLint rules which among other things require dependencies be added to react hook dependency arrays. We wished to extend useSliderState by adding functionality that changed the value on hover. This meant using state.getPercentValue in useEffect, where state is in the dependency array. This is problematic since useSliderState does not make use of useMemo to return a stable object between renders. We could of course disable the lint-rule, however the real issue is most/all of the hook returns contain functions that are re-created every render, or objects that are. Consuming React Stately hooks means we need to be familiar with what the stately hook is doing and respond accordingly.

There were some other issues too, most notability no support for nested menus with useMenu.

Ultimately we kept usePress with a wrapper to give it a proper return signature along with FocusScope (and friends), and usePreventScroll.

In summary, the issues were around a need for strong type signatures and extensibility that doesn't break when used with hooks that require sable dependencies. If we were construing our component library in the way React Spectrum is constructed I'm sure it would would great. However our requirements lead to conflicts with how the react-aria library fundamentally works.

Hope this was helpful, and thank you for your work on this project!

[devongovett]

@chrishoage thanks so much for the detailed feedback!

Just for context about the reasoning for returning HTMLAttributes, we wanted to ensure that we could add additional DOM attributes later if needed without this being treated as a breaking change. For example, if we added a feature or fixed a bug and needed to add a new ARIA attribute or event handler, but users were relying on us only returning certain props and didn't pass that new prop through to their DOM element, things could be broken. The same but potentially worse could happen if we removed a returned attribute. I can see why this could be problematic though, if you're passing props to another component and not directly to a DOM element, so it's an interesting tradeoff. We certainly deal with this ourselves in React Spectrum as well and haven't found a great solution yet. If you have any ideas, we'd love to hear them.

As for memoizing the return values of stately hooks, that's definitely something we should look into.

[bstro]

A goal for our component library was strict types, and every single hook in react-aria returning HTMLAttributes became a huge headache.

I'm running into a similar difficulty. It is difficult to understand what React-ARIA actually does when the return types of its hooks don't provide meaningful signal. My team is also building a ui library and we might not be able to continue down the React-ARIA path, as much as we like the project otherwise.

edit: looks like the React-Stately issue is going to apply to us too, since we also use the same eslint rule (enabled by default on apps created w/ create-react-app)

[devongovett]

@bstro do you have any ideas for how to handle the above issues re breaking changes whenever we add/remove DOM attributes? What specifically are you having difficulty with?

[bstro]

Apologies for my lack of specificity—I first encountered the same interop issue w/ Framer Motion as @chrishoage described.

I don't really have a suggestion yet for how to solve the breaking change consideration, but I'll give it some thought! As a downstream user of this library, I would personally prefer more accurate types, and if/when breaking api changes occur, I would read the release notes for how to update my code. Granted, I've never worked on a library of this size and scope, so that might be a naive take.

[bstro]

I have two ideas, but first I want to clarify my own understanding:

My understanding is that the current API encourages users to spread the entirety of the props returned by React-ARIA's hooks directly into the React element i.e. <button {...buttonProps} …/>. My sense is that using the return type of HTMLAttributes<HTMLElement> basically ensures that consumers will pass all the props through, since they don't actually know the contents of buttonProps, and since consumers are encouraged to use the API in that way, this library can add/remove attributes as desired without any breaking changes. Is this true?

My first thought is, if the above is true, would it work to use the Pick utility to narrow the relevant properties of HTMLAttributes<HTMLElement> for each hook's returned props? The docs could still encourage consumers to pass all the props through, but we'd at least know exactly what we're spreading into our components, which I think is valuable information. For example, the type for overlayProps returned from useOverlayTrigger, could instead be something like Pick<HTMLAttributes<HTMLElement>, 'id'>. That's a simple example since overlayProps is quite minimal and I'm unsure if that's a reasonable approach across-the-board.

Here's another idea, I have no idea if it's workable or not, but what about exposing the same hooks with stricter types under a different name? These hooks would have the exact same implementation, but have narrower/accurate types. For example, in addition to useTextField with its current return type, there could also be useTextFieldExactProps (not sure about the name), that would have the same implementation but return an exact type. That way, consumers could opt into an API that offers more specific return types, with the potential risk of encountering more breaking changes. That way, nothing changes for anyone depending on the current behavior.

[devongovett]

Right, buttonProps should be thought of as an opaque implementation detail of React Aria. We need to be able to add or remove DOM attributes in order to change the implementation without breaking people's code. That could be caused if people only pass through certain props (nothing we can do to stop that), but it could also break in terms of types. It's less about the individual types for each prop and more about the ability to add/remove props in the future.

Say we added an event handler or new DOM attribute to the return type of a hook in a minor release. If those props are being spread onto an element that doesn't expect that attribute, the build will fail and it could be considered a breaking change. Because we tell you to spread, basically any time we add anything it could be breaking. That's why we reserve all DOM attributes for potential use. You work out once how to pass DOM attributes through to your components, and never need to worry about it again, vs potentially having to worry about it whenever your React Aria version changes.

To be clear, we struggle with how to handle this sometimes in our own component library. We don't want to expose all DOM attributes as part of the public API, but if we want to reuse that component in another component and need to pass through some DOM props it becomes difficult.

We've tried a couple different approaches:

1. Split the component into a public version with narrower types, and a base component that accepts DOM props. This way, external consumers get the public version without DOM props, and internal users can use the base component. An example of this is TextField and TextFieldBase.
2. Use a context to pass DOM attributes through so they aren't exposed in the API. We do this in a number of different places in a few ways. One way is to use the PressResponder component which is built into @react-aria/interactions. It's currently undocumented, but we use it in a bunch of places. Basically, it is a provider for props that are consumed by usePress, and it allows DOM attributes to be passed through. This way, anything that uses usePress can consume arbitrary DOM props provided from outside. An example of our usage of this is MenuTrigger, which passes props from useMenuTrigger through to the pressable element inside it, where Button would not accept arbitrary DOM props.

As for interop with other libraries... I'm not sure what the solution is. I'm a bit surprised Framer's motion.div doesn't accept all DOM attributes given it is a DOM element. You always have the option of passing props one by one instead of spreading I guess, or using TS casts or ignores where appropriate.

Again, I'm open to more ideas here, but I think we need to be careful about this so we don't shoot ourselves in the foot down the road.

[devongovett]

I guess an obvious solution would be to have the props defined as any so they never cause errors:

interface ButtonAria {
  buttonProps: any
}

That seems a little weird and might potentially cause other issues though. I noticed that Downshift does this though: https://github.com/downshift-js/downshift/blob/master/typings/index.d.ts#L341

[devongovett]

material-ui's hooks do the same as React Aria: https://github.com/mui-org/material-ui/blob/ef6d5d46bade437f155ce96d7403086cb50f52ef/packages/material-ui-unstyled/src/AutocompleteUnstyled/useAutocomplete.d.ts#L308-L322

[bstro]

Thanks for the thoughtful reply and context. That's really interesting re: material-ui. I see what you mean for their useAutocomplete hook, and I'm also noticing that they provide more specific types for their useButton hook. Still, it looks like early days for them re: their headless API, will be interesting to see where they land.

[devongovett]

Yeah looks like they didn't define any types in that one, just relying on inference.