[react-form] DX improvement ideas

[Lodin]

I have some concerns about the DX of react-form.

What do you think about getting useField back in place of the field directive we use now? I found it quite complex to render a template in response to changes for specific fields.

Let's consider the following code (right from tests):

function UserForm({ model: user }: UserFormProps) {
  const { field, model } = useBinderNode(user);

  return (
    <fieldset>
      <input data-testid="user.name" type="text" {...field(model.name)} />
      {USER_NAME.invalid && <output data-testid="user.name.validation">{USER_NAME.ownErrors.map((e) => e.message).join(', ')}</output>}
      <input data-testid="user.password" type="text" {...field(model.password)} />
      {USER_PASSWORD.invalid && (
        <output data-testid="user.password.validation">{USER_PASSWORD.ownErrors.map((e) => e.message).join(', ')}</output>
      )}
    </fieldset>
  );
}

Ideally, I would like to have a specific invalid for a user.name field and user.password field separately. The React way supposes the following code:

function UserForm({ model: user }: UserFormProps) {
  const { field, model, invalid, ownErrors } = useBinderNode(user);
  const name = useField(model.name);
  const password = useField(model.password);

  return (
    <fieldset>
      <input data-testid="user.name" type="text" name={name.name} ref={name.ref} />
      {name.invalid && (
        <output data-testid="user.name.validation">{name.ownErrors.map((e) => e.message).join(', ')}</output>
      )}
      <input data-testid="user.password" type="text" name={password.name} ref={password.ref} />
      {password.invalid && (
        <output data-testid="user.password.validation">
          {password.ownErrors.map((e) => e.message).join(', ')}
        </output>
      )}
    </fieldset>
  );
}

This approach is the simplest and most convenient for any person who already has some experience with React or for someone who is not familiar with React at all.

If we stick with the current approach, the following code is gonna work:

function UserForm({ model: m }: UserFormProps) {
  const { field, model } = useBinderNode(m);
  const name = getBinderNode(model.name);
  const password = getBinderNode(model.password);

  return (
    <fieldset>
      <input data-testid="user.name" type="text" {...field(model.name)} />
      {name.invalid && (
        <output data-testid="user.name.validation">{name.ownErrors.map((e) => e.message).join(', ')}</output>
      )}
      <input data-testid="user.password" type="text" {...field(model.password)} />
      {password.invalid && (
        <output data-testid="user.password.validation">
          {password.ownErrors.map((e) => e.message).join(', ')}
        </output>
      )}
    </fieldset>
  );
}

However, I don't see any reason to keep this approach if the useField is gonna be more in React-way and simpler, and have less code than this one. It also doesn't force user to use non-React API like getBinderNode.

What do you think?

[platosha]

Why not to use a pair of useBinderNode calls here?

function UserForm({ model: m }: UserFormProps) {
  const name = useBinderNode(model.name);
  const password = useBinderNode(model.password);
  return <>
    {name.invalid ? name.ownErrors[0]?.message}
  </>;
}

[Lodin]

Hmm. But do we need the field directive then?

[platosha]

We still do, cause we want to keep the details of the binding implementation on our side, instead of asking the user to bind the ref={...} and such.

[platosha]

One possible improvement here is making the default model in the field call to be the useBinderNode().model, so that you could:

function UserForm({ model }: UserFormProps) {
  const name = useBinderNode(model.name);
  const password = useBinderNode(model.password);
  return <>
    <input {...name.field()}/>
    <input {...password.field()}/>
  </>;
}

[Lodin]

Yeah, I was just about to write the same 😅

function UserForm({ model: m }: UserFormProps) {
  const name = useField(model.name);

  return <input data-testid="user.name" type="text" {...name.field} />;
}

[Lodin]

I would probably like to have a separate useField hook to store the ref info inside and to emphasize that it is the leaf element of the form.

[platosha]

As discussed with the team, let us:

• rename useBinder to useForm for alignment with React user expectations
• rename useBinderNode to useFormState again for user expectations and better discoverability
• add state access function to both for easy access of nested field state

