Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[FEAT] Improving Type Parameter Specification in useForm Hook #6137

Open
noritsune opened this issue Jul 13, 2024 · 7 comments · May be fixed by #6568
Open

[FEAT] Improving Type Parameter Specification in useForm Hook #6137

noritsune opened this issue Jul 13, 2024 · 7 comments · May be fixed by #6568
Labels
enhancement New feature or request

Comments

@noritsune
Copy link
Contributor

Is your feature request related to a problem? Please describe.

I'm developing a user information editing page using Refine, and I'm facing difficulties in clearly distinguishing between the form structure and the data type submitted to the DataProvider.

If you know of some way, please let me know.

Describe alternatives you've considered

If I don't specify types in the useForm function, this issue doesn't occur. However, that would negate the purpose of using TypeScript.

Additional context

Current Status and Investigation

I've extensively reviewed documentation and advanced tutorials on how to specify useForm's type parameters effectively, but haven't found a solution yet.

Context

Here is the project setup and code where I encountered the issue.

Framework Structure

Routing: Next.js (App Router)
Data Provider: SimpleRest
UI: Material UI

The Flow of Data

DataFlow

Problematic Code

I think it's beneficial that the FormProps type specified as the third type parameter of useForm is used by the reset and handleSubmit functions. This is because they rely on the form structure.

However, the same type is also enforced for the argument of the onFinish function. This causes a type error when trying to convert the FormProps type to the User type inside the handleSubmitInner function during submission.

"use client";

import { HttpError } from "@refinedev/core";
import { Edit } from "@refinedev/mui";
import { useForm } from "@refinedev/react-hook-form";
import { useEffect } from "react";

interface User {
  email: string;
  user_metadata: {
    display_name: string;
  };
}

interface FormProps {
  email: string;
  userName: string;
}

export default function UserEdit() {
  const {
    refineCore: { queryResult, onFinish },
    reset,
    handleSubmit,
  } = useForm<User, HttpError, FormProps>();

  const user = queryResult?.data?.data;

  // Called when the page is loaded
  useEffect(() => {
    // Map the user information received from the API to the form
    // Calling reset sets the values entered in the form all at once
    reset({
      email: user?.email,
      userName: user?.user_metadata?.display_name,
    });
  }, [reset, user]);

  // Called when submitting the form
  const handleSubmitInner = (formProps: FormProps) => {
    // Map the information entered in the form to the API
    onFinish({
      email: formProps.email,
      // A type error is output at this line.
      // This is because the argument type of the onFinish function becomes FormProps,
      // which is specified as the third type parameter of useForm.
      user_metadata: {
        display_name: formProps.userName,
      },
    });
  };

  return (
    <Edit
      saveButtonProps={{
        onClick: handleSubmit(handleSubmitInner),
      }}
    >
      {/* ... */}
    </Edit>
  );
}

Describe the thing to improve

To resolve this issue, consider adding an additional type parameter like SubmissionData to useForm. This would clarify distinctions between form structure and submission data types, ensuring a smoother TypeScript-driven development process.

"use client";

import { HttpError } from "@refinedev/core";
import { Edit } from "@refinedev/mui";
import { useForm } from "@refinedev/react-hook-form";
import { useEffect } from "react";

interface User {
  email: string;
  user_metadata: {
    display_name: string;
  };
}

interface FormProps {
  email: string;
  userName: string;
}

export default function UserEdit() {
  const {
    refineCore: { queryResult, onFinish },
    reset,
    handleSubmit,
    // The 4th type parameter is the type of the submission.
    // It will be a argument of onFinish, so it should be User.
  } = useForm<User, HttpError, FormProps, User>();

  const user = queryResult?.data?.data;

  // Called when the page is loaded
  useEffect(() => {
    // Map the user information received from the API to the form
    // Calling reset sets the values entered in the form all at once
    reset({
      email: user?.email,
      userName: user?.user_metadata?.display_name,
    });
  }, [reset, user]);

  // Called when submitting the form
  const handleSubmitInner = (formProps: FormProps) => {
    // Map the information entered in the form to the API
    onFinish({
      email: formProps.email,
      user_metadata: {
        display_name: formProps.userName,
      },
    });
  };

  return (
    <Edit
      saveButtonProps={{
        onClick: handleSubmit(handleSubmitInner),
      }}
    >
      {/* ... */}
    </Edit>
  );
}
@noritsune noritsune added the enhancement New feature or request label Jul 13, 2024
@aliemir
Copy link
Member

aliemir commented Jul 16, 2024

