From e0798754e719c913d1cf29bfce9abca66225f0a4 Mon Sep 17 00:00:00 2001 From: Matthew Runyon Date: Mon, 13 May 2024 11:31:41 -0500 Subject: [PATCH] Address review comments --- .../js/src/PlotlyExpressChartModel.test.ts | 79 ++++++++++++------- .../src/js/src/PlotlyExpressChartModel.ts | 12 ++- 2 files changed, 60 insertions(+), 31 deletions(-) diff --git a/plugins/plotly-express/src/js/src/PlotlyExpressChartModel.test.ts b/plugins/plotly-express/src/js/src/PlotlyExpressChartModel.test.ts index 5b3df50ff..ddf43b5d5 100644 --- a/plugins/plotly-express/src/js/src/PlotlyExpressChartModel.test.ts +++ b/plugins/plotly-express/src/js/src/PlotlyExpressChartModel.test.ts @@ -1,18 +1,10 @@ import type { Layout } from 'plotly.js'; import { dh as DhType } from '@deephaven/jsapi-types'; import { TestUtils } from '@deephaven/utils'; +import { ChartModel } from '@deephaven/chart'; import { PlotlyExpressChartModel } from './PlotlyExpressChartModel'; import { PlotlyChartWidgetData } from './PlotlyExpressChartUtils'; -jest.mock('./PlotlyExpressChartModel', () => { - const original = jest.requireActual('./PlotlyExpressChartModel'); - original.PlotlyExpressChartModel.prototype.fireDownsampleStart = jest.fn(); - original.PlotlyExpressChartModel.prototype.fireDownsampleFinish = jest.fn(); - original.PlotlyExpressChartModel.prototype.fireDownsampleFail = jest.fn(); - original.PlotlyExpressChartModel.prototype.fireDownsampleNeeded = jest.fn(); - return original; -}); - const SMALL_TABLE = TestUtils.createMockProxy({ columns: [{ name: 'x' }, { name: 'y' }] as DhType.Column[], size: 500, @@ -155,11 +147,13 @@ describe('PlotlyExpressChartModel', () => { jest.fn() ); - await chartModel.subscribe(jest.fn()); + const mockSubscribe = jest.fn(); + await chartModel.subscribe(mockSubscribe); await new Promise(process.nextTick); // Subscribe and addTable are async expect(mockDownsample).toHaveBeenCalledTimes(0); - expect(chartModel.fireDownsampleStart).toHaveBeenCalledTimes(0); - expect(chartModel.fireDownsampleFinish).toHaveBeenCalledTimes(0); + expect(mockSubscribe).toHaveBeenCalledTimes(0); + // expect(chartModel.fireDownsampleStart).toHaveBeenCalledTimes(0); + // expect(chartModel.fireDownsampleFinish).toHaveBeenCalledTimes(0); }); it('should downsample line charts when the table is big', async () => { @@ -170,11 +164,18 @@ describe('PlotlyExpressChartModel', () => { jest.fn() ); - await chartModel.subscribe(jest.fn()); + const mockSubscribe = jest.fn(); + await chartModel.subscribe(mockSubscribe); await new Promise(process.nextTick); // Subscribe and addTable are async expect(mockDownsample).toHaveBeenCalledTimes(1); - expect(chartModel.fireDownsampleStart).toHaveBeenCalledTimes(1); - expect(chartModel.fireDownsampleFinish).toHaveBeenCalledTimes(1); + expect(mockSubscribe).toHaveBeenCalledTimes(2); + expect(mockSubscribe).toHaveBeenNthCalledWith( + 1, + new CustomEvent(ChartModel.EVENT_DOWNSAMPLESTARTED) + ); + expect(mockSubscribe).toHaveBeenLastCalledWith( + new CustomEvent(ChartModel.EVENT_DOWNSAMPLEFINISHED) + ); }); it('should downsample only the required tables', async () => { @@ -189,11 +190,26 @@ describe('PlotlyExpressChartModel', () => { jest.fn() ); - await chartModel.subscribe(jest.fn()); + const mockSubscribe = jest.fn(); + await chartModel.subscribe(mockSubscribe); await new Promise(process.nextTick); // Subscribe and addTable are async expect(mockDownsample).toHaveBeenCalledTimes(2); - expect(chartModel.fireDownsampleStart).toHaveBeenCalledTimes(2); - expect(chartModel.fireDownsampleFinish).toHaveBeenCalledTimes(2); + expect(mockSubscribe).toHaveBeenCalledTimes(4); + expect(mockSubscribe).toHaveBeenNthCalledWith( + 1, + new CustomEvent(ChartModel.EVENT_DOWNSAMPLESTARTED) + ); + expect(mockSubscribe).toHaveBeenNthCalledWith( + 2, + new CustomEvent(ChartModel.EVENT_DOWNSAMPLESTARTED) + ); + expect(mockSubscribe).toHaveBeenNthCalledWith( + 3, + new CustomEvent(ChartModel.EVENT_DOWNSAMPLEFINISHED) + ); + expect(mockSubscribe).toHaveBeenLastCalledWith( + new CustomEvent(ChartModel.EVENT_DOWNSAMPLEFINISHED) + ); }); it('should fail to downsample for non-line plots', async () => { @@ -204,12 +220,14 @@ describe('PlotlyExpressChartModel', () => { jest.fn() ); - await chartModel.subscribe(jest.fn()); + const mockSubscribe = jest.fn(); + await chartModel.subscribe(mockSubscribe); await new Promise(process.nextTick); // Subscribe and addTable are async expect(mockDownsample).toHaveBeenCalledTimes(0); - expect(chartModel.fireDownsampleStart).toHaveBeenCalledTimes(0); - expect(chartModel.fireDownsampleFinish).toHaveBeenCalledTimes(0); - expect(chartModel.fireDownsampleFail).toHaveBeenCalledTimes(1); + expect(mockSubscribe).toHaveBeenCalledTimes(1); + expect(mockSubscribe).toHaveBeenCalledWith( + new CustomEvent(ChartModel.EVENT_DOWNSAMPLEFAILED) + ); }); it('should fetch non-line plots under the max threshold with downsampling disabled', async () => { @@ -220,13 +238,12 @@ describe('PlotlyExpressChartModel', () => { jest.fn() ); + const mockSubscribe = jest.fn(); chartModel.isDownsamplingDisabled = true; - await chartModel.subscribe(jest.fn()); + await chartModel.subscribe(mockSubscribe); await new Promise(process.nextTick); // Subscribe and addTable are async expect(mockDownsample).toHaveBeenCalledTimes(0); - expect(chartModel.fireDownsampleStart).toHaveBeenCalledTimes(0); - expect(chartModel.fireDownsampleFinish).toHaveBeenCalledTimes(0); - expect(chartModel.fireDownsampleFail).toHaveBeenCalledTimes(0); + expect(mockSubscribe).toHaveBeenCalledTimes(0); }); it('should not fetch non-line plots over the max threshold with downsampling disabled', async () => { @@ -237,12 +254,14 @@ describe('PlotlyExpressChartModel', () => { jest.fn() ); + const mockSubscribe = jest.fn(); chartModel.isDownsamplingDisabled = true; - await chartModel.subscribe(jest.fn()); + await chartModel.subscribe(mockSubscribe); await new Promise(process.nextTick); // Subscribe and addTable are async expect(mockDownsample).toHaveBeenCalledTimes(0); - expect(chartModel.fireDownsampleStart).toHaveBeenCalledTimes(0); - expect(chartModel.fireDownsampleFinish).toHaveBeenCalledTimes(0); - expect(chartModel.fireDownsampleFail).toHaveBeenCalledTimes(1); + expect(mockSubscribe).toHaveBeenCalledTimes(1); + expect(mockSubscribe).toHaveBeenCalledWith( + new CustomEvent(ChartModel.EVENT_DOWNSAMPLEFAILED) + ); }); }); diff --git a/plugins/plotly-express/src/js/src/PlotlyExpressChartModel.ts b/plugins/plotly-express/src/js/src/PlotlyExpressChartModel.ts index 0695eef4c..152b7d096 100644 --- a/plugins/plotly-express/src/js/src/PlotlyExpressChartModel.ts +++ b/plugins/plotly-express/src/js/src/PlotlyExpressChartModel.ts @@ -19,8 +19,18 @@ import { const log = Log.module('@deephaven/js-plugin-plotly-express.ChartModel'); export class PlotlyExpressChartModel extends ChartModel { - static AUTO_DOWNSAMPLE_SIZE = 30000; + /** + * The size at which the chart will automatically downsample the data if it can be downsampled. + * If it cannot be downsampled, but the size is below MAX_FETCH_SIZE, + * the chart will show a confirmation to fetch the data since it might be a slow operation. + */ + static AUTO_DOWNSAMPLE_SIZE = 30_000; + /** + * The maximum number of items that can be fetched from a table. + * If a table is larger than this, the chart will not be fetched. + * This is to prevent the chart from fetching too much data and crashing the browser. + */ static MAX_FETCH_SIZE = 1_000_000; static canFetch(table: DhType.Table): boolean {