const { field, state, value, errors,... } = useForm(model);
state(model.name).errors;
state(model.name).invaliid;
state(model.name).value;
state(model.name).setValue("Someone");
const { field, state, value, errors,... } = useFormState(model);

[Legioth]

The state access function is problematic from the point of view of discoverability. Would it be possible change the entry point of the API so that you could use the errors variable that is already defined by the useForm line?

In that way, the developer could discover how to proceed with the help of auto completion from errors. That would imply that it would be something like errors[model.name] or errors.forModel(model.name) instead of state(mode.name).errors.

If we keep state, then I'm wondering if the name should be clarified to avoid confusion with useState.

[Lodin]

For errors yes, we could do that. However, it would also require us to create the same accessors for invalid, value, setValue etc (about 20 props in common). So we chose the less clunky solution here.

[Legioth]

How is that different from adding those same 20 properties to the return value from state?

[Lodin]

Hmm, good question.

So, what if we move towards this direction a bit more?

// If we need to access the current model
function UserPasswordField({ model: password }: UserPasswordField) {
  const {field, invalid, errors} = useFormAPI(password);

  // We just access the same model we have
  return (
    <>
      <input data-testid="user.password" type="text" {...field(password)} />
      {invalid(password) && (
        <output data-testid="user.password.validation">{errors(password).join(', ')}</output>
      )}
    </>
  );
}

function UserForm({ model: user }: UserFormProps) {
  const { field, invalid, errors } = useFormAPI(user);

  return (
    <fieldset>
      <input data-testid="user.name" type="text" {...field(model.name)} />
      {invalid(model.name) && <output data-testid="user.name.validation">{errors(model.name).join(', ')}</output>}

    </fieldset>
  );
}

So, overall, the useFormAPI hook here is used only to extract form- and nesting-related API functions. With them we could have two different fields that change the same data without conflicts.

@Legioth, @platosha, @taefi, @sissbruecker, what do you think?

[Legioth]

I don't understand why "API" is needed in the name of the hook. Aside from that, I consider my concerns about discoverability addressed.

Discoverability would be one notch better if we'd avoid using destructuring assignments with a complex API like this. The difference is that if you do const binder = useBinder(model);, then you can at any place in the code discover what all the things you can do by typing binder. and picking for a list of auto complete suggestions. With the const { foo, bar, baz } = useBinder(model); pattern, you need to go back to that line to see what other things are available and even then you don't get as good indication of what any given operation does.

[Lodin]

I don't understand why "API" is needed in the name of the hook.

Well, that's a matter of naming. I'm not sure what other name is usable here. Maybe, we could make useForm overrided... I wonder.

Discoverability would be one notch better if we'd avoid using destructuring assignments with a complex API like this.

Yeah, I would agree. It is more a documentation matter though, because if you want, you could use a destructuring, and if you don't, you can use a whole object right away.

[Lodin]

After a small discussion with @platosha we came to a compromise solution.

1. The useFormState hook returns a set of two elements: field and access.

   interface UseFormStateResult {
   field(model: Model): Readonly<{ ref: Ref, name: string }>,
   access(model: Model): Readonly<{ errors, invalid, value, setValue, ... }>
   }

2. The useForm hook returns everything useFormState returns plus the model instance:

   interface UseFormResult extends UseFormStateResult {
   model: Model;
   }

So, now if we want to access the state of the specific model, we use the access function:

function UserForm({ model: user }: UserFormProps)  {
  const { field, access } = useFormState(user);
  const name = access(user.name);
  const password = access(user.password);

  return (
    <fieldset>
      <input data-testid="user.name" type="text" {...field(model.name)} />
      {name.invalid && <output data-testid="user.name.validation">{name.errors.map((e) => e.message).join(', ')}</output>}
      <input data-testid="user.password" type="text" {...field(model.password)} />
      {password.invalid && (
        <output data-testid="user.password.validation">{password.errors.map((e) => e.message).join(', ')}</output>
      )}
    </fieldset>
  );
}

[taefi]

