From d6091a3e6c402139af2e9339cae3588ace0e664d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Felix=20St=C3=BCrmer?= Date: Wed, 13 Jul 2022 14:28:40 +0000 Subject: [PATCH 1/7] Fix row height tests and mocks --- .../datagrid/body/data_grid_body.test.tsx | 23 ++- .../datagrid/body/data_grid_body.tsx | 2 +- .../datagrid/body/data_grid_cell.test.tsx | 24 +-- .../datagrid/utils/__mocks__/row_heights.ts | 54 +++--- .../datagrid/utils/row_heights.test.ts | 165 ++++++++++++------ src/components/datagrid/utils/row_heights.ts | 39 ++--- src/services/hooks/index.ts | 1 + src/services/hooks/useLatest.ts | 15 ++ 8 files changed, 195 insertions(+), 128 deletions(-) create mode 100644 src/services/hooks/useLatest.ts diff --git a/src/components/datagrid/body/data_grid_body.test.tsx b/src/components/datagrid/body/data_grid_body.test.tsx index 422162b9492..e3c7488258a 100644 --- a/src/components/datagrid/body/data_grid_body.test.tsx +++ b/src/components/datagrid/body/data_grid_body.test.tsx @@ -9,12 +9,22 @@ import React from 'react'; import { mount, render, shallow } from 'enzyme'; -import { mockRowHeightUtils } from '../utils/__mocks__/row_heights'; +import { RowHeightUtils } from '../utils/__mocks__/row_heights'; import { schemaDetectors } from '../utils/data_grid_schema'; import { EuiDataGridBody, Cell } from './data_grid_body'; describe('EuiDataGridBody', () => { + const gridRef = { + current: { + resetAfterColumnIndex: jest.fn(), + resetAfterRowIndex: jest.fn(), + } as any, + }; + const gridItemsRendered = { current: null }; + const rerenderGridBodyRef = { current: null }; + const rowHeightUtils = new RowHeightUtils(gridRef, rerenderGridBodyRef); + const requiredProps = { headerIsInteractive: true, rowCount: 1, @@ -39,17 +49,12 @@ describe('EuiDataGridBody', () => { setVisibleColumns: jest.fn(), switchColumnPos: jest.fn(), schemaDetectors, - rowHeightUtils: mockRowHeightUtils, + rowHeightUtils, isFullScreen: false, gridStyles: {}, gridWidth: 300, - gridRef: { - current: { - resetAfterColumnIndex: jest.fn(), - resetAfterRowIndex: jest.fn(), - } as any, - }, - gridItemsRendered: {} as any, + gridRef, + gridItemsRendered, wrapperRef: { current: document.createElement('div') }, }; diff --git a/src/components/datagrid/body/data_grid_body.tsx b/src/components/datagrid/body/data_grid_body.tsx index 8928e1c78d8..c51e9bc4e0f 100644 --- a/src/components/datagrid/body/data_grid_body.tsx +++ b/src/components/datagrid/body/data_grid_body.tsx @@ -388,7 +388,7 @@ export const EuiDataGridBody: FunctionComponent = ( * Heights */ const rowHeightUtils = useRowHeightUtils({ - gridRef: gridRef.current, + gridRef, gridStyles, columns, rowHeightsOptions, diff --git a/src/components/datagrid/body/data_grid_cell.test.tsx b/src/components/datagrid/body/data_grid_cell.test.tsx index 61420a60898..9eb003b0437 100644 --- a/src/components/datagrid/body/data_grid_cell.test.tsx +++ b/src/components/datagrid/body/data_grid_cell.test.tsx @@ -9,13 +9,18 @@ import React, { useEffect } from 'react'; import { mount, render, ReactWrapper } from 'enzyme'; import { keys } from '../../../services'; -import { mockRowHeightUtils } from '../utils/__mocks__/row_heights'; +import { RowHeightUtils } from '../utils/__mocks__/row_heights'; import { mockFocusContext } from '../utils/__mocks__/focus_context'; import { DataGridFocusContext } from '../utils/focus'; import { EuiDataGridCell } from './data_grid_cell'; describe('EuiDataGridCell', () => { + const mockRowHeightUtils = new RowHeightUtils( + { current: null }, + { current: null } + ); + const mockPopoverContext = { popoverIsOpen: false, cellLocation: { rowIndex: 0, colIndex: 0 }, @@ -48,23 +53,6 @@ describe('EuiDataGridCell', () => { expect(component).toMatchSnapshot(); }); - it("renders the cell's `aria-rowindex` correctly when paginated on a different page", () => { - const component = mount( - {}, - onChangeItemsPerPage: () => {}, - }} - /> - ); - expect( - component.find('[data-test-subj="dataGridRowCell"]').prop('aria-rowindex') - ).toEqual(61); - }); - it('renders cell actions', () => { const component = mount( ; -export const mockRowHeightUtils = ({ - cacheStyles: jest.fn(), - setGrid: jest.fn(), - getStylesForCell: jest.fn(() => ({ - wordWrap: 'break-word', - wordBreak: 'break-word', - flexGrow: 1, - })), - isAutoHeight: jest.fn(() => false), - setRowHeight: jest.fn(), - pruneHiddenColumnHeights: jest.fn(), - getRowHeight: jest.fn(() => 32), - getRowHeightOption: jest.fn(actual.getRowHeightOption), - getCalculatedHeight: jest.fn(actual.getCalculatedHeight), - getLineCount: jest.fn(actual.getLineCount), - calculateHeightForLineCount: jest.fn(() => 50), - isRowHeightOverride: jest.fn(actual.isRowHeightOverride), - setRerenderGridBody: jest.fn(), -} as unknown) as ActualRowHeightUtils; +export const RowHeightUtils = jest + .fn< + ActualRowHeightUtils, + ConstructorParameters + >() + .mockImplementation((...args) => { + const rowHeightUtils = new ActualRowHeightUtils(...args); -export const RowHeightUtils = jest.fn(() => mockRowHeightUtils); + const rowHeightUtilsMock: RowHeightUtilsPublicAPI = { + cacheStyles: jest.fn(), + getStylesForCell: jest.fn(() => ({ + wordWrap: 'break-word', + wordBreak: 'break-word', + flexGrow: 1, + })), + isAutoHeight: jest.fn(() => false), + setRowHeight: jest.fn(), + pruneHiddenColumnHeights: jest.fn(), + getRowHeight: jest.fn(() => 32), + getRowHeightOption: jest.fn(rowHeightUtils.getRowHeightOption), + getCalculatedHeight: jest.fn(rowHeightUtils.getCalculatedHeight), + getLineCount: jest.fn(rowHeightUtils.getLineCount), + calculateHeightForLineCount: jest.fn(() => 50), + isRowHeightOverride: jest.fn(rowHeightUtils.isRowHeightOverride), + resetRow: jest.fn(), + resetGrid: jest.fn(), + }; + + return (rowHeightUtilsMock as any) as ActualRowHeightUtils; + }); diff --git a/src/components/datagrid/utils/row_heights.test.ts b/src/components/datagrid/utils/row_heights.test.ts index dc63ec3d68b..f4cc83eb41b 100644 --- a/src/components/datagrid/utils/row_heights.test.ts +++ b/src/components/datagrid/utils/row_heights.test.ts @@ -6,18 +6,38 @@ * Side Public License, v 1. */ +import type { MutableRefObject } from 'react'; import { act } from 'react-dom/test-utils'; import { testCustomHook } from '../../../test/internal'; import { startingStyles } from '../controls'; - +import type { ImperativeGridApi } from '../data_grid_types'; import { - RowHeightUtils, cellPaddingsMap, + RowHeightUtils, useRowHeightUtils, } from './row_heights'; describe('RowHeightUtils', () => { - const rowHeightUtils = new RowHeightUtils(); + const gridRef: MutableRefObject = { + current: { + resetAfterIndices: jest.fn(), + resetAfterColumnIndex: jest.fn(), + resetAfterRowIndex: jest.fn(), + scrollTo: jest.fn(), + scrollToItem: jest.fn(), + }, + }; + const rerenderGridBodyRef = { current: jest.fn() }; + const rowHeightUtils = new RowHeightUtils(gridRef, rerenderGridBodyRef); + + beforeEach(() => { + jest.useFakeTimers(); + }); + + afterEach(() => { + jest.useRealTimers(); + jest.resetAllMocks(); + }); describe('getRowHeightOption', () => { const rowHeightsOptions = { @@ -117,8 +137,15 @@ describe('RowHeightUtils', () => { }); describe('auto height', () => { - const getRowHeightSpy = jest.spyOn(rowHeightUtils, 'getRowHeight'); - beforeEach(() => getRowHeightSpy.mockClear()); + let getRowHeightSpy: jest.SpyInstance; + + beforeEach(() => { + getRowHeightSpy = jest.spyOn(rowHeightUtils, 'getRowHeight'); + }); + + afterEach(() => { + getRowHeightSpy.mockRestore(); + }); it('gets the max height for the current row from the heights cache', () => { expect(rowHeightUtils.getCalculatedHeight('auto', 34, 1)).toEqual(0); // 0 is expected since the cache is empty @@ -212,13 +239,19 @@ describe('RowHeightUtils', () => { }); describe('calculateHeightForLineCount', () => { + let getComputedStyleSpy: jest.SpyInstance; + const cell = document.createElement('div'); + beforeEach(() => { rowHeightUtils.cacheStyles({ cellPadding: 'm' }); - jest + getComputedStyleSpy = jest .spyOn(window, 'getComputedStyle') .mockReturnValue({ lineHeight: '24px' } as CSSStyleDeclaration); }); - const cell = document.createElement('div'); + + afterEach(() => { + getComputedStyleSpy.mockRestore(); + }); it('calculates the row height based on the number of lines and cell line height/padding', () => { expect(rowHeightUtils.calculateHeightForLineCount(cell, 1)).toEqual(36); // 1 * 24 + 6 + 6 @@ -275,10 +308,17 @@ describe('RowHeightUtils', () => { }); describe('row height cache', () => { - describe('setRowHeight', () => { - const resetRowSpy = jest.spyOn(rowHeightUtils, 'resetRow'); - beforeEach(() => resetRowSpy.mockClear()); + let resetRowSpy: jest.SpyInstance; + + beforeEach(() => { + resetRowSpy = jest.spyOn(rowHeightUtils, 'resetRow'); + }); + + afterEach(() => { + resetRowSpy.mockRestore(); + }); + describe('setRowHeight', () => { it('setRowHeight', () => { rowHeightUtils.setRowHeight(5, 'a', 50, 0); rowHeightUtils.setRowHeight(5, 'b', 34, 0); @@ -302,11 +342,9 @@ describe('RowHeightUtils', () => { }); it('calls rerenderGridBody', () => { - const rerenderGridBody = jest.fn(); - rowHeightUtils.setRerenderGridBody(rerenderGridBody); - expect(rerenderGridBody).toHaveBeenCalledTimes(0); + expect(rerenderGridBodyRef.current).toHaveBeenCalledTimes(0); rowHeightUtils.setRowHeight(1, 'a', 34, 1); - expect(rerenderGridBody).toHaveBeenCalledTimes(1); + expect(rerenderGridBodyRef.current).toHaveBeenCalledTimes(1); }); }); @@ -321,9 +359,6 @@ describe('RowHeightUtils', () => { }); describe('pruneHiddenColumnHeights', () => { - const resetRowSpy = jest.spyOn(rowHeightUtils, 'resetRow'); - beforeEach(() => resetRowSpy.mockClear()); - it('checks each row height map and deletes column IDs that are no longer visible', () => { rowHeightUtils.pruneHiddenColumnHeights([{ id: 'a' }, { id: 'b' }]); expect(rowHeightUtils.getRowHeight(5)).toEqual(62); @@ -343,21 +378,16 @@ describe('RowHeightUtils', () => { }); describe('grid resetting', () => { - const mockGrid = { resetAfterRowIndex: jest.fn() } as any; - beforeEach(() => jest.clearAllMocks()); - - describe('setGrid', () => { - it('stores the react-window grid as an instance variable', () => { - rowHeightUtils.setGrid(mockGrid); + describe('resetRow', () => { + let resetGridSpy: jest.SpyInstance; - // @ts-ignore this var is private, but we're inspecting it for the sake of the unit test - expect(rowHeightUtils.grid).toEqual(mockGrid); + beforeEach(() => { + resetGridSpy = jest.spyOn(rowHeightUtils, 'resetGrid'); }); - }); - describe('resetRow', () => { - const resetGridSpy = jest.spyOn(rowHeightUtils, 'resetGrid'); - jest.useFakeTimers(); + afterEach(() => { + resetGridSpy.mockRestore(); + }); it('sets this.lastUpdatedRow and resets the grid', () => { rowHeightUtils.resetRow(0); @@ -365,6 +395,7 @@ describe('RowHeightUtils', () => { expect(rowHeightUtils.lastUpdatedRow).toEqual(0); jest.runAllTimers(); + expect(resetGridSpy).toHaveBeenCalled(); // @ts-ignore this var is private, but we're inspecting it for the sake of the unit test expect(rowHeightUtils.lastUpdatedRow).toEqual(Infinity); @@ -374,16 +405,20 @@ describe('RowHeightUtils', () => { describe('resetGrid', () => { it('invokes grid.resetAfterRowIndex with the last visible row', () => { rowHeightUtils.setRowHeight(99, 'a', 34, 99); - rowHeightUtils.resetGrid(); - expect(mockGrid.resetAfterRowIndex).toHaveBeenCalledWith(99); + + jest.runAllTimers(); + + expect(gridRef.current?.resetAfterRowIndex).toHaveBeenCalledWith(99); }); it('invokes resetAfterRowIndex only once with the smallest cached row index', () => { - rowHeightUtils.setRowHeight(97, 'a', 34, 97); - rowHeightUtils.setRowHeight(99, 'a', 34, 99); - rowHeightUtils.resetGrid(); - expect(mockGrid.resetAfterRowIndex).toHaveBeenCalledTimes(1); - expect(mockGrid.resetAfterRowIndex).toHaveBeenCalledWith(97); + rowHeightUtils.setRowHeight(97, 'a', 35, 97); + rowHeightUtils.setRowHeight(99, 'a', 36, 99); + + jest.runAllTimers(); + + expect(gridRef.current?.resetAfterRowIndex).toHaveBeenCalledTimes(1); + expect(gridRef.current?.resetAfterRowIndex).toHaveBeenCalledWith(97); }); }); }); @@ -391,37 +426,52 @@ describe('RowHeightUtils', () => { }); describe('useRowHeightUtils', () => { + const gridRef: MutableRefObject = { + current: { + resetAfterIndices: jest.fn(), + resetAfterColumnIndex: jest.fn(), + + resetAfterRowIndex: jest.fn(), + scrollTo: jest.fn(), + scrollToItem: jest.fn(), + }, + }; + const outerGridElementRef = { current: null }; + const gridItemsRenderedRef = { current: null }; + const mockArgs = { - gridRef: null, + gridRef, + outerGridElementRef, + gridItemsRenderedRef, gridStyles: startingStyles, columns: [{ id: 'A' }, { id: 'B' }], rowHeightOptions: undefined, }; + let requestAnimationFrameSpy: jest.SpyInstance; + + beforeEach(() => { + requestAnimationFrameSpy = jest + .spyOn(window, 'requestAnimationFrame') + .mockImplementation((cb: any) => cb()); + }); + + afterEach(() => { + requestAnimationFrameSpy.mockRestore(); + }); + it('instantiates and returns an instance of RowHeightUtils', () => { - const { return: rowHeightUtils } = testCustomHook(() => + const { return: rowHeightUtils } = testCustomHook(() => useRowHeightUtils(mockArgs) ); expect(rowHeightUtils).toBeInstanceOf(RowHeightUtils); }); - it('populates internal RowHeightUtils vars from outside dependencies', () => { - const args = { ...mockArgs, gridRef: {} as any }; - const { return: rowHeightUtils } = testCustomHook(() => - useRowHeightUtils(args) - ); - // @ts-ignore - intentionally inspecting private var for test - expect(rowHeightUtils.grid).toEqual(args.gridRef); - // @ts-ignore - intentionally inspecting private var for test - expect(rowHeightUtils.rerenderGridBody).toBeInstanceOf(Function); - }); - it('forces a rerender every time rowHeightsOptions changes', () => { - const requestAnimationFrameSpy = jest - .spyOn(window, 'requestAnimationFrame') - .mockImplementation((cb: any) => cb()); - - const { updateHookArgs } = testCustomHook(useRowHeightUtils, mockArgs); + const { updateHookArgs } = testCustomHook( + useRowHeightUtils, + mockArgs + ); expect(requestAnimationFrameSpy).toHaveBeenCalledTimes(1); updateHookArgs({ rowHeightsOptions: { defaultHeight: 300 } }); @@ -434,10 +484,9 @@ describe('useRowHeightUtils', () => { }); it('updates internal cached styles whenever gridStyle.cellPadding changes', () => { - const { return: rowHeightUtils, updateHookArgs } = testCustomHook( - useRowHeightUtils, - mockArgs - ); + const { return: rowHeightUtils, updateHookArgs } = testCustomHook< + RowHeightUtils + >(useRowHeightUtils, mockArgs); updateHookArgs({ gridStyles: { ...startingStyles, cellPadding: 's' } }); // @ts-ignore - intentionally inspecting private var for test diff --git a/src/components/datagrid/utils/row_heights.ts b/src/components/datagrid/utils/row_heights.ts index 6d313fcf4a4..5d2f13d991b 100644 --- a/src/components/datagrid/utils/row_heights.ts +++ b/src/components/datagrid/utils/row_heights.ts @@ -13,10 +13,12 @@ import { useCallback, useContext, CSSProperties, + MutableRefObject, + Component, } from 'react'; import type { VariableSizeGrid as Grid } from 'react-window'; import { isObject, isNumber } from '../../../services/predicate'; -import { useForceRender } from '../../../services'; +import { useForceRender, useLatest } from '../../../services'; import { EuiDataGridStyleCellPaddings, EuiDataGridStyle, @@ -36,7 +38,14 @@ export const cellPaddingsMap: Record = { export const AUTO_HEIGHT = 'auto'; export const DEFAULT_ROW_HEIGHT = 34; +type IGrid = Omit; + export class RowHeightUtils { + constructor( + private gridRef: MutableRefObject, + private rerenderGridBodyRef: MutableRefObject<(() => void) | null> + ) {} + getRowHeightOption( rowIndex: number, rowHeightsOptions?: EuiDataGridRowHeightsOptions @@ -157,9 +166,7 @@ export class RowHeightUtils { private heightsCache = new Map>(); private timerId?: number; - private grid?: Grid; private lastUpdatedRow: number = Infinity; - private rerenderGridBody: Function = () => {}; isAutoHeight( rowIndex: number, @@ -205,7 +212,7 @@ export class RowHeightUtils { // When an auto row height is updated, force a re-render // of the grid body to update the unconstrained height - this.rerenderGridBody(); + this.rerenderGridBodyRef.current?.(); } pruneHiddenColumnHeights(visibleColumns: EuiDataGridColumn[]) { @@ -236,17 +243,9 @@ export class RowHeightUtils { } resetGrid() { - this.grid?.resetAfterRowIndex(this.lastUpdatedRow); + this.gridRef.current?.resetAfterRowIndex(this.lastUpdatedRow); this.lastUpdatedRow = Infinity; } - - setGrid(grid: Grid) { - this.grid = grid; - } - - setRerenderGridBody(rerenderGridBody: Function) { - this.rerenderGridBody = rerenderGridBody; - } } /** @@ -259,19 +258,17 @@ export const useRowHeightUtils = ({ columns, rowHeightsOptions, }: { - gridRef: Grid | null; + gridRef: MutableRefObject; gridStyles: EuiDataGridStyle; columns: EuiDataGridColumn[]; rowHeightsOptions?: EuiDataGridRowHeightsOptions; }) => { - const rowHeightUtils = useMemo(() => new RowHeightUtils(), []); - - // Update rowHeightUtils with internal vars from outside dependencies const forceRender = useForceRender(); - useEffect(() => { - if (gridRef) rowHeightUtils.setGrid(gridRef); - rowHeightUtils.setRerenderGridBody(forceRender); - }, [gridRef, forceRender, rowHeightUtils]); + const forceRenderRef = useLatest(forceRender); + const rowHeightUtils = useMemo( + () => new RowHeightUtils(gridRef, forceRenderRef), + [forceRenderRef, gridRef] + ); // Forces a rerender whenever the row heights change, as this can cause the // grid to change height/have scrollbars. Without this, grid rerendering is stale diff --git a/src/services/hooks/index.ts b/src/services/hooks/index.ts index 6f7778cc4f0..16243548f85 100644 --- a/src/services/hooks/index.ts +++ b/src/services/hooks/index.ts @@ -10,5 +10,6 @@ export * from './useDependentState'; export * from './useCombinedRefs'; export * from './useForceRender'; export * from './useIsWithinBreakpoints'; +export * from './useLatest'; export * from './useMouseMove'; export * from './useUpdateEffect'; diff --git a/src/services/hooks/useLatest.ts b/src/services/hooks/useLatest.ts new file mode 100644 index 00000000000..d56f7730bee --- /dev/null +++ b/src/services/hooks/useLatest.ts @@ -0,0 +1,15 @@ +/* + * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one + * or more contributor license agreements. Licensed under the Elastic License + * 2.0 and the Server Side Public License, v 1; you may not use this file except + * in compliance with, at your election, the Elastic License 2.0 or the Server + * Side Public License, v 1. + */ + +import { MutableRefObject, useRef } from 'react'; + +export function useLatest(value: Value): MutableRefObject { + const latestValueRef = useRef(value); + latestValueRef.current = value; + return latestValueRef; +} From 24f6bce78efcca9605e63f669401068eb00f3715 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Felix=20St=C3=BCrmer?= Date: Wed, 13 Jul 2022 14:41:57 +0000 Subject: [PATCH 2/7] Restore accidentally removed test case --- .../datagrid/body/data_grid_cell.test.tsx | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/src/components/datagrid/body/data_grid_cell.test.tsx b/src/components/datagrid/body/data_grid_cell.test.tsx index 9eb003b0437..799a9e398da 100644 --- a/src/components/datagrid/body/data_grid_cell.test.tsx +++ b/src/components/datagrid/body/data_grid_cell.test.tsx @@ -53,6 +53,23 @@ describe('EuiDataGridCell', () => { expect(component).toMatchSnapshot(); }); + it("renders the cell's `aria-rowindex` correctly when paginated on a different page", () => { + const component = mount( + {}, + onChangeItemsPerPage: () => {}, + }} + /> + ); + expect( + component.find('[data-test-subj="dataGridRowCell"]').prop('aria-rowindex') + ).toEqual(61); + }); + it('renders cell actions', () => { const component = mount( Date: Thu, 14 Jul 2022 12:35:05 +0200 Subject: [PATCH 3/7] Update src/components/datagrid/utils/row_heights.test.ts Co-authored-by: Constance --- src/components/datagrid/utils/row_heights.test.ts | 1 - 1 file changed, 1 deletion(-) diff --git a/src/components/datagrid/utils/row_heights.test.ts b/src/components/datagrid/utils/row_heights.test.ts index f4cc83eb41b..7b541888d5a 100644 --- a/src/components/datagrid/utils/row_heights.test.ts +++ b/src/components/datagrid/utils/row_heights.test.ts @@ -430,7 +430,6 @@ describe('useRowHeightUtils', () => { current: { resetAfterIndices: jest.fn(), resetAfterColumnIndex: jest.fn(), - resetAfterRowIndex: jest.fn(), scrollTo: jest.fn(), scrollToItem: jest.fn(), From 6c2c170341925a465786d2fdf7869a27363073bc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Felix=20St=C3=BCrmer?= Date: Mon, 18 Jul 2022 15:39:34 +0000 Subject: [PATCH 4/7] Clean up forceRender usage and RowHeightUtils instantiation --- src/components/datagrid/utils/row_heights.ts | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-) diff --git a/src/components/datagrid/utils/row_heights.ts b/src/components/datagrid/utils/row_heights.ts index 5d2f13d991b..c13fea1d4eb 100644 --- a/src/components/datagrid/utils/row_heights.ts +++ b/src/components/datagrid/utils/row_heights.ts @@ -263,24 +263,26 @@ export const useRowHeightUtils = ({ columns: EuiDataGridColumn[]; rowHeightsOptions?: EuiDataGridRowHeightsOptions; }) => { - const forceRender = useForceRender(); - const forceRenderRef = useLatest(forceRender); - const rowHeightUtils = useMemo( - () => new RowHeightUtils(gridRef, forceRenderRef), - [forceRenderRef, gridRef] + const forceRenderRef = useLatest(useForceRender()); + const [rowHeightUtils] = useState( + () => new RowHeightUtils(gridRef, forceRenderRef) ); // Forces a rerender whenever the row heights change, as this can cause the // grid to change height/have scrollbars. Without this, grid rerendering is stale useEffect(() => { - requestAnimationFrame(forceRender); + if (forceRenderRef.current == null) { + return; + } + + requestAnimationFrame(forceRenderRef.current); }, [ // Effects that should cause rerendering rowHeightsOptions?.defaultHeight, rowHeightsOptions?.rowHeights, // Dependencies rowHeightUtils, - forceRender, + forceRenderRef, ]); // Re-cache styles whenever grid density changes From fd360979a847f92d6631d3d2584228df95659d99 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Felix=20St=C3=BCrmer?= Date: Mon, 18 Jul 2022 15:40:16 +0000 Subject: [PATCH 5/7] Deduplicate grid API type --- src/components/datagrid/data_grid_types.ts | 2 ++ src/components/datagrid/utils/row_heights.ts | 9 +++------ 2 files changed, 5 insertions(+), 6 deletions(-) diff --git a/src/components/datagrid/data_grid_types.ts b/src/components/datagrid/data_grid_types.ts index 556c07faae3..33462e4c857 100644 --- a/src/components/datagrid/data_grid_types.ts +++ b/src/components/datagrid/data_grid_types.ts @@ -29,6 +29,8 @@ import { RowHeightUtils } from './utils/row_heights'; import { IconType } from '../icon'; import { EuiTokenProps } from '../token'; +// since react-window doesn't export a type with the imperative api only we can +// use this to omit the react-specific class component methods export type ImperativeGridApi = Omit; export interface EuiDataGridToolbarProps { diff --git a/src/components/datagrid/utils/row_heights.ts b/src/components/datagrid/utils/row_heights.ts index c13fea1d4eb..d5556ad6ee8 100644 --- a/src/components/datagrid/utils/row_heights.ts +++ b/src/components/datagrid/utils/row_heights.ts @@ -14,9 +14,7 @@ import { useContext, CSSProperties, MutableRefObject, - Component, } from 'react'; -import type { VariableSizeGrid as Grid } from 'react-window'; import { isObject, isNumber } from '../../../services/predicate'; import { useForceRender, useLatest } from '../../../services'; import { @@ -25,6 +23,7 @@ import { EuiDataGridRowHeightOption, EuiDataGridRowHeightsOptions, EuiDataGridColumn, + ImperativeGridApi, } from '../data_grid_types'; import { DataGridSortingContext } from './sorting'; @@ -38,11 +37,9 @@ export const cellPaddingsMap: Record = { export const AUTO_HEIGHT = 'auto'; export const DEFAULT_ROW_HEIGHT = 34; -type IGrid = Omit; - export class RowHeightUtils { constructor( - private gridRef: MutableRefObject, + private gridRef: MutableRefObject, private rerenderGridBodyRef: MutableRefObject<(() => void) | null> ) {} @@ -258,7 +255,7 @@ export const useRowHeightUtils = ({ columns, rowHeightsOptions, }: { - gridRef: MutableRefObject; + gridRef: MutableRefObject; gridStyles: EuiDataGridStyle; columns: EuiDataGridColumn[]; rowHeightsOptions?: EuiDataGridRowHeightsOptions; From c6ac32b612975e551cc04542693aa1b599fc2114 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Felix=20St=C3=BCrmer?= Date: Mon, 18 Jul 2022 16:24:37 +0000 Subject: [PATCH 6/7] Comment and test the useLatest hook --- src/services/hooks/useLatest.test.tsx | 69 +++++++++++++++++++++++++++ src/services/hooks/useLatest.ts | 4 ++ 2 files changed, 73 insertions(+) create mode 100644 src/services/hooks/useLatest.test.tsx diff --git a/src/services/hooks/useLatest.test.tsx b/src/services/hooks/useLatest.test.tsx new file mode 100644 index 00000000000..d4d5b62b87d --- /dev/null +++ b/src/services/hooks/useLatest.test.tsx @@ -0,0 +1,69 @@ +/* + * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one + * or more contributor license agreements. Licensed under the Elastic License + * 2.0 and the Server Side Public License, v 1; you may not use this file except + * in compliance with, at your election, the Elastic License 2.0 or the Server + * Side Public License, v 1. + */ + +import { mount } from 'enzyme'; +import React, { MutableRefObject, useEffect } from 'react'; +import { useLatest } from './useLatest'; + +describe('useLatest', () => { + beforeEach(() => { + jest.clearAllMocks(); + }); + + afterEach(() => { + jest.clearAllMocks(); + }); + + it('updates the ref value but not the ref identity on render', () => { + const onRefChange = jest.fn(); + const onRefCurrentValueChange = jest.fn(); + + const wrapper = mount( + + ); + + expect(onRefChange).toHaveBeenCalledTimes(1); + expect(onRefChange).toHaveBeenLastCalledWith({ current: 'first' }); + expect(onRefCurrentValueChange).toHaveBeenCalledTimes(1); + expect(onRefCurrentValueChange).toHaveBeenLastCalledWith('first'); + + wrapper.setProps({ value: 'second' }); + + expect(onRefChange).toHaveBeenCalledTimes(1); // the ref's identity has not changed + expect(onRefCurrentValueChange).toHaveBeenCalledTimes(2); + expect(onRefCurrentValueChange).toHaveBeenLastCalledWith('second'); + }); +}); + +const MockComponent = ({ + value, + onRefChange, + onRefCurrentValueChange, +}: { + value: string; + onRefChange: (ref: MutableRefObject) => void; + onRefCurrentValueChange: (currentValue: string | null) => void; +}) => { + const valueRef = useLatest(value); + + useEffect(() => { + onRefChange(valueRef); + }, [onRefChange, valueRef]); + + useEffect(() => { + onRefCurrentValueChange(valueRef.current); + // we're intentionally looking into the ref + // eslint-disable-next-line react-hooks/exhaustive-deps + }, [onRefCurrentValueChange, valueRef.current]); + + return null; +}; diff --git a/src/services/hooks/useLatest.ts b/src/services/hooks/useLatest.ts index d56f7730bee..98d9f4b8243 100644 --- a/src/services/hooks/useLatest.ts +++ b/src/services/hooks/useLatest.ts @@ -8,6 +8,10 @@ import { MutableRefObject, useRef } from 'react'; +/** + * Wraps the given `value` into a `MutableRefObject` and keeps the `current` + * value up-to-date on very render cycle. + */ export function useLatest(value: Value): MutableRefObject { const latestValueRef = useRef(value); latestValueRef.current = value; From b5c152bacc0a92df16c4e3845bad7a26349f75b1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Felix=20St=C3=BCrmer?= Date: Mon, 18 Jul 2022 22:29:20 +0200 Subject: [PATCH 7/7] Fix typo in comment Co-authored-by: Constance --- src/services/hooks/useLatest.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/services/hooks/useLatest.ts b/src/services/hooks/useLatest.ts index 98d9f4b8243..dea76b61404 100644 --- a/src/services/hooks/useLatest.ts +++ b/src/services/hooks/useLatest.ts @@ -10,7 +10,7 @@ import { MutableRefObject, useRef } from 'react'; /** * Wraps the given `value` into a `MutableRefObject` and keeps the `current` - * value up-to-date on very render cycle. + * value up-to-date on every render cycle. */ export function useLatest(value: Value): MutableRefObject { const latestValueRef = useRef(value);