Add abstractions for controlled PF inputs with react-hook-form, improve types in ProxyForm and IdentityForm #404

mturley · 2022-09-13T19:47:34Z

This PR makes a handful of changes with the goal of reducing code duplication and improving type safety in our forms, starting with the ProxyForm (Administrator view -> Proxy) and the IdentityForm (Administrator view -> Credentials -> Create/Edit modal).

Lessons learned

The original plan for this PR was to add stricter types to our existing react-hook-form usage and replace the use of react-hook-form's <Controller> entirely with their simpler {...register('fieldName')} syntax as shown in their basic examples. That didn't work out for a few reasons:

The register function returns props/attributes intended to go directly on an HTML <input> element. While PatternFly's TextInput (for example) extends the interface of React.HTMLProps<HTMLInputElement> for the most part, it has slightly different types for some props (onChange for example takes (value, event) arguments rather than just event), so directly spreading <TextInput {...register('fieldName')}> causes type errors and doesn't work. I considered making some kind of custom function like registerPF(register('fieldName')) that would adjust the props as necessary, but it was awkward and didn't solve all the problems.
The main benefit of the register approach is to use uncontrolled inputs (don't re-render the whole form when a value changes, just let the DOM store the values and them imperatively when they are needed / on submit). However, PatternFly (like most React component libraries) is very much designed around controlled inputs. To properly render the labels, validation styles, etc around a field we need to render a <FormGroup> component which will re-render when the field values change. The same goes for anything else on the page that needs to react to changing fields (like enabling/disabling buttons). So we were already going to have to force react-hook-form to watch all field values all the time and behave more like Formik, and the performance benefits of register are moot. Additionally, to get the values and errors of each field in scope for <FormGroup>s we would need to pass down a bunch of extra stuff from useForm and repeat it in each field, which we don't need to do with <Controller> since it encapsulates all that per-field.
There were always going to be other fields that couldn't use register anyway due to custom controls (e.g. Switch, Select), so they would have had to use <Controller> anyway and things would have been inconsistent.
So it appears react-hook-form's <Controller> is actually a good fit for what we need. The only problem is that it is verbose and repetitive, especially when used with <FormGroup> with things like validated and helperTextInvalid wired up the same way for every field. So I figured, why not introduce some abstraction!

See "Accessibility and QE selectors" below for some additional lessons learned.

Notable changes

New components under client/src/app/shared/components/hook-form-pf-fields:
- <HookFormPFGroupController> renders a react-hook-form <Controller> around a PatternFly <FormGroup>. It takes the props control (from useForm) and name (field name string), and wires up the validation styles and error messages for you and accepts a renderInput prop of the same type as the Controller's render prop so it can be used with any kind of field. TypeScript Generics are used (automatically inferred from the control prop) such that name will be required to match an actual form field name and the value argument of renderInput will have the correct type for that specific field. Some common props of FormGroup such as label, isRequired and fieldId are directly accepted, but you can also pass any additional custom props to the FormGroup as a formGroupProps object.
- <HookFormPFTextInput> renders a <HookFormPFGroupController> around a PatternFly <TextInput>. It extends the props interfaces of both HookFormPFGroupController and TextInput, extracting any props for the former and passing any remaining props directly to the latter. This way it can accept control, name, formGroupProps etc. and also accept TextInput props like type and arbitrary HTML attributes like data-testid.
- <HookFormPFTextArea> behaves the same way as HookFormPFTextInput, but renders a PF TextArea instead of a TextInput and extends that component's props interface.
- We could possibly add additional HookFormPF___ components later, but I decided to stop here for now.
  - I started to make a HookFormPFSwitch, but the Switch doesn't need a FormGroup so there was little to gain from the abstraction (would have saved maybe one or two lines of code?). It's plenty readable to directly wrap a <Controller> around a <Switch> in the form.
  - I started to make a HookFormPFSimpleSelect, but I didn't feel confident it was the right abstraction (SimpleSelect itself could use some work TypeScript-wise). I just directly used <HookFormPFGroupController> around <SimpleSelect> in the forms instead for now.
Refactoring in ProxyForm and IdentityForm:
- Replaces most direct use of <Controller>/<FormGroup>/<TextInput>/<TextArea> with the new components to reduce duplication and make the form code more readable
- Adds explicit type interfaces for ProxyFormValues and IdentityFormValues, and uses these as generic type arguments for useForm and with yup.SchemaOf<...> as the type annotation for the schema.
  - Note: Because the control returned by useForm inherits the type argument passed to useForm, the HookFormPF* components can infer these ProxyFormValues / IdentityFormValues type arguments simply by passing in the control prop. No need to repeat those type args in every field.
- Fixes type errors resulting from the use of these interfaces (added some missing fields to schema, added .defined() calls on some fields in schema, removed some unused fields from defaultValues, etc)
- Replaces the use of "true" / "false" string literals with proper boolean types
- Replaces the use of "" as a fallback for some empty values with undefined to support the use of string literal union types (for example, the kind on an Identity object now has type "source" | "maven" | "proxy" rather than string, which enforces that any usage or checks of that property match one of those values exactly. Empty-string would cause type errors here, but making the field optional and having it fall back to undefined when there is no selection resolves this)
- Moves any state which was used to determine requiredFields for a schema into the form state itself (HTTP/HTTPS proxy switches, for example). As a result, replaces conditionally-defined yup.lazy() schema fields with yup's .when() feature to have schema vary based on the values of other form fields.
- Removes the use of uppercase constants for field names (deletes the client/src/app/pages/proxies/field-names.ts file). Now that we have strict type interfaces for these fields that are enforced in all 3 places (useForm, schema, components), field name constants don't add any value. Using an invalid field name or leaving out a field in any of these 3 places will now cause type errors.
- Replaces explicit any with strict types.
- Cleans up redundant ARIA attributes (see "Accessibility and QE selectors" below).
- Misc other cleanup (unused variables/imports, some redundant state, some inconsistent field spacing, etc).
- Overall each of these two forms have about ~100-200 fewer lines of code after this change.
In the ProxyForm specifically, another notable fix has been added: in client/src/app/pages/proxies/proxies.tsx, the ProxyForm is not rendered until isFetching is false, so we no longer need to reinitialize the form when the existing proxies are loaded (that useEffect has been removed from ProxyForm). Instead, the ProxyForm has access to all the data it needs when it is first rendered, so its defaultValues in useForm are correct right away. This should allow us to remove a hack from the QE tests where the tests have to wait a few seconds after loading this page before checking values.
Unit tests for these two forms have been cleaned up and changed to use accessible labels wherever possible, and some eslint errors in the test files have been addressed.
The @types/yup dependency has been removed because types are now bundled in the yup package directly.

If we all agree on the approach I've taken with these two forms, I think this will be a good foundation for cleaning up the rest of our react-hook-form usage and eventually converting our Formik forms to react-hook-form as well.

Accessibility and QE selectors

Another thing I looked at as part of this PR is how we can best support QE while also making correct use of ARIA attributes. Currently, we use an assortment of things as identifiers for QE test automation: id in some places, aria-label in some places, data-testid in some places, etc. A few observations here:

ARIA attributes such as aria-label are intended to provide disability accessibility to elements which don't natively support it by surfacing human-readable labels. So we shouldn't be putting shorthand ids in an aria-label, they will be read out loud to a sight-impaired user and should be more like full sentences or phrases with spaces. Similarly, aria-describedby is supposed to be the id of an element containing a longer description of the field, so it shouldn't be its own identifier for direct use.
The first rule of ARIA is:

If you can use a native HTML element or attribute with the semantics and behavior you require already built in, instead of re-purposing an element and adding an ARIA role, state or property to make it accessible, then do so.

There is another quote in the MDN ARIA docs:

No ARIA is better than bad ARIA.

So we also should be avoiding aria-label in general if there is already an accessible label provided by a native HTML element. Most of our fields have an HTML <label> element (either rendered by FormGroup or rendered as part of the field component itself). <label> has an attribute called for (expressed in JSX as htmlFor):

The value of the for attribute must be a single id for a labelable form-related element in the same document as the <label> element

And if you look at the source for PatternFly's FormGroup component, you can see the fieldId prop is passed into the <label> as its htmlFor prop. This means that as long as we have an id on our fields themselves and pass that same id as fieldId on the FormGroup, we already have an accessible label for that field and we don't need a redundant aria-label. I verified this with my changes to the unit tests for these forms; react-testing-library is happy to use these labels via findByLabelText. The only caveat there is that required fields end up with an asterisk (*) in their <label>, so for those you have to do e.g. findByLabelText("Username *").
QE has requested stable identifiers of some kind for each form field so they can use those rather than label text. This is understandable since label text can change with the design and we can't update QE tests with new label text in the same pull request like we can with unit tests. When we last met with QE we concluded that we would use data-testid attributes wherever necessary. Previously we decided we didn't want to use regular ids for tests because react-testing-library advises against it, but react-testing-library also says to prefer accessible labels wherever possible and only fall back to data-testid if that is impossible for some reason.

Now that I've realized we need a regular old id on each field anyway in order to provide accessible labels without redundant ARIA attributes, I wonder if we should just be communicating that QE should use those plain ids wherever possible, and we will commit to adding them wherever possible. QE gets a stable identifier, we get our built-in accessible labels, no ARIA required. We can always fall back to data-testid or helping them construct a different selector manually if there are specific cases where an id isn't possible. (Those cases should also probably be considered PatternFly bugs).

We can still use aria-label where there is no other good way to provide an accessible label (for example, with SimpleSelect). I think the fact that id can't be directly used by react-testing-library is a good thing: we will be forcing ourselves to both provide a stable identifier to QE and also make sure every field has an accessible label in order for tests to pass.