Skip to content

refactor: convert controlUtils to TypeScript (2 of 2) #13520

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
ktmud merged 4 commits into from
Mar 12, 2021
Merged

refactor: convert controlUtils to TypeScript (2 of 2) #13520

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
3f78ef4
refactor: convert controlUtils to TypeScript (2 of 2)
ktmud Mar 8, 2021
1541fe7
Update npm pacakges
ktmud Mar 10, 2021
2b20293
Fix typing
ktmud Mar 10, 2021
5268258
Fix unit test
ktmud Mar 10, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
546 changes: 262 additions & 284 deletions superset-frontend/package-lock.json
View file
Open in desktop

Large diffs are not rendered by default.

52 changes: 26 additions & 26 deletions superset-frontend/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,34 +65,34 @@
"@babel/runtime-corejs3": "^7.12.5",
"@data-ui/sparkline": "^0.0.84",
"@emotion/core": "^10.0.35",
"@superset-ui/chart-controls": "^0.17.15",
"@superset-ui/chart-controls": "^0.17.17",
"@superset-ui/core": "^0.17.15",
"@superset-ui/legacy-plugin-chart-calendar": "^0.17.15",
"@superset-ui/legacy-plugin-chart-chord": "^0.17.15",
"@superset-ui/legacy-plugin-chart-country-map": "^0.17.15",
"@superset-ui/legacy-plugin-chart-event-flow": "^0.17.15",
"@superset-ui/legacy-plugin-chart-force-directed": "^0.17.15",
"@superset-ui/legacy-plugin-chart-heatmap": "^0.17.15",
"@superset-ui/legacy-plugin-chart-histogram": "^0.17.15",
"@superset-ui/legacy-plugin-chart-horizon": "^0.17.15",
"@superset-ui/legacy-plugin-chart-map-box": "^0.17.15",
"@superset-ui/legacy-plugin-chart-paired-t-test": "^0.17.15",
"@superset-ui/legacy-plugin-chart-parallel-coordinates": "^0.17.15",
"@superset-ui/legacy-plugin-chart-partition": "^0.17.15",
"@superset-ui/legacy-plugin-chart-pivot-table": "^0.17.15",
"@superset-ui/legacy-plugin-chart-rose": "^0.17.15",
"@superset-ui/legacy-plugin-chart-sankey": "^0.17.15",
"@superset-ui/legacy-plugin-chart-sankey-loop": "^0.17.15",
"@superset-ui/legacy-plugin-chart-sunburst": "^0.17.15",
"@superset-ui/legacy-plugin-chart-treemap": "^0.17.15",
"@superset-ui/legacy-plugin-chart-world-map": "^0.17.15",
"@superset-ui/legacy-preset-chart-big-number": "^0.17.15",
"@superset-ui/legacy-plugin-chart-calendar": "^0.17.17",
"@superset-ui/legacy-plugin-chart-chord": "^0.17.17",
"@superset-ui/legacy-plugin-chart-country-map": "^0.17.17",
"@superset-ui/legacy-plugin-chart-event-flow": "^0.17.17",
"@superset-ui/legacy-plugin-chart-force-directed": "^0.17.17",
"@superset-ui/legacy-plugin-chart-heatmap": "^0.17.17",
"@superset-ui/legacy-plugin-chart-histogram": "^0.17.17",
"@superset-ui/legacy-plugin-chart-horizon": "^0.17.17",
"@superset-ui/legacy-plugin-chart-map-box": "^0.17.17",
"@superset-ui/legacy-plugin-chart-paired-t-test": "^0.17.17",
"@superset-ui/legacy-plugin-chart-parallel-coordinates": "^0.17.17",
"@superset-ui/legacy-plugin-chart-partition": "^0.17.17",
"@superset-ui/legacy-plugin-chart-pivot-table": "^0.17.17",
"@superset-ui/legacy-plugin-chart-rose": "^0.17.17",
"@superset-ui/legacy-plugin-chart-sankey": "^0.17.17",
"@superset-ui/legacy-plugin-chart-sankey-loop": "^0.17.17",
"@superset-ui/legacy-plugin-chart-sunburst": "^0.17.17",
"@superset-ui/legacy-plugin-chart-treemap": "^0.17.17",
"@superset-ui/legacy-plugin-chart-world-map": "^0.17.17",
"@superset-ui/legacy-preset-chart-big-number": "^0.17.17",
"@superset-ui/legacy-preset-chart-deckgl": "^0.4.6",
"@superset-ui/legacy-preset-chart-nvd3": "^0.17.15",
"@superset-ui/plugin-chart-echarts": "^0.17.15",
"@superset-ui/plugin-chart-table": "^0.17.15",
"@superset-ui/plugin-chart-word-cloud": "^0.17.15",
"@superset-ui/preset-chart-xy": "^0.17.15",
"@superset-ui/legacy-preset-chart-nvd3": "^0.17.17",
"@superset-ui/plugin-chart-echarts": "^0.17.17",
"@superset-ui/plugin-chart-table": "^0.17.17",
"@superset-ui/plugin-chart-word-cloud": "^0.17.17",
"@superset-ui/preset-chart-xy": "^0.17.17",
"@vx/responsive": "^0.0.195",
"abortcontroller-polyfill": "^1.1.9",
"antd": "^4.9.4",
Expand Down
71 changes: 38 additions & 33 deletions ...javascripts/explore/controlUtils_spec.jsx → ...javascripts/explore/controlUtils_spec.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,8 +16,13 @@
* specific language governing permissions and limitations
* under the License.
*/

