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

chore: remove useForm dependency from BasicToolForm #987

Conversation

ryanhopperlowe
Copy link
Contributor

@ryanhopperlowe ryanhopperlowe commented Dec 19, 2024

TLDR;
useForm returns useRef().current under the hood as a hack to optimize rendering. For the most part, this is totally fine, but it CAN cause some erratic behavior when certain conditions are met (I think it has to do with resetting the form's default values...????)

Either way, it added way more complexity than it was worth (at least for this component) so removing it was the best solution

More in-depth:
After mount, the first rerender (change event) of this specific usage of BasicToolForm was causing the form.watch method to change references (not only that but it was being treated as a NOOP??) as a result, the onChange prop was not getting triggered, but only for the first change. Subsequent rerenders did not reinitialize the watch method and it behaved as expected.

Looking into the code of react-hook-form there is no reasonable explanation for the reference of form.watch to change between renders.

Essentially what happens is useForm initializes a useRef object with the watch method and never updates it again. The only way that re-initialization could happen is if the parent component is remounted (tested and this is not the case).

The only thing that seemed off from within the useForm code is that it returns the current property of the previously mentioned useRef object. This technically breaks the rules of useRef (this one specifically) which "makes your component's behavior unpredictable"

In the interest of spending another 3+ hours on this topic, I'm sticking with this as the working diagnosis

useForm returns `useRef().current` under the hood as a hack to optimize rendering, which causes some erratic behavior when certain conditions are met (I think it has to do with resetting the form's default values...????)

Either way, it added way more complexity than it was worth (at least for this component) so removing it was the best solution
@ryanhopperlowe ryanhopperlowe merged commit 01885c2 into obot-platform:main Dec 19, 2024
2 checks passed
@ryanhopperlowe ryanhopperlowe deleted the chore/admin/remove-useForm-dependency-from-BasicToolForm branch December 19, 2024 20:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants