From c61955f145c6cfdbbd009f3303de94dfa743136c Mon Sep 17 00:00:00 2001 From: Crux Date: Thu, 3 Sep 2020 10:39:50 +0900 Subject: [PATCH 1/5] [EuiValidatableControl] Fixed ref not being handled properly when used with react-hook-form - Fix react-hook-form's reset() not working properly with EuiFieldText and more - See https://github.com/react-hook-form/react-hook-form/issues/2637 --- .../validatable_control.tsx | 24 +++++++++---------- 1 file changed, 11 insertions(+), 13 deletions(-) diff --git a/src/components/form/validatable_control/validatable_control.tsx b/src/components/form/validatable_control/validatable_control.tsx index 110b9e31d4e..eb8c12cc1e3 100644 --- a/src/components/form/validatable_control/validatable_control.tsx +++ b/src/components/form/validatable_control/validatable_control.tsx @@ -77,22 +77,20 @@ export class EuiValidatableControl extends Component< this.updateValidity(); } - setRef = (element: HTMLConstraintValidityElement) => { - this.control = element; - - // Call the original ref, if any - const { ref } = this.props.children; - if (typeof ref === 'function') { - ref(element); - } else if (isMutableRef(ref)) { - ref.current = element; - } - }; - render() { const child = Children.only(this.props.children); return cloneElement(child, { - ref: this.setRef, + ref: (element: HTMLConstraintValidityElement) => { + this.control = element; + + // Call the original ref, if any + const { ref } = this.props.children; + if (typeof ref === 'function') { + ref(element); + } else if (isMutableRef(ref)) { + ref.current = element; + } + }, }); } } From 257e98ebdfee9ebd157868673bb9749c911ce08b Mon Sep 17 00:00:00 2001 From: Crux Date: Thu, 3 Sep 2020 10:55:35 +0900 Subject: [PATCH 2/5] Updated changelog --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index a09bc0e29bb..e73fee462b7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,7 @@ - Fixed type mismatch between `EuiSelectable` options extended via `EuiSelectableOption` and internal option types ([#3983](https://github.com/elastic/eui/pull/3983)) - Fixed `EuiButton` CSS for RTL languages by using `margin-inline-[pos]` instead of `margin-[pos]` ([#3974](https://github.com/elastic/eui/pull/3974)) - Fix server-side rendering of `EuiBreadcrumbs` and `EuiCollapsibleNav` ([#3970](https://github.com/elastic/eui/pull/3970)) +- Fixed ref not being handled properly in `EuiValidatableControl` when used with [react-hook-form](https://react-hook-form.com/) ([#4001](https://github.com/elastic/eui/pull/4001)) ## [`28.3.0`](https://github.com/elastic/eui/tree/v28.3.0) From 8c403094d49b44c34321b1b6e4510e3a992f37e8 Mon Sep 17 00:00:00 2001 From: Crux Date: Fri, 4 Sep 2020 17:43:45 +0900 Subject: [PATCH 3/5] [EuiValidatableControl] Reimplemented using react hooks - Utilize useMemo() to prevent stable ref from being called on every render --- .../validatable_control.tsx | 72 +++++++++---------- 1 file changed, 35 insertions(+), 37 deletions(-) diff --git a/src/components/form/validatable_control/validatable_control.tsx b/src/components/form/validatable_control/validatable_control.tsx index eb8c12cc1e3..3b4ef3eeb49 100644 --- a/src/components/form/validatable_control/validatable_control.tsx +++ b/src/components/form/validatable_control/validatable_control.tsx @@ -20,10 +20,13 @@ import { Children, cloneElement, - Component, MutableRefObject, ReactElement, Ref, + FunctionComponent, + useRef, + useMemo, + useEffect, } from 'react'; import { CommonProps } from '../../common'; @@ -49,48 +52,43 @@ export interface EuiValidatableControlProps { children: ReactElementWithRef; } -export class EuiValidatableControl extends Component< - CommonProps & EuiValidatableControlProps -> { - private control?: HTMLConstraintValidityElement; +export const EuiValidatableControl: FunctionComponent = ({ isInvalid, children }) => { + const control = useRef(null); - updateValidity() { + const child = Children.only(children); + const childRef = child.ref; + + const replacedRef = useMemo( + () => (element: HTMLConstraintValidityElement) => { + control.current = element; + + // Call the original ref, if any + if (typeof childRef === 'function') { + childRef(element); + } else if (isMutableRef(childRef)) { + childRef.current = element; + } + }, + [childRef] + ); + + useEffect(() => { if ( - this.control == null || - typeof this.control.setCustomValidity !== 'function' + control.current === null || + typeof control.current.setCustomValidity !== 'function' ) { return; // jsdom doesn't polyfill this for the server-side } - if (this.props.isInvalid) { - this.control.setCustomValidity('Invalid'); + if (isInvalid) { + control.current.setCustomValidity('Invalid'); } else { - this.control.setCustomValidity(''); + control.current.setCustomValidity(''); } - } - - componentDidMount() { - this.updateValidity(); - } + }); - componentDidUpdate() { - this.updateValidity(); - } - - render() { - const child = Children.only(this.props.children); - return cloneElement(child, { - ref: (element: HTMLConstraintValidityElement) => { - this.control = element; - - // Call the original ref, if any - const { ref } = this.props.children; - if (typeof ref === 'function') { - ref(element); - } else if (isMutableRef(ref)) { - ref.current = element; - } - }, - }); - } -} + return cloneElement(child, { + ref: replacedRef, + }); +}; From d9c93b4ef59df3846fff54066f53cff1fb985b6d Mon Sep 17 00:00:00 2001 From: Crux Date: Mon, 7 Sep 2020 14:14:46 +0900 Subject: [PATCH 4/5] [EuiValidatableControl] Added more unit tests for ref management --- .../validatable_control.test.tsx | 102 ++++++++++++++++++ 1 file changed, 102 insertions(+) diff --git a/src/components/form/validatable_control/validatable_control.test.tsx b/src/components/form/validatable_control/validatable_control.test.tsx index 8ea3d87ca23..19c035a769d 100644 --- a/src/components/form/validatable_control/validatable_control.test.tsx +++ b/src/components/form/validatable_control/validatable_control.test.tsx @@ -61,5 +61,107 @@ describe('EuiValidatableControl', () => { expect(ref.current).not.toBeNull(); expect(ref.current!.getAttribute('id')).toBe('testInput'); }); + + it('calls stable ref function only once on re-render', async () => { + const ref = jest.fn(); + + const Component = () => ( + + + + ); + + const wrapper = mount(); + + expect(ref).toHaveBeenCalledTimes(1); + expect(ref.mock.calls[0][0].getAttribute('id')).toBe('testInput'); + + // Force re-render + wrapper.setProps({}); + + expect(ref).toHaveBeenCalledTimes(1); + expect(ref.mock.calls[0][0].getAttribute('id')).toBe('testInput'); + }); + + it('calls unstable ref function again on re-render', async () => { + const ref = jest.fn(); + + const Component = () => ( + + ref(el)} /> + + ); + + const wrapper = mount(); + + expect(ref).toHaveBeenCalledTimes(1); + expect(ref.mock.calls[0][0].getAttribute('id')).toBe('testInput'); + + // Force re-render + wrapper.setProps({}); + + expect(ref).toHaveBeenCalledTimes(3); + + expect(ref.mock.calls[1][0]).toBe(null); + expect(ref.mock.calls[2][0].getAttribute('id')).toBe('testInput'); + }); + + it('calls a ref function again when the child element changes', () => { + const ref = jest.fn(); + + const Component = ({ change }: { change: boolean }) => ( + + {!change ? ( + + ) : ( + + )} + + ); + + const wrapper = mount(); + + expect(ref).toHaveBeenCalledTimes(1); + expect(ref.mock.calls[0][0].getAttribute('id')).toBe('testInput'); + + wrapper.setProps({ change: true }); + + expect(ref).toHaveBeenCalledTimes(3); + + expect(ref.mock.calls[1][0]).toBe(null); + expect(ref.mock.calls[2][0].getAttribute('id')).toBe('testInput2'); + + // Ensure that the child element has changed + expect(ref.mock.calls[0][0]).not.toBe(ref.mock.calls[2][0]); + }); + + it('sets a ref object\'s "current" property when the child element changes', () => { + const ref = React.createRef(); + + const Component = ({ change }: { change: boolean }) => ( + + {!change ? ( + + ) : ( + + )} + + ); + + const wrapper = mount(); + + expect(ref.current).not.toBeNull(); + expect(ref.current!.getAttribute('id')).toBe('testInput'); + + const prevRef = ref.current; + + wrapper.setProps({ change: true }); + + expect(ref.current).not.toBeNull(); + expect(ref.current!.getAttribute('id')).toBe('testInput2'); + + // Ensure that the child element has changed + expect(ref.current).not.toBe(prevRef); + }); }); }); From 8e92d496490ac389a21d66e7db4108765cc5401e Mon Sep 17 00:00:00 2001 From: Crux Date: Wed, 9 Sep 2020 23:57:53 +0900 Subject: [PATCH 5/5] [EuiValidatableControl] Replace useMemo with useCallback --- .../form/validatable_control/validatable_control.tsx | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/components/form/validatable_control/validatable_control.tsx b/src/components/form/validatable_control/validatable_control.tsx index 3b4ef3eeb49..e1d3d4f6d06 100644 --- a/src/components/form/validatable_control/validatable_control.tsx +++ b/src/components/form/validatable_control/validatable_control.tsx @@ -25,8 +25,8 @@ import { Ref, FunctionComponent, useRef, - useMemo, useEffect, + useCallback, } from 'react'; import { CommonProps } from '../../common'; @@ -59,8 +59,8 @@ export const EuiValidatableControl: FunctionComponent (element: HTMLConstraintValidityElement) => { + const replacedRef = useCallback( + (element: HTMLConstraintValidityElement) => { control.current = element; // Call the original ref, if any