diff --git a/CHANGELOG.md b/CHANGELOG.md index f9f017061fd..9ca976bb352 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,6 +11,7 @@ - Fixed `EuiIcon` accessibility by adding a `title` prop and a default `aria-label` ([#2554](https://github.com/elastic/eui/pull/2554)) - Fixed `EuiDataGrid`'s in-memory sorting of numeric columns when the cell data contains multiple digit groups ([#2603](https://github.com/elastic/eui/pull/2603)) - Improved pagination in `EuiBasicTable`. `paginationBar` is hidden when there is no data and `EuiPagination` is displayed even when there is only one page ([#2598](https://github.com/elastic/eui/pull/#2598)) +- Fixed react-dom warning when `EuiPopover` was unmounted before calls to setState ([#2614](https://github.com/elastic/eui/pull/2614)) ## [`17.0.0`](https://github.com/elastic/eui/tree/v17.0.0) diff --git a/src/components/popover/popover.test.tsx b/src/components/popover/popover.test.tsx index 354370cbdd3..0a5950eec68 100644 --- a/src/components/popover/popover.test.tsx +++ b/src/components/popover/popover.test.tsx @@ -254,6 +254,64 @@ describe('EuiPopover', () => { }); }); }); + + describe('listener cleanup', () => { + let _raf: typeof window['requestAnimationFrame']; + let _caf: typeof window['cancelAnimationFrame']; + beforeAll(() => { + jest.useFakeTimers(); + _raf = window.requestAnimationFrame; + _caf = window.cancelAnimationFrame; + + const activeAnimationFrames = new Map(); + let nextAnimationFrameId = 0; + window.requestAnimationFrame = fn => { + const animationFrameId = nextAnimationFrameId++; + activeAnimationFrames.set(animationFrameId, setTimeout(fn)); + return animationFrameId; + }; + window.cancelAnimationFrame = (id: number) => { + const timeoutId = activeAnimationFrames.get(id); + if (timeoutId) { + clearTimeout(timeoutId); + activeAnimationFrames.delete(id); + } + }; + }); + + afterAll(() => { + jest.useRealTimers(); + window.requestAnimationFrame = _raf; + window.cancelAnimationFrame = _caf; + }); + + it('cleans up timeouts and rAFs on unmount', () => { + const component = mount( + } + closePopover={() => {}} + panelPaddingSize="s" + isOpen={false} + /> + ); + + component.setProps({ isOpen: true }); + + component.unmount(); + + // EUI's jest configuration throws an error if there are any console.error calls, like + // React's setState on an unmounted component warning + // to be future proof, verify that's still the case + expect(() => { + console.error('This is a test'); + }).toThrow(); + + // execute any pending timeouts or animation frame callbacks + // and validate the timeout/rAF clearing done by EuiPopover + jest.advanceTimersByTime(10); + }); + }); }); describe('getPopoverPositionFromAnchorPosition', () => { diff --git a/src/components/popover/popover.tsx b/src/components/popover/popover.tsx index 3e98caf2da0..27761afd48d 100644 --- a/src/components/popover/popover.tsx +++ b/src/components/popover/popover.tsx @@ -270,7 +270,10 @@ export class EuiPopover extends Component { return null; } + private respositionTimeout: number | undefined; private closingTransitionTimeout: number | undefined; + private closingTransitionAnimationFrame: number | undefined; + private updateFocusAnimationFrame: number | undefined; private button: HTMLElement | null = null; private panel: HTMLElement | null = null; @@ -304,7 +307,7 @@ export class EuiPopover extends Component { updateFocus() { // Wait for the DOM to update. - window.requestAnimationFrame(() => { + this.updateFocusAnimationFrame = window.requestAnimationFrame(() => { if (!this.props.ownFocus || !this.panel) { return; } @@ -374,11 +377,13 @@ export class EuiPopover extends Component { clearTimeout(this.closingTransitionTimeout); // We need to set this state a beat after the render takes place, so that the CSS // transition can take effect. - window.requestAnimationFrame(() => { - this.setState({ - isOpening: true, - }); - }); + this.closingTransitionAnimationFrame = window.requestAnimationFrame( + () => { + this.setState({ + isOpening: true, + }); + } + ); // for each child element of `this.panel`, find any transition duration we should wait for before stabilizing const { durationMatch, delayMatch } = Array.prototype.slice @@ -398,7 +403,7 @@ export class EuiPopover extends Component { { durationMatch: 0, delayMatch: 0 } ); - setTimeout(() => { + this.respositionTimeout = window.setTimeout(() => { this.setState({ isOpenStable: true }, () => { this.positionPopoverFixed(); this.updateFocus(); @@ -429,7 +434,10 @@ export class EuiPopover extends Component { componentWillUnmount() { window.removeEventListener('scroll', this.positionPopoverFixed); + clearTimeout(this.respositionTimeout); clearTimeout(this.closingTransitionTimeout); + cancelAnimationFrame(this.closingTransitionAnimationFrame!); + cancelAnimationFrame(this.updateFocusAnimationFrame!); } onMutation = (records: MutationRecord[]) => {