Shopify / quilt

A loosely related set of packages for JavaScript/TypeScript projects at Shopify
MIT License
1.7k stars 220 forks source link

@shopify/react-form: types defined to capture self intentionally? #1936

Open dgattey opened 3 years ago

dgattey commented 3 years ago

Overview

Thanks for a great set of packages! I started using @shopify/react-form today with Polaris. Works really well for our use case, but we have a small problem with types.

Our Typescript + ESLint config has @typescript-eslint/unbound-method on by default. For some reason, this code causes that rule to trigger - just the use of useForm and passing formFields and onSubmit into it is enough. Most of this is just setup code to show how we're using useForm - the real meat is the hook call itself that triggers the warning.

import { Form as PolarisForm } from '@shopify/polaris';
import { FieldBag, FormMapping, SubmitHandler, useForm } from '@shopify/react-form';

type Props = Omit<React.ComponentPropsWithoutRef<typeof PolarisForm>, 'onSubmit'> & {
    // The parent of this form defines the fields
    formFields: FieldBag;

    // Swaps the normal onSubmit handler for this type instead
    onSubmit: SubmitHandler<FormMapping<FieldBag, 'value'>>;
};

interface FormRef {
    fields: FieldBag;
};

const Form = React.forwardRef<FormRef, Props>(
  ({ formFields, onSubmit, children }: Props, ref): React.ReactElement => {

    // HERE IT IS - we're not capturing this, but there's still a warning here
    const { submit, submitting, submitErrors, fields } = useForm({
      fields: formFields,
      onSubmit,
    });

    // What's below here doesn't matter
   ...
    return <PolarisForm ...>{children} ...</PolarisForm>
})

I think the issue stems from this definition for Form and any like it, because you don't use arrow types, thus capturing this, instead of something like validate: () => FormError[]. Via https://stackoverflow.com/questions/62864908/how-to-prevent-typescript-eslint-unbound-method-errors-on-non-class-typescrip, that seems to be a common cause of this ESLint rule trigger.

export interface Form<T extends FieldBag> {
    fields: T;
    dirty: boolean;
    submitting: boolean;
    submitErrors: FormError[];
    validate(): FormError[];
    reset(): void;
    submit(event?: React.FormEvent): void;
    makeClean(): void;
}

Would it be possible to change the type definitions to not capture self, unless there's a reason to be capturing self in those contexts? Thanks a ton!

Consuming repo

(Private source repo), but the above is enough to trigger the rule

Area

Scope


Checklist

Chadyka commented 1 year ago

+1 I'm trying to use this package without Polaris as a replacement for react-hook-form and I'm facing the same problem too. Did you find any workarounds in the meantime? Apart from the obvious advice of turning @typescript-eslint/unbound-method off.