refinedev / refine

A React Framework for building internal tools, admin panels, dashboards & B2B apps with unmatched flexibility.
https://refine.dev
MIT License
27.05k stars 2.08k forks source link

[BUG] Cannot use react-hook-form Controller with Refine's useForm #6139

Closed ritute closed 3 weeks ago

ritute commented 1 month ago

Describe the bug

I'm following the example for Material UI on this page: https://refine.dev/docs/packages/react-hook-form/introduction/#usage

My imports are:

import { useForm } from "@refinedev/react-hook-form";
import { Controller } from "react-hook-form";

The problem is that it looks we have to use the Edit component from Material UI in order to use Controller, otherwise I get the error:

React Router caught the following error during render TypeError: Cannot read properties of undefined (reading 'subscribe')

When I wrap my form within the Edit component, this error goes away, but I don't want to use Edit from material UI for my form. Is there a way around this? I'm currently having to use register instead, but I need to use some Material components like Autocomplete.

Steps To Reproduce

  1. Follow the steps in this documentation for Material UI: https://refine.dev/docs/packages/react-hook-form/introduction/#usage
  2. Remove the Edit component (don't wrap form with it)

Expected behavior

Shouldn't require the Edit component in order to use Controller in my form.

Packages

Additional Context

No response

aliemir commented 1 month ago

Hey @ritute, using a base Refine + Material UI + React Hook Form setup, I've followed the steps to repro the issue. Just copy-pasted the full Material UI code block and removed the <Edit /> component but could not reproduce. Can you try to share the broken code from your source or provide a minimal repro? 🙏

ritute commented 1 month ago

https://refine.dev/docs/packages/react-hook-form/introduction/#usage

Hmm, okay I just started with a base sandbox and don't see the issue either. Gonna try to figure out what's different then in my project's setup.

ritute commented 1 month ago

@aliemir here's a reproduction

The subscribe error only happens when use useForm from refine, not when using the one from react-hook-form. It actually doesn't work even with the Edit wrapped around in this case.

alicanerdurmaz commented 1 month ago

Hi @ritute,

I found some syntax problems in your project and fixed them, but I didn't see the issue you're facing. However, I did find another error:

  ➜  Local:   http://localhost:3000/
  ➜  Network: use --host to expose
  ➜  press h + enter to show help
✘ [ERROR] Could not resolve "@mui/material/DefaultPropsProvider"

    node_modules/@mui/lab/LoadingButton/LoadingButton.js:12:32:
      12 │ import { useDefaultProps } from '@mui/material/DefaultPropsProvider';
         ╵                                 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

  You can mark the path "@mui/material/DefaultPropsProvider" as external to exclude it from the
  bundle, which will remove this error and leave the unresolved path in the bundle.

/Users/alicanerdurmaz/Desktop/refine-form-demo/node_modules/esbuild/lib/main.js:1651
  let error = new Error(text);
              ^

Error: Build failed with 1 error:
node_modules/@mui/lab/LoadingButton/LoadingButton.js:12:32: ERROR: Could not resolve "@mui/material/DefaultPropsProvider"
    at failureErrorWithLog (/Users/alicanerdurmaz/Desktop/refine-form-demo/node_modules/esbuild/lib/main.js:1651:15)
    at /Users/alicanerdurmaz/Desktop/refine-form-demo/node_modules/esbuild/lib/main.js:1059:25
    at /Users/alicanerdurmaz/Desktop/refine-form-demo/node_modules/esbuild/lib/main.js:1527:9
    at process.processTicksAndRejections (node:internal/process/task_queues:95:5) {
  errors: [Getter/Setter],
  warnings: [Getter/Setter]
}

Node.js v20.11.0

This error was resolved when I updated @mui/material to the latest version.

Could you please recheck the project you shared? I couldn't reproduce the error you got.

ritute commented 1 month ago

@alicanerdurmaz @aliemir fixed the syntax error for the commented Edit wrapper, my bad. I don't get that MUI error. Can you make sure you're using the yarn lock file that's there with yarn install? I just pulled the repo from scratch and ran into the issue I posted after a yarn install. It appears in the browser, not in the terminal output. Here's a screenshot.

Screen Shot 2024-07-18 at 12 19 41 PM
aliemir commented 1 month ago

Hey @ritute thank you for the update. I cloned the repo and used yarn v1 to install. Now I can reproduce the same issue 🙌

I'll try to investigate on what caused the issue but when I replace the react-hook-form dependency version from 7.30.0 to ^7.30.0 and that installed 7.52.1 (latest) and the error is gone and rendering is done properly. 🚀

Can you check if updating the dependency also works for you? Do you have any limitations to keep the react-hook-form pinned at 7.30.0? 🤔

ritute commented 1 month ago

@aliemir I noticed that the Refine library was using this version 7.30.0 for react-hook-form, so I decided to go with that one. But I don't need to, I'll try updating it, thanks! If the root cause can't be fixed, would be great to update the docs to ensure people don't use that version 👍

ritute commented 1 month ago

@aliemir it worked with version 7.52.1 (but needed to add it to resolutions since Refine is using 7.30.0)

aliemir commented 1 month ago

The version bump of react-hook-form to ^7.30.0 is done when we're moving to React 18. react-hook-form added React 18 support with v7.29.0.

I found the issue in the react-hook-form package, which was resolved with PR https://github.com/react-hook-form/react-hook-form/pull/10052 and released with 7.43.5. I didn't check our code for the useForm but looks like one of the changes made since the last year doesn't work with versions prior 7.43.5 🤔

Happy to see the issue is resolved after the update. Since we've found the exact version that resolves the issue, I think we can mark our react-hook-form dependency as ^7.43..5 🙌

aliemir commented 1 month ago

If anyone is interested on working on the fix, it should be as simple as replacing all "react-hook-form": "^7.30.0" with "react-hook-form": "^7.43.5" in the repo. @ritute I can assign this to you if you can work on it. We'll be happy to see your PR 🙏

ritute commented 1 month ago

Okay sure I can do that. But hmm something doesn’t make sense as when I used the older version of react-hook-form I didn’t run into this runtime error when using the useForm from their package vs Refine’s. You can try that repo and just instead of using Refine’s useForm, use the one from react-hook-form.

On Fri, Jul 19, 2024 at 2:46 AM Ali Emir Şen @.***> wrote:

If anyone is interested on working on the fix, it should be as simple as replacing all "react-hook-form": "^7.30.0" with "react-hook-form": "^7.43.5" in the repo. @ritute https://github.com/ritute I can assign this to you if you can work on it. We'll be happy to see your PR 🙏

— Reply to this email directly, view it on GitHub https://github.com/refinedev/refine/issues/6139#issuecomment-2238462536, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAFVZ45UXJLHS4JKOJENBJDZNCY5TAVCNFSM6AAAAABK3OY3Y6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDEMZYGQ3DENJTGY . You are receiving this because you were mentioned.Message ID: @.***>

ritute commented 1 month ago

@aliemir is there any reason you guys don't expose Controller in your @refinedev/react-hook-form package, so that we don't need those two separate dependencies?

aliemir commented 1 month ago

@ritute, @refinedev/react-hook-form is an extension package that extends useForm of @refinedev/core with abilities of react-hook-form. We try to keep our packages clean and don't export anything that we don't modify/extend. We take the useForm from react-hook-form and add all existing features of @refinedev/core's useForm such as API interactions, error propagation, redirections, invalidation etc.

For the previous question, check out the source code for the useForm from @refinedev/react-hook-form:

https://github.com/refinedev/refine/blob/master/packages/react-hook-form/src/useForm/index.ts

We're not just using or re-implementing the useForm hook from react-hook-form but we're extending it. Looks like watch (useSubscribe under the hood -the one causing the errors-) is used for notifying the user on unsaved changes and for the auto-save feature. Since those are not present in react-hook-form's useForm, we don't get any errors if we use it directly.

I hope I clarified the difference and the reason behind two packages and two hooks ✌️

ritute commented 1 month ago

@aliemir getting this when running pnpm install in latest master.

Screen Shot 2024-07-19 at 12 59 12 PM
aliemir commented 1 month ago

@ritute I checked but couldn't find the same version code. Can you check blog-material-ui-datagrid/package.json to see if there's a broken version tag in react dependency?

ritute commented 1 month ago

@aliemir ah, oh my, I somehow added that... 😂. All good! BTW, there are several tests that fail in master for me - looks like a timezone related issue.

Screen Shot 2024-07-19 at 2 11 55 PM Screen Shot 2024-07-19 at 2 11 40 PM
ritute commented 1 month ago

https://github.com/refinedev/refine/pull/6161