From 95f9cd923756923f422d4104bf62398c3d5c2404 Mon Sep 17 00:00:00 2001 From: justinpark Date: Wed, 27 Aug 2025 16:25:36 -0700 Subject: [PATCH 1/6] fix(ui-core): Invalid postTransform process --- .../chart/components/SuperChartCore.test.tsx | 75 +++++++++++++++++++ .../src/chart/components/SuperChartCore.tsx | 53 ++++++++++--- .../src/components/Chart/ChartRenderer.jsx | 1 + .../components/Chart/ChartRenderer.test.jsx | 25 ++++++- 4 files changed, 140 insertions(+), 14 deletions(-) create mode 100644 superset-frontend/packages/superset-ui-core/src/chart/components/SuperChartCore.test.tsx diff --git a/superset-frontend/packages/superset-ui-core/src/chart/components/SuperChartCore.test.tsx b/superset-frontend/packages/superset-ui-core/src/chart/components/SuperChartCore.test.tsx new file mode 100644 index 000000000000..becc2a95c1ce --- /dev/null +++ b/superset-frontend/packages/superset-ui-core/src/chart/components/SuperChartCore.test.tsx @@ -0,0 +1,75 @@ +import { render, waitFor } from '@testing-library/react'; + +import { + ChartPlugin, + ChartMetadata, + ChartProps, + DatasourceType, + getChartComponentRegistry, +} from '@superset-ui/core'; + +import SuperChartCore from './SuperChartCore'; + +describe('processChartProps', () => { + const props = { + chartType: 'line', + }; + const FakeChart = () => test; + + beforeEach(() => { + const metadata = new ChartMetadata({ + name: 'test-chart', + thumbnail: '', + }); + const buildQuery = () => ({ + datasource: { id: 1, type: DatasourceType.Table }, + queries: [{ granularity: 'day' }], + force: false, + result_format: 'json', + result_type: 'full', + }); + const controlPanel = { abc: 1 }; + const plugin = new ChartPlugin({ + metadata, + Chart: FakeChart, + buildQuery, + controlPanel, + }); + plugin.configure({ key: props.chartType }).register(); + }); + + test('should return the same result for identical inputs (cache hit)', async () => { + const pre = jest.fn(x => x); + const transform = jest.fn(x => x); + const post = jest.fn(x => x); + expect(getChartComponentRegistry().get(props.chartType)).toBe(FakeChart); + + expect(pre).toHaveBeenCalledTimes(0); + const { rerender } = render( + , + ); + + await waitFor(() => expect(pre).toHaveBeenCalledTimes(1)); + expect(transform).toHaveBeenCalledTimes(1); + expect(post).toHaveBeenCalledTimes(1); + + const updatedPost = jest.fn(x => x); + + rerender( + , + ); + await waitFor(() => expect(updatedPost).toHaveBeenCalledTimes(1)); + expect(transform).toHaveBeenCalledTimes(1); + expect(pre).toHaveBeenCalledTimes(1); + }); +}); diff --git a/superset-frontend/packages/superset-ui-core/src/chart/components/SuperChartCore.tsx b/superset-frontend/packages/superset-ui-core/src/chart/components/SuperChartCore.tsx index f980ed13cf05..00232431fcae 100644 --- a/superset-frontend/packages/superset-ui-core/src/chart/components/SuperChartCore.tsx +++ b/superset-frontend/packages/superset-ui-core/src/chart/components/SuperChartCore.tsx @@ -89,25 +89,55 @@ export default class SuperChartCore extends PureComponent { * and return previous value * unless one of * - preTransformProps - * - transformProps - * - postTransformProps * - chartProps * is changed. */ - processChartProps = createSelector( + preSelector = createSelector( [ (input: { chartProps: ChartProps; preTransformProps?: PreTransformProps; - transformProps?: TransformProps; - postTransformProps?: PostTransformProps; }) => input.chartProps, input => input.preTransformProps, + ], + (chartProps, pre = IDENTITY) => pre(chartProps), + ); + + /** + * memoized function so it will not recompute + * and return previous value + * unless one of + * - transformProps + * - preprocessed result + * is changed. + */ + transformSelector = createSelector( + [ + (input: { chartProps: ChartProps; transformProps?: TransformProps }) => + input.chartProps, input => input.transformProps, + ], + (preprocessedChartProps, transform = IDENTITY) => + transform(preprocessedChartProps), + ); + + /** + * memoized function so it will not recompute + * and return previous value + * unless one of + * - postTransformProps + * - transformed result + * is changed. + */ + postSelector = createSelector( + [ + (input: { + chartProps: ChartProps; + postTransformProps?: PostTransformProps; + }) => input.chartProps, input => input.postTransformProps, ], - (chartProps, pre = IDENTITY, transform = IDENTITY, post = IDENTITY) => - post(transform(pre(chartProps))), + (transformedChartProps, post = IDENTITY) => post(transformedChartProps), ); /** @@ -156,10 +186,11 @@ export default class SuperChartCore extends PureComponent { return ( diff --git a/superset-frontend/src/components/Chart/ChartRenderer.jsx b/superset-frontend/src/components/Chart/ChartRenderer.jsx index c3e2f3a8e0ee..d10d7b0439a9 100644 --- a/superset-frontend/src/components/Chart/ChartRenderer.jsx +++ b/superset-frontend/src/components/Chart/ChartRenderer.jsx @@ -182,6 +182,7 @@ class ChartRenderer extends Component { this.props.formData.subcategories || nextProps.cacheBusterProp !== this.props.cacheBusterProp || nextProps.emitCrossFilters !== this.props.emitCrossFilters || + nextProps.postTransformProps !== this.props.postTransformProps || hasMatrixifyChanges() ); } diff --git a/superset-frontend/src/components/Chart/ChartRenderer.test.jsx b/superset-frontend/src/components/Chart/ChartRenderer.test.jsx index 90442564b596..3b406cb8c287 100644 --- a/superset-frontend/src/components/Chart/ChartRenderer.test.jsx +++ b/superset-frontend/src/components/Chart/ChartRenderer.test.jsx @@ -16,7 +16,7 @@ * specific language governing permissions and limitations * under the License. */ -import { render } from 'spec/helpers/testing-library'; +import { render, waitFor } from 'spec/helpers/testing-library'; import { ChartMetadata, getChartMetadataRegistry, @@ -27,8 +27,10 @@ import { ChartSource } from 'src/types/ChartSource'; jest.mock('@superset-ui/core', () => ({ ...jest.requireActual('@superset-ui/core'), - SuperChart: ({ formData }) => ( -
{JSON.stringify(formData)}
+ SuperChart: ({ postTransformProps = x => x, ...props }) => ( +
+ {JSON.stringify(postTransformProps(props).formData)} +
), })); @@ -119,6 +121,23 @@ test('should detect changes in matrixify properties', () => { }); }); +test('should detect changes in postTransformProps', () => { + const postTransformProps = jest.fn(x => x); + const initialProps = { + ...requiredProps, + queriesResponse: [{ data: 'initial' }], + chartStatus: 'success', + }; + const { rerender } = render(); + const updatedProps = { + ...initialProps, + postTransformProps, + }; + expect(postTransformProps).toHaveBeenCalledTimes(0); + rerender(); + expect(postTransformProps).toHaveBeenCalledTimes(1); +}); + test('should identify matrixify property changes correctly', () => { // Test that formData with different matrixify properties triggers updates const initialProps = { From 4aeb98f28cb7b01da3708e32b2ad3ba540f704db Mon Sep 17 00:00:00 2001 From: justinpark Date: Wed, 27 Aug 2025 16:40:16 -0700 Subject: [PATCH 2/6] missing license and spec description --- .../chart/components/SuperChartCore.test.tsx | 21 +++++++++++++++++-- 1 file changed, 19 insertions(+), 2 deletions(-) diff --git a/superset-frontend/packages/superset-ui-core/src/chart/components/SuperChartCore.test.tsx b/superset-frontend/packages/superset-ui-core/src/chart/components/SuperChartCore.test.tsx index becc2a95c1ce..a8362d2715b4 100644 --- a/superset-frontend/packages/superset-ui-core/src/chart/components/SuperChartCore.test.tsx +++ b/superset-frontend/packages/superset-ui-core/src/chart/components/SuperChartCore.test.tsx @@ -1,9 +1,26 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ import { render, waitFor } from '@testing-library/react'; import { ChartPlugin, ChartMetadata, - ChartProps, DatasourceType, getChartComponentRegistry, } from '@superset-ui/core'; @@ -38,7 +55,7 @@ describe('processChartProps', () => { plugin.configure({ key: props.chartType }).register(); }); - test('should return the same result for identical inputs (cache hit)', async () => { + test('should return the result from cache unless transformProps has changed', async () => { const pre = jest.fn(x => x); const transform = jest.fn(x => x); const post = jest.fn(x => x); From dd5fde211152b7d702a6941aea2ca122be8ad411 Mon Sep 17 00:00:00 2001 From: justinpark Date: Wed, 27 Aug 2025 17:39:31 -0700 Subject: [PATCH 3/6] restore processChartProps --- .../src/chart/components/SuperChartCore.tsx | 28 +++++++++++++++---- 1 file changed, 23 insertions(+), 5 deletions(-) diff --git a/superset-frontend/packages/superset-ui-core/src/chart/components/SuperChartCore.tsx b/superset-frontend/packages/superset-ui-core/src/chart/components/SuperChartCore.tsx index 00232431fcae..29e8dc98a862 100644 --- a/superset-frontend/packages/superset-ui-core/src/chart/components/SuperChartCore.tsx +++ b/superset-frontend/packages/superset-ui-core/src/chart/components/SuperChartCore.tsx @@ -140,6 +140,25 @@ export default class SuperChartCore extends PureComponent { (transformedChartProps, post = IDENTITY) => post(transformedChartProps), ); + processChartProps = ({ + chartProps, + preTransformProps, + transformProps, + postTransformProps, + }: { + chartProps: ChartProps; + preTransformProps?: PreTransformProps; + transformProps?: TransformProps; + postTransformProps?: PostTransformProps; + }) => + this.postSelector({ + chartProps: this.transformSelector({ + chartProps: this.preSelector({ chartProps, preTransformProps }), + transformProps, + }), + postTransformProps, + }); + /** * memoized function so it will not recompute * and return previous value @@ -186,11 +205,10 @@ export default class SuperChartCore extends PureComponent { return ( From 3d206352b1d607e4359259f6070ee92550c2e4b5 Mon Sep 17 00:00:00 2001 From: justinpark Date: Wed, 27 Aug 2025 18:14:08 -0700 Subject: [PATCH 4/6] lint fix --- .../src/chart/components/SuperChartCore.test.tsx | 2 +- superset-frontend/src/components/Chart/ChartRenderer.test.jsx | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/superset-frontend/packages/superset-ui-core/src/chart/components/SuperChartCore.test.tsx b/superset-frontend/packages/superset-ui-core/src/chart/components/SuperChartCore.test.tsx index a8362d2715b4..2801a336e58c 100644 --- a/superset-frontend/packages/superset-ui-core/src/chart/components/SuperChartCore.test.tsx +++ b/superset-frontend/packages/superset-ui-core/src/chart/components/SuperChartCore.test.tsx @@ -55,7 +55,7 @@ describe('processChartProps', () => { plugin.configure({ key: props.chartType }).register(); }); - test('should return the result from cache unless transformProps has changed', async () => { + it('should return the result from cache unless transformProps has changed', async () => { const pre = jest.fn(x => x); const transform = jest.fn(x => x); const post = jest.fn(x => x); diff --git a/superset-frontend/src/components/Chart/ChartRenderer.test.jsx b/superset-frontend/src/components/Chart/ChartRenderer.test.jsx index 3b406cb8c287..c5734fdcce05 100644 --- a/superset-frontend/src/components/Chart/ChartRenderer.test.jsx +++ b/superset-frontend/src/components/Chart/ChartRenderer.test.jsx @@ -16,7 +16,7 @@ * specific language governing permissions and limitations * under the License. */ -import { render, waitFor } from 'spec/helpers/testing-library'; +import { render } from 'spec/helpers/testing-library'; import { ChartMetadata, getChartMetadataRegistry, From 1c3d95596e8cbe4e08dfb89ea9a645528339f85f Mon Sep 17 00:00:00 2001 From: justinpark Date: Thu, 28 Aug 2025 09:00:25 -0700 Subject: [PATCH 5/6] update comments --- .../src/chart/components/SuperChartCore.tsx | 22 +++++++------------ 1 file changed, 8 insertions(+), 14 deletions(-) diff --git a/superset-frontend/packages/superset-ui-core/src/chart/components/SuperChartCore.tsx b/superset-frontend/packages/superset-ui-core/src/chart/components/SuperChartCore.tsx index 29e8dc98a862..3f847ea4b572 100644 --- a/superset-frontend/packages/superset-ui-core/src/chart/components/SuperChartCore.tsx +++ b/superset-frontend/packages/superset-ui-core/src/chart/components/SuperChartCore.tsx @@ -85,8 +85,7 @@ export default class SuperChartCore extends PureComponent { container?: HTMLElement | null; /** - * memoized function so it will not recompute - * and return previous value + * memoized function so it will not recompute and return previous value * unless one of * - preTransformProps * - chartProps @@ -104,12 +103,8 @@ export default class SuperChartCore extends PureComponent { ); /** - * memoized function so it will not recompute - * and return previous value - * unless one of - * - transformProps - * - preprocessed result - * is changed. + * memoized function so it will not recompute and return previous value + * unless one of the input arguments have changed. */ transformSelector = createSelector( [ @@ -122,12 +117,8 @@ export default class SuperChartCore extends PureComponent { ); /** - * memoized function so it will not recompute - * and return previous value - * unless one of - * - postTransformProps - * - transformed result - * is changed. + * memoized function so it will not recompute and return previous value + * unless one of the input arguments have changed. */ postSelector = createSelector( [ @@ -140,6 +131,9 @@ export default class SuperChartCore extends PureComponent { (transformedChartProps, post = IDENTITY) => post(transformedChartProps), ); + /** + * Using each memoized function to retrieve the computed chartProps + */ processChartProps = ({ chartProps, preTransformProps, From 7f3db1e93a6e93098a49bba6971a450338a11b81 Mon Sep 17 00:00:00 2001 From: justinpark Date: Thu, 28 Aug 2025 11:21:46 -0700 Subject: [PATCH 6/6] replace describe/it by test --- .../chart/components/SuperChartCore.test.tsx | 108 +++++++++--------- 1 file changed, 53 insertions(+), 55 deletions(-) diff --git a/superset-frontend/packages/superset-ui-core/src/chart/components/SuperChartCore.test.tsx b/superset-frontend/packages/superset-ui-core/src/chart/components/SuperChartCore.test.tsx index 2801a336e58c..4a77e184d5f4 100644 --- a/superset-frontend/packages/superset-ui-core/src/chart/components/SuperChartCore.test.tsx +++ b/superset-frontend/packages/superset-ui-core/src/chart/components/SuperChartCore.test.tsx @@ -27,66 +27,64 @@ import { import SuperChartCore from './SuperChartCore'; -describe('processChartProps', () => { - const props = { - chartType: 'line', - }; - const FakeChart = () => test; +const props = { + chartType: 'line', +}; +const FakeChart = () => test; - beforeEach(() => { - const metadata = new ChartMetadata({ - name: 'test-chart', - thumbnail: '', - }); - const buildQuery = () => ({ - datasource: { id: 1, type: DatasourceType.Table }, - queries: [{ granularity: 'day' }], - force: false, - result_format: 'json', - result_type: 'full', - }); - const controlPanel = { abc: 1 }; - const plugin = new ChartPlugin({ - metadata, - Chart: FakeChart, - buildQuery, - controlPanel, - }); - plugin.configure({ key: props.chartType }).register(); +beforeEach(() => { + const metadata = new ChartMetadata({ + name: 'test-chart', + thumbnail: '', }); + const buildQuery = () => ({ + datasource: { id: 1, type: DatasourceType.Table }, + queries: [{ granularity: 'day' }], + force: false, + result_format: 'json', + result_type: 'full', + }); + const controlPanel = { abc: 1 }; + const plugin = new ChartPlugin({ + metadata, + Chart: FakeChart, + buildQuery, + controlPanel, + }); + plugin.configure({ key: props.chartType }).register(); +}); - it('should return the result from cache unless transformProps has changed', async () => { - const pre = jest.fn(x => x); - const transform = jest.fn(x => x); - const post = jest.fn(x => x); - expect(getChartComponentRegistry().get(props.chartType)).toBe(FakeChart); +test('should return the result from cache unless transformProps has changed', async () => { + const pre = jest.fn(x => x); + const transform = jest.fn(x => x); + const post = jest.fn(x => x); + expect(getChartComponentRegistry().get(props.chartType)).toBe(FakeChart); - expect(pre).toHaveBeenCalledTimes(0); - const { rerender } = render( - , - ); + expect(pre).toHaveBeenCalledTimes(0); + const { rerender } = render( + , + ); - await waitFor(() => expect(pre).toHaveBeenCalledTimes(1)); - expect(transform).toHaveBeenCalledTimes(1); - expect(post).toHaveBeenCalledTimes(1); + await waitFor(() => expect(pre).toHaveBeenCalledTimes(1)); + expect(transform).toHaveBeenCalledTimes(1); + expect(post).toHaveBeenCalledTimes(1); - const updatedPost = jest.fn(x => x); + const updatedPost = jest.fn(x => x); - rerender( - , - ); - await waitFor(() => expect(updatedPost).toHaveBeenCalledTimes(1)); - expect(transform).toHaveBeenCalledTimes(1); - expect(pre).toHaveBeenCalledTimes(1); - }); + rerender( + , + ); + await waitFor(() => expect(updatedPost).toHaveBeenCalledTimes(1)); + expect(transform).toHaveBeenCalledTimes(1); + expect(pre).toHaveBeenCalledTimes(1); });