forge42dev / remix-hook-form

Open source wrapper for react-hook-form aimed at Remix.run
MIT License
330 stars 27 forks source link

Remix singleFetch and FieldError type #100

Open luzat opened 2 months ago

luzat commented 2 months ago

I have set up my Remix project to use unstable_singleFetch. This means that I am generally not using return json({ … }) (even though I sometimes could):

export const action = defineAction(async ({ request }) => {
  const { errors, data, receivedValues: defaultValues } = await getValidatedFormData<FormData>(request, resolver)

  if (errors) {
    return {
      errors, // <= error
      defaultValues,
    }
  }

  return {
    data,
  }
})

This gives a TypeScript error, because FieldError elements contains a ref?: Ref which cannot be serialized.

I think ref will always be undefined here, though, and getValidatedFormData's return type should probably reflect that. This may need to be solved for other non-hook methods, too. As a hack, I did this:

import type {
  BrowserNativeObject,
  DeepRequired,
  FieldError,
  FieldValues,
  GlobalError,
  IsAny,
  Merge,
} from 'react-hook-form'
import { getValidatedFormData, useRemixForm } from 'remix-hook-form'

// …

export type FieldErrorNoRef = FieldError & {
  root?: FieldErrorNoRef
  ref?: undefined
}

export type FieldErrorsNoRefImpl<T extends FieldValues = FieldValues> = {
  [K in keyof T]?: T[K] extends BrowserNativeObject | Blob
    ? FieldErrorNoRef
    : K extends 'root' | `root.${string}`
      ? GlobalError
      : T[K] extends object
        ? Merge<FieldErrorNoRef, FieldErrorsNoRefImpl<T[K]>>
        : FieldErrorNoRef
}

export type FieldErrorsNoRef<T extends FieldValues = FieldValues> = Partial<
  FieldValues extends IsAny<FieldValues> ? any : FieldErrorsNoRefImpl<DeepRequired<T>>
> & {
  root?: Record<string, GlobalError> & GlobalError
}

export const action = defineAction(async ({ request }) => {
  const { errors, data, receivedValues: defaultValues } = await getValidatedFormData<FormData>(request, resolver)

  if (errors) {
    return {
      errors: errors as FieldErrorsNoRef, // <= No error
      defaultValues,
    }
  }

  return {
    data,
  }
})

I am not sure if this solves every problem with singleFetch, but the cast avoids TypeScript errors. I will probably wrap all relevant methods for now, but it might suffice to define FieldErrorsNoRef (and the other relevant types) as above and change the cast in validateFormData:

return { errors: errors as FieldErrorsNoRef<T>, data: undefined };

This is obviously not very elegant as it mirrors the types from react-hook-form.

timvandam commented 2 months ago

running into this as well!

AlemTuzlak commented 3 weeks ago

I'll try to fix this asap, hopefully very soon

AlemTuzlak commented 2 weeks ago

@luzat what version of Remix is this on? Is it prior to 2.11.x? I don't get the same errors in the latest version, or I'm missing something?

luzat commented 2 weeks ago

@AlemTuzlak This was with Remix 2.10.0 I think (current package versions from bug report date), but I can still reproduce it with Remix 2.11.2 and current versions of remix-hook-form and react-hook-form. Here's a more minimal test:

import { zodResolver } from '@hookform/resolvers/zod'
import { unstable_defineAction as defineAction } from '@remix-run/node'
import { getValidatedFormData } from 'remix-hook-form'
import { z } from 'zod'

const schema = z.object({
  value: z.string(),
})

type FormData = Zod.infer<typeof schema>

const resolver = zodResolver(schema)

export const action = defineAction(async ({ request }) => {
  const { errors } = await getValidatedFormData<FormData>(request, resolver)
  return errors ? { errors } : {}
})

TypeScript error:

app/routes/test.tsx:14:36 - error TS2345: Argument of type '({ request }: ActionFunctionArgs) => Promise<{ errors: FieldErrors<{ value: string; }>; } | { errors?: undefined; }>' is not assignable to parameter of type 'Action'.
  Type 'Promise<{ errors: FieldErrors<{ value: string; }>; } | { errors?: undefined; }>' is not assignable to type 'MaybePromise<DataFunctionReturnValue>'.
    Type 'Promise<{ errors: FieldErrors<{ value: string; }>; } | { errors?: undefined; }>' is not assignable to type '{ [key: string]: Serializable; [key: number]: Serializable; [key: symbol]: Serializable; } | Map<Serializable, Serializable> | Set<...> | Promise<...> | Promise<...>'.
      Type 'Promise<{ errors: FieldErrors<{ value: string; }>; } | { errors?: undefined; }>' is not assignable to type 'Promise<Serializable>'.
        Type '{ errors: FieldErrors<{ value: string; }>; } | { errors?: undefined; }' is not assignable to type 'Serializable'.
          Type '{ errors: FieldErrors<{ value: string; }>; }' is not assignable to type 'Serializable'.
            Type '{ errors: FieldErrors<{ value: string; }>; }' is not assignable to type '{ [key: string]: Serializable; [key: number]: Serializable; [key: symbol]: Serializable; }'.
              Property 'errors' is incompatible with index signature.
                Type 'FieldErrors<{ value: string; }>' is not assignable to type 'Serializable'.
                  Type 'FieldErrors<{ value: string; }>' is not assignable to type '{ [key: string]: Serializable; [key: number]: Serializable; [key: symbol]: Serializable; }'.
                    Property 'value' is incompatible with index signature.
                      Type 'FieldError' is not assignable to type 'Serializable'.

14 export const action = defineAction(async ({ request }) => {
                                      ~~~~~~~~~~~~~~~~~~~~~~~~

With FieldError not being serializable because its ref could refer to some element according to its type. tsconfig.json contains the option:

{
  "compilerOptions": {
    "types": ["@remix-run/node", "vite/client", "@remix-run/react/future/single-fetch.d.ts"],
    …
  }
  …
}

Package versions are 2.11.2 for Remix, 5.0.4 for remix-hook-form, 7.53.0 for react-hook-form and 5.5.4 for TypeScript.

This is to be expected, given the FieldError type, which is returned by resolvers. One can remove defineAction to skip the type check completely. This is probably not too bad, because resolvers should in most cases return a type which is serializable by turbo-stream and useRemixForm doesn't care about the return type of the action method, but it'd be certainly cleaner to get a serializable type. Don't have any idea how to get that asserted by TypeScript more easily than with the redefined types and a cast I provided above, though.

AlemTuzlak commented 2 weeks ago

@luzat ahh okay the defineAction was the thing that was missing for me, FYI this API is completely removed in Remix on this PR: https://github.com/remix-run/remix/pull/9893 and they fixed some type-safety which might make this problem completely go away in the future release. I'll see if there is an easier way to exclude the ref without copy/pasting the internal types from react-hook-form in the meantime.

luzat commented 2 weeks ago

@AlemTuzlak Looking at this, it's certainly not that important to fix the types currently. If the types are to be improved in the future, it might make more sense to first include an option or helper in react-hook-form, to (optionally) return Ref-free types.

AlemTuzlak commented 2 weeks ago

I'm just a bit hesitant to add a fix for this now as there is a lot of API churn around single_fetch and there is no guarantee that this will be the case. I definitely plan to do something about it if it doesn't go away, until then your workaround will work for anyone on the unstable feature.