import { getChartControlPanelRegistry, t } from '@superset-ui/core';
import {
ControlConfig,
ControlPanelState,
CustomControlItem,
DatasourceMeta,
} from '@superset-ui/chart-controls';
import {
getControlConfig,
getControlState,
Expand All @@ -30,13 +35,20 @@ import {
controlPanelSectionsChartOptionsTable,
} from 'spec/javascripts/explore/fixtures';

const getKnownControlConfig = (controlKey: string, vizType: string) =>
getControlConfig(controlKey, vizType) as ControlConfig;

const getKnownControlState = (...args: Parameters<typeof getControlState>) =>
getControlState(...args) as Exclude<ReturnType<typeof getControlState>, null>;

describe('controlUtils', () => {
const state = {
datasource: {
columns: ['a', 'b', 'c'],
const state: ControlPanelState = {
datasource: ({
columns: [{ column_name: 'a' }],
metrics: [{ metric_name: 'first' }, { metric_name: 'second' }],
},
} as unknown) as DatasourceMeta,
controls: {},
form_data: { datasource: '1__table', viz_type: 'table' },
};

beforeAll(() => {
Expand Down Expand Up @@ -66,15 +78,14 @@ describe('controlUtils', () => {
describe('getControlConfig', () => {
it('returns a valid spatial controlConfig', () => {
const spatialControl = getControlConfig('color_scheme', 'test-chart');
expect(spatialControl.type).toEqual('ColorSchemeControl');
expect(spatialControl?.type).toEqual('ColorSchemeControl');
});

it('overrides according to vizType', () => {
let control = getControlConfig('color_scheme', 'test-chart');
let control = getKnownControlConfig('color_scheme', 'test-chart');
expect(control.label).toEqual('Color Scheme');

// deck_polygon overrides and removes validators
control = getControlConfig('color_scheme', 'test-chart-override');
control = getKnownControlConfig('color_scheme', 'test-chart-override');
expect(control.label).toEqual('My beautiful colors');
});

Expand Down Expand Up @@ -102,27 +113,21 @@ describe('controlUtils', () => {

describe('applyMapStateToPropsToControl,', () => {
it('applies state to props as expected', () => {
let control = getControlConfig('all_columns', 'table');
let control = getKnownControlConfig('all_columns', 'table');
control = applyMapStateToPropsToControl(control, state);
expect(control.options).toEqual(['a', 'b', 'c']);
});

it('removes the mapStateToProps key from the object', () => {
let control = getControlConfig('all_columns', 'table');
control = applyMapStateToPropsToControl(control, state);
expect(control.mapStateToProps[0]).toBe(undefined);
expect(control.options).toEqual([{ column_name: 'a' }]);
});
});

describe('getControlState', () => {
it('to still have the functions', () => {
const control = getControlState('metrics', 'table', state, ['a']);
const control = getKnownControlState('metrics', 'table', state, 'a');
expect(typeof control.mapStateToProps).toBe('function');
expect(typeof control.validators[0]).toBe('function');
expect(typeof control.validators?.[0]).toBe('function');
});

it('to fix multi with non-array values', () => {
const control = getControlState('all_columns', 'table', state, 'a');
it('to make sure value is array', () => {
const control = getKnownControlState('all_columns', 'table', state, 'a');
expect(control.value).toEqual(['a']);
});

Expand All @@ -133,10 +138,10 @@ describe('controlUtils', () => {
state,
'stack',
);
expect(control.value).toBe('stack');
expect(control?.value).toBe('stack');

control = getControlState('stacked_style', 'test-chart', state, 'FOO');
expect(control.value).toBeNull();
expect(control?.value).toBeNull();
});

it('returns null for non-existent field', () => {
Expand All @@ -146,19 +151,19 @@ describe('controlUtils', () => {

it('applies the default function for metrics', () => {
const control = getControlState('metrics', 'table', state);
expect(control.default).toEqual(['first']);
expect(control?.default).toEqual(['first']);
});

it('applies the default function for metric', () => {
const control = getControlState('metric', 'table', state);
expect(control.default).toEqual('first');
expect(control?.default).toEqual('first');
});

it('applies the default function, prefers count if it exists', () => {
const stateWithCount = {
...state,
datasource: {
...state.datasource,
...(state.datasource as DatasourceMeta),
metrics: [
{ metric_name: 'first' },
{ metric_name: 'second' },
Expand All @@ -167,23 +172,23 @@ describe('controlUtils', () => {
},
};
const control = getControlState('metrics', 'table', stateWithCount);
expect(control.default).toEqual(['count']);
expect(control?.default).toEqual(['count']);
});

it('should not apply mapStateToProps when initializing', () => {
const control = getControlState('metrics', 'table', {
...state,
controls: undefined,
});
expect(typeof control.default).toBe('function');
expect(control.value).toBe(undefined);
expect(typeof control?.default).toBe('function');
expect(control?.value).toBe(undefined);
});
});

describe('validateControl', () => {
it('validates the control, returns an error if empty', () => {
const control = getControlState('metric', 'table', state, null);
expect(control.validationErrors).toEqual(['cannot be empty']);
expect(control?.validationErrors).toEqual(['cannot be empty']);
});
it('should not validate if control panel is initializing', () => {
const control = getControlState(
Expand All @@ -192,7 +197,7 @@ describe('controlUtils', () => {
{ ...state, controls: undefined },
undefined,
);
expect(control.validationErrors).toBeUndefined();
expect(control?.validationErrors).toBeUndefined();
});
});

Expand All @@ -209,14 +214,14 @@ describe('controlUtils', () => {
let controlItem = findControlItem(
Copy link
Contributor

@kristw kristw Mar 12, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we do let controlItem: CustomControlItem?

Copy link
Member Author

@ktmud ktmud Mar 12, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately no, because findControlItem may also return null or ReactElement. Using simple type declaration will throw an error:

Screen Shot 2021-03-11 at 5 18 26 PM

controlPanelSectionsChartOptions,
'rose_area_proportion',
);
) as CustomControlItem;
expect(controlItem.name).toEqual('rose_area_proportion');
expect(controlItem).toHaveProperty('config');

controlItem = findControlItem(
controlPanelSectionsChartOptions,
'stacked_style',
);
) as CustomControlItem;
expect(controlItem.name).toEqual('stacked_style');
expect(controlItem).toHaveProperty('config');
});
Expand Down
15 changes: 10 additions & 5 deletions ...end/spec/javascripts/explore/fixtures.jsx → ...end/spec/javascripts/explore/fixtures.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,9 +19,14 @@

import React from 'react';
import { t } from '@superset-ui/core';
import { ColumnOption } from '@superset-ui/chart-controls';
import {
ColumnMeta,
ColumnOption,
ControlConfig,
ControlPanelSectionConfig,
} from '@superset-ui/chart-controls';

export const controlPanelSectionsChartOptions = [
export const controlPanelSectionsChartOptions: ControlPanelSectionConfig[] = [
{
label: t('Chart Options'),
expanded: true,
Expand Down Expand Up @@ -63,15 +68,15 @@ export const controlPanelSectionsChartOptions = [
},
];

export const controlPanelSectionsChartOptionsOnlyColorScheme = [
export const controlPanelSectionsChartOptionsOnlyColorScheme: ControlPanelSectionConfig[] = [
{
label: t('Chart Options'),
expanded: true,
controlSetRows: [['color_scheme']],
},
];

export const controlPanelSectionsChartOptionsTable = [
export const controlPanelSectionsChartOptionsTable: ControlPanelSectionConfig[] = [
{
label: t('Chart Options'),
expanded: true,
Expand All @@ -96,7 +101,7 @@ export const controlPanelSectionsChartOptionsTable = [
}),
commaChoosesOption: false,
freeForm: true,
},
} as ControlConfig<'SelectControl', ColumnMeta>,
},
],
],
Expand Down
28 changes: 14 additions & 14 deletions superset-frontend/src/explore/controlUtils/getControlConfig.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,20 +23,6 @@ import {
expandControlConfig,
} from '@superset-ui/chart-controls';

const getMemoizedControlConfig = memoizeOne(
(controlKey, controlPanelConfig) => {
const {
controlOverrides = {},
controlPanelSections = [],
} = controlPanelConfig;
const control = expandControlConfig(
findControlItem(controlPanelSections, controlKey),
controlOverrides,
);
return control && 'config' in control ? control.config : control;
},
);

/**
* Find control item from control panel config.
*/
Expand All @@ -59,6 +45,20 @@ export function findControlItem(
);
}

const getMemoizedControlConfig = memoizeOne(
Copy link
Contributor

@kristw kristw Mar 11, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this one just changing location?

Copy link
Member Author

@ktmud ktmud Mar 12, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, because ESLint complains function being used before definition.

(controlKey, controlPanelConfig) => {
const {
controlOverrides = {},
controlPanelSections = [],
} = controlPanelConfig;
const control = expandControlConfig(
findControlItem(controlPanelSections, controlKey),
controlOverrides,
);
return control && 'config' in control ? control.config : control;
},
);

export const getControlConfig = function getControlConfig(
controlKey: string,
vizType: string,
Expand Down
Loading