fabian-hiller / modular-forms

The modular and type-safe form library for SolidJS, Qwik and Preact
https://modularforms.dev
MIT License
1.05k stars 55 forks source link

Question: Creating components for subforms #51

Open lars-berger opened 1 year ago

lars-berger commented 1 year ago

Mentioned in the SolidJS Discord earlier and was recommended to post about it here.

I'm trying to figure out a good way to separate out a reusable component for an X/Y position form field, something like so:

type MyForm = {
  position: {
    x: number
    y: number
  }
};

// template:
<Field of={myForm} name="position"
  {field => (
    // where `PositionInput` consists of 2 text inputs: one for x and another for y
    <PositionInput {...field.props} value={field.value} />
  )}
</Field>

But with the above, I don't see how the onBlur, onInput, and onChange callbacks could be passed to 2 <input> elements.

So I've been trying an alternative approach, but here I'm unsure how to make the TS types behave correctly:

// Parent:
<Form of={myForm}>
  <PositionField of={myForm} name="position" />
</Form>

// PositionField:
export function PositionField<T>(props: { of: FormState<T>, name: string }) {
  return (
    <Field of={props.of} name={`${props.name}.x`}> // throws: Type '`${string}.x`' is not assignable to type 'ValuePaths<T>'
      {field => <input {...field.props) value={field.value} />}
    </Field>
    <Field of={props.of} name={`${props.name}.y`}>
      {field => <input {...field.props) value={field.value} />}
    </Field>
  )
}

In short, is there a recommendation on how to create reusable subforms?

fabian-hiller commented 1 year ago

What you have in mind works, but is challenging to implement in a fully type-safe way. I am currently reworking the SolidJS version of Modular Forms and hope that it will be possible without typecasting afterwards.

Basically, I would always try to keep the Field component in the form and not outsource it to a separate component.

// components/Position.tsx

import { JSX } from 'solid-js';

export function Root(props: { children: JSX.Element }) {
  // Add root HTML and CSS here
  return <div>{props.children}</div>;
}

export function Input(props: JSX.InputHTMLAttributes<HTMLInputElement>) {
  // Add input HTML and CSS here
  return <input {...props} />;
}

// routes/your-page.tsx

import { createForm, Field, Form } from '@modular-forms/solid';
import Position from '~/components/Position';

type PositionForm = {
  heading: string;
  position: {
    x: number;
    y: number;
  };
};

export default function YourPage() {
  // Create position form
  const positionForm = createForm<PositionForm>();

  return (
    <Form of={positionForm} onSubmit={() => {}}>
      <Field of={positionForm} name="heading">
        {(field) => <input {...field.props} value={field.value} />}
      </Field>
      <Position.Root>
        <Field of={positionForm} name="position.x">
          {(field) => <Position.Input {...field.props} value={field.value} />}
        </Field>
        <Field of={positionForm} name="position.y">
          {(field) => <Position.Input {...field.props} value={field.value} />}
        </Field>
      </Position.Root>
      <input type="submit" />
    </Form>
  );
}

If you absolutely want to move the Field to another component, the following code can be used if the key to the coordinates is always position.

// components/PositionInput.tsx

import { Field, FieldPath, FormState } from '@modular-forms/solid';

type PositionValues = {
  position: { x: number; y: number };
};

type PositionInputProps<TPositionValues extends PositionValues> = {
  of: FormState<TPositionValues>;
};

export default function PositionInput<TPositionValues extends PositionValues>(
  props: PositionInputProps<TPositionValues>
) {
  return (
    <div>
      <Field of={props.of} name={'position.x' as FieldPath<TPositionValues>}>
        {(field) => <input {...field.props} value={field.value} />}
      </Field>
      <Field of={props.of} name={'position.x' as FieldPath<TPositionValues>}>
        {(field) => <input {...field.props} value={field.value} />}
      </Field>
    </div>
  );
}

// routes/your-page.tsx

import { createForm, Field, Form } from '@modular-forms/solid';
import PositionInput from '~/components/PositionInput';

type PositionForm = {
  heading: string;
  position: {
    x: number;
    y: number;
  };
};

