refinedev / refine

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

[BUG] useForm from @refinedev/react-hook-form race condition for setting values from query result #6182

Closed mjbergman92 closed 1 month ago

mjbergman92 commented 3 months ago

Describe the bug

The useEffect in the useForm hook from @refinedev/react-hook-form may run BEFORE all fields have been registered, especially if the queryResult is already cached.

Somehow the useEffect to call the setValue with the queryResult data needs to be called for values that may not be registered yet OR when registered, the useEffect needs to run again.

Steps To Reproduce

  1. useForm hook from @refinedev/react-hook-form
  2. action is edit mode
  3. go to edit page once
  4. return to list
  5. go to same edit page a second time, which should use the queryClient cache

Expected behavior

The field values from the query result should be set for all registered field values after rendering.

Packages

├─ @refinedev/cli@2.16.31 ├─ @refinedev/core@4.49.2 ├─ @refinedev/devtools-internal@1.1.9 ├─ @refinedev/devtools-server@1.1.29 ├─ @refinedev/devtools-shared@1.1.7 ├─ @refinedev/devtools@1.2.1 ├─ @refinedev/kbar@1.3.10 ├─ @refinedev/mui@5.19.0 │ └─ @refinedev/react-hook-form@4.8.20 ├─ @refinedev/react-hook-form@4.8.18 ├─ @refinedev/react-router-v6@4.5.9 ├─ @refinedev/react-table@5.6.12 ├─ @refinedev/simple-rest@5.0.6 └─ @refinedev/ui-types@1.22.9 ├─ @refinedev/core@4.53.0 ├─ @refinedev/devtools-internal@1.1.13 └─ @refinedev/devtools-shared@1.1.11

└─ react-hook-form@7.51.4

Additional Context

I'm going to have to come up with a work around for now. I havent came up with one yet that makes sense.

mjbergman92 commented 3 months ago

It may have something to do with the fact that I am using a Controller component...

I went ahead and supplied the defaultValue prop to the controller as queryResult?.data.data.description in my case and it is now working, but this is less than ideal IMO.

aliemir commented 3 months ago

Hey @mjbergman92, I think I understand the issue here. <Controller /> may register the field after the first run and our effect may miss it when we're setting the values. We only trigger the initial value set when the query result changes. This can be changed to also trigger the effect if the registered fields are updated (say a new one is added). This should be done carefully and only set values if the registered field doesn't have a value so we can make sure we're not breaking anything if user passes it manually through <Controller /> or similar. Maybe we can get all the registered fields

We can subscribe to changes using watch and concatenate field names and have a primitive string value which we can use to detect changes in the registered fields. AFAIK there's not much way to access or subscribe to the registered fields 🤔

mjbergman92 commented 3 months ago

@aliemir I was thinking through the same concerns. IMO, there's already quite a bit of "magic" behind the value getting populated simply because it was registered.

Again, my solution has been to set the default value of the Controller using the queryResult data from the refineCore prop returned from useForm, which actually makes sense when reading the code.

nbran commented 3 months ago

I'm seeing this issue as well and I'm not using Controller. Sometimes values are rendered and others they are not.

mjbergman92 commented 3 months ago

Ideally, react hook form would provide a callback when a field was registered, that way we could trigger our setValue call.

stale[bot] commented 1 month ago

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.