@Lodin Isn't it now a little bit confusing where to use that name thingy returned by access(user.name); and where to use model.name and where to use user.name itself? There are too many similar names and APIs now, and they are making it complicated IMO. Maybe the above is not the best "idea seller" example :)

[Lodin]

@taefi Oops, my bad. Insteaf of model there should be user everywhere

[Lodin]

Renamed local variables to make it easier to read.

function UserForm({ model: userModel }: UserFormProps)  {
  const { field, access } = useFormState(userModel);
  const nameState = access(userModel.name);
  const passwordState = access(userModel.password);

  return (
    <fieldset>
      <input data-testid="user.name" type="text" {...field(userModel.name)} />
      {nameState.invalid && <output data-testid="user.name.validation">{nameState.errors.map((e) => e.message).join(', ')}</output>}
      <input data-testid="user.password" type="text" {...field(userModel.password)} />
      {passwordState.invalid && (
        <output data-testid="user.password.validation">{passwordState.errors.map((e) => e.message).join(', ')}</output>
      )}
    </fieldset>
  );
}

[taefi]

@Lodin Thanks for the correction. Just to understand the logic behind the additional access element (or is it directive), is it kinda an additional layer to obtain the references of errors, invalid, value, setValue, ... which was available so far directly in the result of calling useForm or useFormState? Or is it providing any additional feature?

[platosha]

The point here is that instead of having two ways of accessing value and errors (directly or via state()), there is only one way using access().

[taefi]

If the access is completely hiding the existence of state I like the idea 👍 However, in the above sample code by @Lodin I got a little bit confused about the following part:

const { field, access } = useFormState(userModel); // <-- useForm or useFormState?
const nameState = access(userModel.name);
const passwordState = access(userModel.password);

Shouldn't it be useForm (at the marked line) when I try to get the field and access directives at the form level?

[Legioth]

Would UseFormResult still also contain e.g. an invalid property that reflects the status of the whole form?

[taefi]

As UseFormResult extends UseFormStateResult I would expect so. But, if I understood correctly, it is not accessible directly, but through calling that newly proposed access directive (which is not improving the DX IMO).

Btw, now that I'm thinking about it, I don't understand how should I access the errors or invalid at the form level. Is it like:

const { field, access } = useForm(userModel);
const formState = access(userModel); // this feels so redundant 

const isInvalid = formState.invalid;  
const errors = formState.errors;

I hope this to be just my misunderstanding :)

[platosha]

I'd suggest to keep separating useForm for the top-level form view components, and useFormState for subforms.

If we stick to the preference of avoiding multiple ways of accessing the state, then yes, it should all end up in access:

// top-level form
const { field, access, model } = useForm(UserModel);
const formInvalid = access(model).invalid;

// form section
const { field, access, model } = useFormState(addressModel);
const postalCodeInvalid = access(addressModel.postalCode).invalid;  

[sissbruecker]

Having to use access for (top) form-level state would hurt discoverability a lot. This is the stuff you most commonly want to get (value, invalid, dirty), it shouldn't be hidden behind a function call.

[platosha]

Additionally, useFormState would be confusing if that returns access, not the state.

[Lodin]

After a live discussion, we have reached the conclusion that we could go with the implementation we have now. It is a bit unusual but less weird than access/state functions suggested here. So, the final example should look like the following (considering the renaming useBinder -> useForm, useBinderNode -> useFormPart):

// Note that this function doesn't use `useForm` because it is a subform of a `LoginForm` component.
function UserForm({ model: user }: UserFormProps) {
  const { field } = useFormPart(user);
  const nameState = useFormPart(user.name);
  const passwordState = useFormPart(user.password);

  return (
    <fieldset>
      <input data-testid="user.name" type="text" {...field(user.name)} />
      {nameState.invalid && <output data-testid="user.name.validation">{nameState.ownErrors.map((e) => e.message).join(', ')}</output>}
      <input data-testid="user.password" type="text" {...field(user.password)} />
      {passwordState.invalid && (
        <output data-testid="user.password.validation">{passwordState.ownErrors.map((e) => e.message).join(', ')}</output>
      )}
    </fieldset>
  );
}