export default function YourPage() {
  // Create position form
  const positionForm = createForm<PositionForm>();

  return (
    <Form of={positionForm} onSubmit={() => {}}>
      <Field of={positionForm} name="heading">
        {(field) => <input {...field.props} value={field.value} />}
      </Field>
      <PositionInput of={positionForm} />
      <input type="submit" />
    </Form>
  );
}

If the key is to be dynamic, it gets a bit more complicated. Unfortunately, I have not found a satisfactory solution yet.

lars-berger commented 1 year ago

@fabian-hiller Thanks for the quick response, gonna go with the type coercion approach for now. I've got a few reusable subforms that have 4+ inputs, so it's pretty tedious to keep all the fields within the parent form. It'd be super nice to have it be type safe in the future.

Do you think something like this might work? Couldn't get to a concrete implementation.

export type PositionPropertyForm = {
  x: number;
  y: number;
};

export type WithNestedKey = ?? // wraps and overrides deeply nested key with value

export interface PositionPropertyInputProps<
  TVal extends FieldValues,
  TKey extends FieldPath<TVal>,
  TForm extends WithNestedKey<TVal, TKey, PositionPropertyForm>,
> {
  of: FormState<TForm>;
  name: FieldPath<TForm>;
}

export function PositionPropertyInput<
  TVal extends FieldValues,
  TKey extends FieldPath<TVal>,
  TForm extends WithNestedKey<TVal, TKey, PositionPropertyForm>,
>(props: PositionPropertyInputProps<TVal, TKey, TForm>) {
  ...
}
fabian-hiller commented 1 year ago

I tried exactly this implementation, but was only partially successful. Let's exchange again here in about 2 weeks, once I have revised the SolidJS version.

lars-berger commented 1 year ago

Sounds good :raised_hands:

lars-berger commented 1 year ago

Throwing out another suggestion

Could perhaps allow objects as field values, like so:

// ================
// parent form:
type MyForm = {
  position: {
    x: number
    y: number
  }
};

<Field of={myForm} name="position">
  // ^ rather than erroring here, allow an incomplete path - in which case, `field.value` becomes an object
  {field => (
    <PositionInput onBlur={field.props.onBlur} onChange={field.props.onChange} value={field.value} />
  )}
</Field>

// ================
// custom form input:
export type PositionInputValue = {
  x: number;
  y: number;
};

export type PositionInputProps = {
  onBlur: () => void;
  onChange: (val: PositionInputValue) => void;
  value: PositionInputValue;
};

export function PositionInput(props: PositionInputProps) {
  return (
    // explicitly call `props.onBlur` and `props.onChange`
  )
}

After some thought, this approach would be better IMO compared to custom field components like above. This means the <Field> could be kept in the parent form and the form field wouldn't have to directly map to a single <input />.

It'd also make it possible to bind custom inputs that might not even have an <input /> element either. Like say you have a date range selector that opens up a modal to select 2 different dates (ie. there is no text input, select, etc).

fabian-hiller commented 1 year ago

This does not work with the implementation of Modular Forms. Modular Forms keeps the state of a field under its name. In your case, Modular Forms maps the fields internally as follows:

form.internal.fields = {
  'position.x': {…},
  'position.y': {…},
};

This means that the entire name must always be available to access the state of a field. I deliberately chose this implementation because it has several advantages.

const positionX = form.internal.fields['position.x'];
lars-berger commented 1 year ago

Would it be possible to additionally store incomplete fieldpaths? Eg

form.internal.fields = {
  'position': {…},
  'position.x': {…},
  'position.y': {…},
};
fabian-hiller commented 1 year ago

I don't think so, since the content and data processing would then have to be quite different than from a direct field. This could make the code chaotic and increases the bundle size.

lars-berger commented 1 year ago

If form.internal.fields is a SolidJS store, can you have an effect that updates the direct fields on changes to the partial fieldpaths?

IMO this issue severely limits extensibility of the library. Eg. how would you wire up a UI component for a date range selector that has a value with a startDate and endDate? Like say it has the following API:

export interface DateRangeValue {
  startDate: Date;
  endDate: Date;
}

export interface ThirdPartyDateRangeSelectorProps {
  onBlur: () => void;
  onChange: (val: DateRangeValue) => void;
  initialValue: DateRangeValue;
}

export function ThirdPartyDateRangeSelector(props: ThirdPartyDateRangeSelectorProps) { ... }

