Skip to content
This repository has been archived by the owner on Nov 3, 2023. It is now read-only.

Commit

Permalink
fix: optimize mapStateToProps for chart controls (apache#10264)
Browse files Browse the repository at this point in the history
  • Loading branch information
ktmud authored and serenajiang committed Jul 9, 2020
1 parent 418ec8b commit 6e3b809
Show file tree
Hide file tree
Showing 6 changed files with 63 additions and 28 deletions.
Expand Up @@ -27,6 +27,7 @@ import {
getFormDataFromControls,
applyMapStateToPropsToControl,
getAllControlsState,
getControlsState,
} from 'src/explore/controlUtils';

describe('controlUtils', () => {
Expand Down Expand Up @@ -248,6 +249,14 @@ describe('controlUtils', () => {
const control = getControlState('metrics', 'table', stateWithCount);
expect(control.default).toEqual(['count']);
});

it('should not apply mapStateToProps when initializing', () => {
const control = getControlState('metrics', 'table', {
...state,
isInitializing: true,
});
expect(control.default).toEqual(null);
});
});

describe('validateControl', () => {
Expand Down
Expand Up @@ -201,7 +201,6 @@ describe('ExploreResultsButton', () => {
const calls = fetchMock.calls(visualizeEndpoint);
expect(calls).toHaveLength(1);
const formData = calls[0][1].body;

Object.keys(mockOptions).forEach(key => {
// eslint-disable-next-line no-unused-expressions
expect(formData.get(key)).toBeDefined();
Expand Down
46 changes: 32 additions & 14 deletions superset-frontend/src/explore/controlUtils.js
Expand Up @@ -88,15 +88,15 @@ export const getControlConfig = memoizeOne(function getControlConfig(
return control?.config || control;
});

export function applyMapStateToPropsToControl(control, state) {
if (control.mapStateToProps) {
const appliedControl = { ...control };
if (state) {
Object.assign(appliedControl, control.mapStateToProps(state, control));
}
return appliedControl;
export function applyMapStateToPropsToControl(controlState, controlPanelState) {
const { mapStateToProps } = controlState;
if (mapStateToProps && controlPanelState) {
return {
...controlState,
...mapStateToProps(controlPanelState, controlState),
};
}
return control;
return controlState;
}

function handleMissingChoice(control) {
Expand All @@ -121,19 +121,37 @@ function handleMissingChoice(control) {
return control;
}

export function getControlStateFromControlConfig(controlConfig, state, value) {
export function getControlStateFromControlConfig(
controlConfig,
controlPanelState,
value,
) {
// skip invalid config values
if (!controlConfig) {
return null;
}
const controlState = applyMapStateToPropsToControl(
{ ...controlConfig },
state,
);
let controlState = { ...controlConfig };
// only apply mapStateToProps when control states have been initialized
if (
controlPanelState &&
(controlPanelState.controls || !controlPanelState.isInitializing)
) {
controlState = applyMapStateToPropsToControl(
controlState,
controlPanelState,
);
}

// If default is a function, evaluate it
if (typeof controlState.default === 'function') {
controlState.default = controlState.default(controlState);
controlState.default = controlState.default(
controlState,
controlPanelState,
);
// if default is still a function, discard
if (typeof controlState.default === 'function') {
delete controlState.default;
}
}

// If a choice control went from multi=false to true, wrap value in array
Expand Down
1 change: 0 additions & 1 deletion superset-frontend/src/explore/controls.jsx
Expand Up @@ -316,7 +316,6 @@ export const controls = {
'filter below is applied against this column or ' +
'expression',
),
default: control => control.default,
clearable: false,
optionRenderer: c => <ColumnOption column={c} showType />,
valueRenderer: c => <ColumnOption column={c} />,
Expand Down
33 changes: 22 additions & 11 deletions superset-frontend/src/explore/reducers/getInitialState.js
Expand Up @@ -21,27 +21,41 @@ import shortid from 'shortid';
import getToastsFromPyFlashMessages from '../../messageToasts/utils/getToastsFromPyFlashMessages';
import { getChartKey } from '../exploreUtils';
import { getControlsState } from '../store';
import { getFormDataFromControls } from '../controlUtils';
import {
getFormDataFromControls,
applyMapStateToPropsToControl,
} from '../controlUtils';

export default function getInitialState(bootstrapData) {
const controls = getControlsState(bootstrapData, bootstrapData.form_data);
const rawFormData = { ...bootstrapData.form_data };

const { form_data: rawFormData } = bootstrapData;
const slice = bootstrapData.slice;
const sliceName = slice ? slice.slice_name : null;
const bootstrappedState = {
...bootstrapData,
sliceName,
common: {
flash_messages: bootstrapData.common.flash_messages,
conf: bootstrapData.common.conf,
},
rawFormData,
controls,
filterColumnOpts: [],
isDatasourceMetaLoading: false,
isStarred: false,
isInitializing: true,
};
const controls = getControlsState(bootstrappedState, rawFormData);
bootstrappedState.controls = controls;

const slice = bootstrappedState.slice;
const sliceName = slice ? slice.slice_name : null;
// apply initial mapStateToProps for all controls, must execute AFTER
// bootstrappedState has initialized `controls`. Order of execution is not
// guaranteed, so controls shouldn't rely on the each other's mapped state.
Object.entries(controls).forEach(([key, controlState]) => {
controls[key] = applyMapStateToPropsToControl(
controlState,
bootstrappedState,
);
});
bootstrappedState.isInitializing = false;

const sliceFormData = slice
? getFormDataFromControls(getControlsState(bootstrapData, slice.form_data))
Expand Down Expand Up @@ -69,10 +83,7 @@ export default function getInitialState(bootstrapData) {
dashboards: [],
saveModalAlert: null,
},
explore: {
...bootstrappedState,
sliceName,
},
explore: bootstrappedState,
impressionId: shortid.generate(),
messageToasts: getToastsFromPyFlashMessages(
(bootstrapData.common || {}).flash_messages || [],
Expand Down
1 change: 0 additions & 1 deletion superset-frontend/src/explore/store.js
Expand Up @@ -39,7 +39,6 @@ export function getControlsState(state, inputFormData) {
* adds value keys coming from inputFormData passed here. This can't be an action creator
* just yet because it's used in both the explore and dashboard views.
* */

// Getting a list of active control names for the current viz
const formData = { ...inputFormData };
const vizType = formData.viz_type || 'table';
Expand Down

0 comments on commit 6e3b809

Please sign in to comment.