From e9e562060e6f2266b059ccc69bbe795f886e4422 Mon Sep 17 00:00:00 2001 From: Adi Dahiya Date: Tue, 15 Mar 2022 10:21:58 -0400 Subject: [PATCH] [v4] [table] feat: experimental cellRendererDependencies prop (#5162) --- packages/table/src/common/errors.ts | 2 + packages/table/src/index.ts | 2 +- packages/table/src/table2.tsx | 57 +++++++++++++++++++++++++---- packages/table/test/table2Tests.tsx | 54 +++++++++++++++++++++++++-- 4 files changed, 103 insertions(+), 12 deletions(-) diff --git a/packages/table/src/common/errors.ts b/packages/table/src/common/errors.ts index 2b2f8fdf67..6dc95fbc2b 100644 --- a/packages/table/src/common/errors.ts +++ b/packages/table/src/common/errors.ts @@ -39,3 +39,5 @@ export const TABLE_NUM_COLUMNS_COLUMN_WIDTHS_MISMATCH = ns + ` Table requires columnWidths.length to equal the number of s if provided.`; export const TABLE_UNMOUNTED_RESIZE_WARNING = `${ns} Table resize method called while component is unmounted, this is a no-op.`; + +export const TABLE_INVALID_CELL_RENDERER_DEPS = `${ns} cellRendererDependencies should either always be defined or undefined; this feature cannot be enabled during the component lifecycle`; diff --git a/packages/table/src/index.ts b/packages/table/src/index.ts index 6a67baf85d..52d9f2d77e 100644 --- a/packages/table/src/index.ts +++ b/packages/table/src/index.ts @@ -91,4 +91,4 @@ export { ITableProps, TableProps } from "./tableProps"; export { Table } from "./table"; -export { Table2 } from "./table2"; +export { Table2, Table2Props } from "./table2"; diff --git a/packages/table/src/table2.tsx b/packages/table/src/table2.tsx index fe7863f4cc..bb8331d2bc 100644 --- a/packages/table/src/table2.tsx +++ b/packages/table/src/table2.tsx @@ -59,7 +59,18 @@ import type { TableProps, TablePropsDefaults, TablePropsWithDefaults } from "./t import type { TableState, TableSnapshot } from "./tableState"; import { clampNumFrozenColumns, clampNumFrozenRows, hasLoadingOption } from "./tableUtils"; -export class Table2 extends AbstractComponent2 { +export interface Table2Props extends TableProps { + /** + * Warning: experimental feature! + * + * This dependency list may be used to trigger a re-render of all cells when one of its elements changes + * (compared using shallow equality checks). This is done by invalidating the grid, which forces + * TableQuadrantStack to re-render. + */ + cellRendererDependencies?: React.DependencyList; +} + +export class Table2 extends AbstractComponent2 { public static displayName = `${DISPLAYNAME_PREFIX}.Table2`; public static defaultProps: TablePropsDefaults = { @@ -419,7 +430,7 @@ export class Table2 extends AbstractComponent2 dep !== (prevProps.cellRendererDependencies ?? [])[index], + ); + + const didColumnWidthsChange = + this.props.columnWidths !== undefined && + !Utils.compareSparseArrays(this.props.columnWidths, prevState.columnWidths); + const didRowHeightsChange = + this.props.rowHeights !== undefined && + !Utils.compareSparseArrays(this.props.rowHeights, prevState.rowHeights); + const shouldInvalidateGrid = didChildrenChange || - !Utils.compareSparseArrays(this.props.columnWidths, prevState.columnWidths) || - !Utils.compareSparseArrays(this.props.rowHeights, prevState.rowHeights) || + didCellRendererDependenciesChange || + didColumnWidthsChange || + didRowHeightsChange || this.props.numRows !== prevProps.numRows || (this.props.forceRerenderOnSelectionChange && this.props.selectedRegions !== prevProps.selectedRegions); @@ -577,6 +606,11 @@ export class Table2 extends AbstractComponent2 { - // the first onCompleteRender is triggered before the viewportRect is - // defined and the second after the viewportRect has been set. the cells + // The first onCompleteRender is triggered before the viewportRect is + // defined and the second after the viewportRect has been set. The cells // will only actually render once the viewportRect is defined though, so // we defer invoking onCompleteRender until that check passes. - if (this.state.viewportRect != null && this.state.didHeadersMount) { + + // Additional note: we run into an unfortunate race condition between the order of execution + // of this callback and this.handleHeaderMounted(...). The setState() call in the latter + // does not update this.state quickly enough for us to query for the new state here, so instead + // we read the private member variables which are the dependent parts of that "didHeadersMount" + // state. + const didHeadersMount = this.didColumnHeaderMount && this.didRowHeaderMount; + if (this.state.viewportRect != null && didHeadersMount) { this.props.onCompleteRender?.(); this.didCompletelyMount = true; } diff --git a/packages/table/test/table2Tests.tsx b/packages/table/test/table2Tests.tsx index c7f7773a22..ce82199416 100644 --- a/packages/table/test/table2Tests.tsx +++ b/packages/table/test/table2Tests.tsx @@ -703,7 +703,7 @@ describe("", function (this) { , ); - expect(onCompleteRenderSpy.callCount, "call count on mount").to.equal(1); + expect(onCompleteRenderSpy.callCount, "call count on mount").to.equal(2); }); it("triggers immediately on mount/update with RenderMode.BATCH for very small batches", () => { @@ -717,9 +717,9 @@ describe("", function (this) { , ); - expect(onCompleteRenderSpy.callCount, "call count on mount").to.equal(1); + expect(onCompleteRenderSpy.callCount, "call count on mount").to.equal(2); table.setProps({ numRows: 2 }); // still small enough to fit in one batch - expect(onCompleteRenderSpy.callCount, "call count on update").to.equal(2); + expect(onCompleteRenderSpy.callCount, "call count on update").to.equal(3); }); }); @@ -2068,6 +2068,54 @@ describe("", function (this) { } }); + describe("EXPERIMENTAL: cellRendererDependencies", () => { + it("Does not re-render cells when dependencies don't change", () => { + const dependencies: any[] = ["stable"]; + const cellRenderer = sinon.stub().returns(foo); + const table = mount( + + + , + ); + const originalCallCount = cellRenderer.callCount; + + // replace the single dependency with a shallow-equal value + dependencies[0] = "stable"; + table.setProps({ cellRendererDependencies: dependencies }); + + expect(cellRenderer.callCount).to.be.equal( + originalCallCount, + "cellRenderer should not have been called again when cellRendererDependencies are the same", + ); + }); + + it("Does re-render cells when dependencies change", () => { + const dependencies: string[] = ["some data from external store"]; + const cellRenderer = () => {dependencies[0]}; + const table = mount( + + + , + ); + expect(getFirstCellText()).to.equal(dependencies[0]); + + // replace the single dependency with a shallow-equal value + dependencies[0] = "new data from external store"; + table.setProps({ cellRendererDependencies: dependencies.slice() }); + expect(getFirstCellText()).to.equal( + dependencies[0], + "cellRenderer should have been called again when cellRendererDependencies changed", + ); + + function getFirstCellText() { + return table + .find(`.${Classes.rowCellIndexClass(0)}.${Classes.columnCellIndexClass(0)}`) + .hostNodes() + .text(); + } + }); + }); + function renderDummyCell() { return gg; }