[EuiValidatableControl] Fixed ref not being handled properly when used with react-hook-form #4001
Conversation
…d with react-hook-form - Fix react-hook-form's reset() not working properly with EuiFieldText and more - See react-hook-form/react-hook-form#2637
|
Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually? |
There was a problem hiding this comment.
Thanks for the simple test case & sandbox proving out your change. Had to do a little digging on react-hook-form to understand why the change is necessary: in the hook version every render pass resets the library's knowledge of the form and creates a new ref handler, which EuiValidatableControl was ignoring. This has been a lingering bug in this component for some time.
The proposed change introduces some overhead where a stable ref (e.g. a static function) will be called with null & then the element every render, which could be an issue if the ref function is doing heavy work. This is an edge case and resolving it correctly in EuiValidatableControl would be challenging at best, while consuming applications should be able to mitigate any potential issues without much effort.
The code changes look good to me. One request though: let's add a couple more cases to the control's ref management unit tests to ensure:
- given two renders, where the child element changes (e.g. maybe EuiFieldText first and EuiFieldNumbere second)
- with ref function, we expect to receive the child instance, null, and the new child instance
- with a ref object (from e.g. useRef) we should see
ref.currentwith the first child and then after a render seeref.currentas the second child
- Utilize useMemo() to prevent stable ref from being called on every render
|
@chandlerprall I've reimplemented EuiValidatableControl as function component, leveraging useMemo() hook to prevent stable ref from being called on every render. https://codesandbox.io/s/white-haze-b5ucb I'll add some tests soon. |
|
@chandlerprall I've added the requested unit tests and more. Please take a look! |
|
jenkins test this |
|
@chandlerprall While reviewing the change, I found that |
|
Preview documentation changes for this PR: https://eui.elastic.co/pr_4001/ |
|
|
|
Preview documentation changes for this PR: https://eui.elastic.co/pr_4001/ |
|
Merged. Thanks @crux153 for describing & re-creating the issue, as well as providing the fix! |
|
|
|
@ince01 I guess you have two version of React running in your app. Checking your dependencies might help. |
|
@crux153 I only found 1 version of React in node_modules, I use React v17 in this project. Can u suggest me some other solutions? This project is a mono-repo with 2 packages, 1 common UI library wrap @elastic/eui as a base dependency, 1 app consume this common UI library |
|
|
|
@ince01 Hmm... this is weird, as I don't see any reason of this breaking rules of hooks. If you are passing ref to child of |
|
@crux153 I don't use ref, this is git repo of the project. You can check it. Follow these steps to start this project. I'm looking forward to ur help :(( |
Summary
Related to react-hook-form/react-hook-form#2637.
This fixes react-hook-form's reset() method not working properly with EuiFieldText and more.
It seems like passing ref function with reference equality doesn't makes it to be called again on form reset, which is the behavior that react-hook-form lib depends on.
This can be fixed by simply moving the ref handling function from arrow function in class property to inlined arrow function. Although this creates a new function every render, its performance impact is negligible and facebook/react#8873 (comment) does this too.
Demo
https://codesandbox.io/s/zealous-grothendieck-k3923
Clicking the reset button only sets value on
EuiCustomFieldText, notEuiFieldText.Checklist