From my understanding, you simply wouldn't be able to use a component like this with modular-forms. If modular-forms is only meant to be used with native input and select elements (and only where a field maps directly to a single input or select element), then that should be clarified. Having only some form state managed via modular-forms is a pain, especially in complex form structures (eg. array fields within array fields).

For example, Angular forms lets you bind a field to any component that implements ControlValueAccessor. It passes down callbacks for when the field is touched/changed and you just gotta call them wherever appropriate.

fabian-hiller commented 1 year ago

With the various methods like setValue that Modular Forms provides, you can control the state of a field yourself. That is, you don't have to bind it to an HTML element. This should make it possible to use Modular Forms with UI components that do not contain <input />, <textarea /> or <select /> elements.

I understand the problem and will try to find a solution for it. My approach so far is not to include Modular Forms in your own components like PositionInput or DateRangeSelector but to use the library as a data layer for the properties of these components. This way these components remain independent from a form library and the whole form handling is located directly in the form and is not spread over several components.

One more idea that came to me is to nest the Field component to be able to access the state of two fields within:

<Field name="position.x">
  {(...x) => (
    <Field name="position.y">
      {(...y) => (
        <PositionInput x={x} y={y} />
      )}
    </Field>
  )}
</Field>

Since the code and DX is not so sexy here, it might make sense to provide a separate component to access the state of multiple fields:

<Fields names={['position.x', 'position.y']}>
  {(x, y) => <PositionInput x={x} y={y} />}
</Fields>

However, this is only a first thought and not yet thought through to the end. It is important that we ask ourselves the question, what would be the best DX and how would the API of Modular Forms look like for it. Feel free to make suggestions if you have ideas.

lars-berger commented 1 year ago

@fabian-hiller The <Fields> looks component good, that's pretty clever 👍

Could the consuming application then do something like below?

function mergeOnBlur(...fields) {
  return (event) => {
    fields.forEach(field => field.props.onBlur(event));
  }
}

function mergeOnChanges(...fields) {
  return (event) => {
    field.forEach(field => {
      if (event.target.value[field.name]) {
        // ^ custom input component needs to emit event with value `{ x: number; y: number }`
        field.props.onChange(event)
      }
    });
  }
}

// ===============
// form component:
<Fields names={['position.x', 'position.y']}>
  {(x, y) => {
    <PositionInput onBlur={mergeOnBlur(x, y)} onChanges={mergeOnChanges(x, y)} />
  })
</Fields>

// ===============
// input component:
export interface PositionInputProps {
  onBlur: (e) => void;
  onChange: (e) => void;
}

export function PositionInput(props: PositionInputProps) { ... }

The reason being is that it seems like it'd be awkward to manage 2 different sets of field props (ie. onBlur, onChange, etc.). Most 3rd party components with object value types (eg. a date range selector with value { minDate: string; maxDate: string }) are usually not designed to take in onBlur, onChange, etc. props for each of its keys (ie. minDate, maxDate).

lars-berger commented 1 year ago

To be perfectly honest though, allowing partial field paths still seems like it'd be good DX.

Came across a project a few days ago called hotscript and immediately thought that maybe it'd be useful for modular-forms.

This is one of the helpers, Objects.get:

image

fabian-hiller commented 1 year ago

Thank you very much for the info. Currently I am very busy. As soon as I have some more time, I will address the problem. I think that a solution is important for the future of the library.

lars-berger commented 1 year ago

Alright, I at least appreciate that the issue is in consideration 👍

fabian-hiller commented 1 year ago

Now that I have revised the SolidJS version and also improved the types, I will try to work out different solutions for this problem.

fabian-hiller commented 1 year ago

I would like to give a brief update on this issue. I still think it is important to find a better solution for this problem. Unfortunately I haven't found the time to investigate the problem further.

Due to the different properties of the Field component like validate and transform I think a Fields component (to access multiple fields) is currently not a good idea, because the developer experience would be chaotic. Until I get back here, I think the best workaround is to nest the Field component.

<Field name="position.x">
  {(...x) => (
    <Field name="position.y">
      {(...y) => (
        <PositionInput x={x} y={y} />
      )}
    </Field>
  )}
</Field>

If anyone has any other ideas to fix this problem or improve the DX, I welcome inspiration.