Skip to content

Commit

Permalink
[EuiResizableContainer] Fix issue where onResizeEnd becomes stale w…
Browse files Browse the repository at this point in the history
…hen renders occur between resize start and end (#7468)

Co-authored-by: Cee Chen <[email protected]>
  • Loading branch information
davismcphee and cee-chen authored Jan 25, 2024
1 parent 516215f commit fca1f5e
Show file tree
Hide file tree
Showing 3 changed files with 55 additions and 6 deletions.
3 changes: 3 additions & 0 deletions changelogs/upcoming/7468.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
**Bug fixes**

- Fixed an issue in `EuiResizableContainer` where `onResizeEnd` could become a stale closure when renders occured between resize start and end, resulting in an outdated version of a consumer's `onResizeEnd` callback being called
44 changes: 43 additions & 1 deletion src/components/resizable_container/resizable_container.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,9 @@
* Side Public License, v 1.
*/

import React from 'react';
import React, { useState } from 'react';
import { act } from 'react-dom/test-utils';
import { fireEvent } from '@testing-library/react';
import { mount } from 'enzyme';

import { findTestSubject, requiredProps } from '../../test';
Expand Down Expand Up @@ -348,5 +349,46 @@ describe('EuiResizableContainer', () => {
button.simulate('blur');
expect(onResizeEnd).toHaveBeenCalledTimes(1);
});

test('unmemoized consumer onResizeStart/End callbacks do not cause stale closures', () => {
const ConsumerUsage = () => {
const [rerender, setRerender] = useState(0);
// Unmemoized consumer callbacks
const onResizeStart = () => {
setRerender(rerender + 1);
};
const onResizeEnd = () => {
setRerender(rerender + 1);
};

return (
<EuiResizableContainer
{...requiredProps}
onResizeStart={onResizeStart}
onResizeEnd={onResizeEnd}
>
{(EuiResizablePanel, EuiResizableButton) => (
<>
<EuiResizablePanel initialSize={50}>Testing</EuiResizablePanel>
<EuiResizableButton data-test-subj="euiResizableButton" />
<EuiResizablePanel initialSize={50} data-test-subj="rerenders">
{rerender}
</EuiResizablePanel>
</>
)}
</EuiResizableContainer>
);
};
const { getByTestSubject } = render(<ConsumerUsage />);
expect(getByTestSubject('rerenders')).toHaveTextContent('0');

fireEvent.mouseDown(getByTestSubject('euiResizableButton'));
expect(getByTestSubject('rerenders')).toHaveTextContent('1');

fireEvent.mouseUp(getByTestSubject('euiResizableButton'));
expect(getByTestSubject('rerenders')).toHaveTextContent('2');
// Without `useLatest`, the rerender count doesn't correctly update due to `onResizeEnd`
// not being memoized and causing a stale `resizeEnd` closure to be called on event end
});
});
});
14 changes: 9 additions & 5 deletions src/components/resizable_container/resizable_container.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ import React, {
import classNames from 'classnames';

import { CommonProps } from '../common';
import { keys } from '../../services';
import { keys, useLatest } from '../../services';
import { useResizeObserver } from '../observer/resize_observer';
import { EuiResizableContainerContextProvider } from './context';
import {
Expand Down Expand Up @@ -100,6 +100,10 @@ export const EuiResizableContainer: FunctionComponent<
onResizeEnd,
...rest
}) => {
// Note: It's important to memoize consumer callbacks to prevent our own functions
// from reinstantiating unnecessarily & causing window event listeners to call stale closures
const onResizeEndRef = useLatest(onResizeEnd);
const onResizeStartRef = useLatest(onResizeStart);
const containerRef = useRef<HTMLDivElement>(null);
const isHorizontal = direction === 'horizontal';

Expand Down Expand Up @@ -135,9 +139,9 @@ export const EuiResizableContainer: FunctionComponent<
}>({});

const resizeEnd = useCallback(() => {
onResizeEnd?.();
onResizeEndRef.current?.();
resizeContext.current = {};
}, [onResizeEnd]);
}, [onResizeEndRef]);

const resizeStart = useCallback(
(trigger: ResizeTrigger, keyMoveDirection?: KeyMoveDirection) => {
Expand All @@ -148,10 +152,10 @@ export const EuiResizableContainer: FunctionComponent<
if (resizeContext.current.trigger) {
resizeEnd();
}
onResizeStart?.(trigger);
onResizeStartRef.current?.(trigger);
resizeContext.current = { trigger, keyMoveDirection };
},
[onResizeStart, resizeEnd]
[onResizeStartRef, resizeEnd]
);

const onMouseDown = useCallback(
Expand Down

0 comments on commit fca1f5e

Please sign in to comment.