Make FormControl externally extensible (via HOC)

[iansan5653]

Motivation

FormControl supports an 'auto-wiring' functionality where it automatically applies certain props to the wrapped input in order to make accessibility easier. For example, it automatically applies an id prop (if none is provided) to link the wrapped input to the Label.

At present, this is only supported for a limited subset of components, defined internally:

react/src/FormControl/FormControl.tsx

Lines 58 to 67 in 65b888c

	const expectedInputComponents = [
	Autocomplete,
	Checkbox,
	Radio,
	Select,
	TextInput,
	TextInputWithTokens,
	Textarea,
	InlineAutocomplete,
	]

Consumers of the library cannot add to this list, so it's impossible for consumers to create their own components that support integration with FormControl.

As I work on moving the InlineAutocomplete component out of this repository, we either need to make this functionality externally extensible or we have to break the InlineAutocomplete component and require it to be manually wired up whenever used. I think the first one is much more desirable as it's generally useful for all component authoring.

Solution

To solve this, I added a new unique Symbol that is internal to @primer/react. Component authors must explicitly attach this Symbol to their components by wrapping the component in the FormControl.autoWirable higher-order component (HOC). This is the only way a component can be registered. The FormControl component will now simply look for the presence of this symbol on the type of each child element.

Example:

const RedTextInput = FormControl.autoWirable(
  (props) => <TextInput sx={{backgroundColor: 'red'}} {...props} />
)

This HOC is fully compatible with memo and forwardRef.

As described in the documentation comments, wrapping a component in this HOC is a promise that the component will forward the desired props to the underlying input. This cannot be enforced by types or even runtime checks - there's no way to know that the underlying component is actually forwarding all the props. This is why it must be an explicit, well-documented decision.

The HOC does have some type-safety: it won't accept components that are incompatible with the forwarded props. For example, a type error will be raised if a component has the prop {required: number} because number is not compatible with the boolean type that is forwarded.

[changeset-bot]

🦋 Changeset detected

Latest commit: 29f40f3

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
@primer/react	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

[iansan5653]

This issue was already present - I just noticed it while working here and decided to call it out explicitly

[iansan5653]

An advantage of this approach is that it's internally much easier to typecheck, allowing for significantly reduced boilerplate to satisfy the compiler. This is because the isValidAutoWirableElement check not only filters out disallowed elements but also asserts that the element in question supports the prop shape being assigned.

[mattcosta7]

🚀

[siddharthkp]

Would also like @mperrotti's thoughts here, added him as reviewer

[joshblack]

Love how easy this makes it to participate in the FormControl component 🔥 Seems to make it dead simple to just call FormControl.autowirable on your component and you're good to go.

In terms of approach, could you share more about the pros/cons of this approach versus others we could take? For example, if we exposed something like a useFormControl hook that one could call to get access to the props for the input?

For context, one concern I have (generally) is with strict Parent > Child relationships and I'd personally love to explore how we could support more flexibility when supporting the children prop. However, totally understand if that's just not something we really could ever explore here due to existing limitations or functionality we have to support.

[iansan5653]

In terms of approach, could you share more about the pros/cons of this approach versus others we could take? For example, if we exposed something like a useFormControl hook that one could call to get access to the props for the input?

I went with this approach because it was most similar to what we were already doing, so it felt like the lowest-risk change.

I think the most common alternative here would be to accept a render function as children, but this is (arguably) pretty hard to read and cumbersome to use. It becomes even more cumbersome when combined with the composable API of the component where we also have FormControl.Label in children.

Using context + a hook is an interesting idea that for some reason I never even thought to consider on Friday. 🤔 Typically the disadvantage here vs Children based APIs is that the component must manually opt in by calling the hook. But here the component must manually opt in anyway -- maybe this is way to go? It's a more significant change but it lets us move away from the legacy Children API and in some ways it's actually safer and cleaner to use. I might open a second PR so we can compare and contrast the two approaches. Perhaps there are limitations that prevent this.

[iansan5653]

Here's the hook-based approach: #3632

Honestly, I say we close this PR and go with that approach. It's much cleaner code and arguably just as nice of an API. Great suggestion @joshblack ❤️

[joshblack]

Wow! Thanks so much for opening up that exploration for it, can't wait to check it out 👀 What you shared as next steps sound great to me @iansan5653, I'll make sure to checkout the PR tomorrow or Thursday if that works timing-wise!