Hey @noritsune thank you for the issue! I think I understand the problem here. While the useForm hook from @refinedev/core exposes an onFinish handler to submit values and doesn't integrate with forms automatically, having a single generic TVariables is enough but when we extend the useForm with form libraries (such as react-hook-form) it's understandable to require two generics, lets say TVariables and TSubmission, one is working with the form elements and other can be used to type onSubmit function to make sure the specs are met.

The 4th generic can be the same with the 3rd by default and it will also only trigger errors if onSubmit (or equivalent) is used manually. We can use type casting to prevent errors internally and assume users know what they're doing when they provide the 4th generic 🤔

Do I understand it correctly?

BTW, as a workaround I remember that one time I used a union type as 3rd that matches both the form and submission type and handled types in the custom onFinish/handleSubmit handler but having an option of a 4th generic looks better to me 😅

@noritsune
Copy link
Contributor Author

@aliemir Thank you for your response. I think your understanding is correct. Also, I've mostly grasped what you're saying. So, are you suggesting that useForm should allow specifying the type of the onFinish function's argument (i.e., the type of data sent to the DataProvider) as the 4th type option?

Copy link

stale bot commented Sep 16, 2024

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the wontfix This will not be worked on label Sep 16, 2024
@aliemir aliemir removed the wontfix This will not be worked on label Sep 17, 2024
Copy link

stale bot commented Nov 16, 2024

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the wontfix This will not be worked on label Nov 16, 2024
@BatuhanW BatuhanW removed the wontfix This will not be worked on label Nov 20, 2024
@chhaysotheara
Copy link

+1

It's really hard to use TS without proper typing in here.
Also, thanks to @aliemir for the workaround.

I think it's better to use union type rather than "any"

Hopefully we can get an enhancement on this soon.

@aliemir
Copy link
Member

aliemir commented Nov 25, 2024

@chhaysotheara thank you for bumping the issue 😅 I'm leaving this comment as an implementation guide that we can follow when working on this feature.

Using a union type for TVariable works but doesn't enforce anyone of the types and can allow corrupted values. For now this can be used as a workaround.

As I mentioned earlier, one more generic can be added but even now the generics are too crowded imo. Instead, now I'm more into having utility types to manage two of the generics in one. This will also allow us to continue accepting TVariables like now without any issues (we don't always need to customize them in handleFinish or onFinish).

We can create one utility type CreateFormVariables<TForm, TSubmission> like below:

// using symbols to avoid conflicting with the basic TVariables shape
const FormVariablesSymbol = Symbol("FormVariables");
const SubmissionVariablesSymbol = Symbol("SubmissionVariables");

export type CreateFormVariables<TFormVariables, TSubmissionVariables> = {
  [FormVariablesSymbol]: TFormVariables;
  [SubmissionVariablesSymbol]: TSubmissionVariables;
};

Then we can allow passing the TVariables (3rd) generic as an object or return type of CreateFormVariables, then we'll extract the appropriate types via something like below:

type ExtractFormVariables<T> = T extends { [FormVariablesSymbol]: infer F } ? F : T;
type ExtractSubmissionVariables<T> = T extends { [SubmissionVariablesSymbol]: infer S; } ? S : T;

Then in the implementation we can use those types and achieve this:

type Data = { id: number; name: string; date: string; }
type FormValues = { name: string; }
type SubmissionValues = { name: string; date: Date; }

const { handleSubmit, refineCore: { onFinish } } = useForm<
  Data,
  HttpError,
  CreateFormVariables<FormValues, SubmissionValues>
>({ ... });

// `values` will be in `FormValues` type
handleSubmit((values) => {
  // `onFinish` will expect the argument to be in `SubmissionValues` type and will error if it doesn't match.
  onFinish({ ...values, date: new Date() });
});

As you can see from the extraction utilities, current implementation is also supported with no changes:

type Data = { id: number; name: string; }
type FormValues = { name: string; }

const { handleSubmit, refineCore: { onFinish } } = useForm<
  Data,
  HttpError,
  /// this will continue working as expected
  FormValues
  // or
  // FormValues | SubmissionValues
  // will continue working as is
>({ ... });

CreateFormVariables, ExtractFormVariables and ExtractSubmissionVariables can be exported from @refinedev/core and can be used in the related form packages (@refinedev/antd, @refinedev/react-hook-form and @refinedev/mantine).

Let us know what are your thoughts on this. We can include this in our roadmap for the upcoming improvements and features to Refine. We're currently at planning stage for this. Or if anyone is interested in working on this issue, they can use this comment as a guide. We'll happy to help to the contributors 🚀 🚀

@OmkarBansod02
Copy link
Contributor

hey @aliemir I’ve read through the discussion and I’d love to take on this issue. Thank you for the detailed implementation guide. I will go over it and fix it soon 🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants