refactor(explore): convert ControlPanelsContainer to typescript #13221

ktmud · 2021-02-18T19:11:08Z

SUMMARY

Convert ControlPanelsContainer and some other related components to TypeScript.

Big refactoring are coming to chart controls and the Explore page, in order to support reading query results in controls, which is needed for things like ad-hoc column formatting in table chart.

Also did some refactoring to redo #12782 and #12609 . See comments for details.

Upgraded superset-ui to v0.17.11 to bring in:
apache-superset/superset-ui#962
apache-superset/superset-ui#969

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

N/A

There should be no visible change.

TEST PLAN

CI and manual testing the chart control area.

ADDITIONAL INFORMATION

Has associated issue: Requires: fix(chart-controls): expose datasource for some controls apache-superset/superset-ui#969
Changes UI
Requires DB Migration.
Confirm DB Migration upgrade and downgrade tested.
Introduces new feature or API
Removes existing feature or API

ktmud · 2021-02-18T19:12:19Z

superset-frontend/src/explore/actions/exploreActions.ts

-export const REMOVE_CONTROL_PANEL_ALERT = 'REMOVE_CONTROL_PANEL_ALERT';
-export function removeControlPanelAlert() {
-  return { type: REMOVE_CONTROL_PANEL_ALERT };
-}


Cleaning up dead code.

This is the alert this action is trying to clear, which we are not using anymore.

ktmud · 2021-02-18T19:37:36Z

superset-frontend/src/explore/components/ControlPanelsContainer.tsx

@@ -161,29 +178,39 @@ class ControlPanelsContainer extends React.Component {
        validationErrors={validationErrors}
        actions={actions}
        formData={provideFormDataToProps ? formData : null}
-        datasource={formData?.datasource}


Remove this exposed datasource prop from #12609 since it's already available in formData, plus The typing would not conform with datasource for DatasourceControl, which uses the assigns datasource prop with the whole metadata of a datasource in mapStateToProps.

cc @pkdotson @zhaoyongjie

junlincc · 2021-02-18T21:00:02Z

yeah, you are getting ready to work on dynamic controls thanks! ♥️

ktmud · 2021-02-20T00:14:06Z

Moved debounce delay updates to another PR: #13250

Will rebase once merged.

ktmud · 2021-02-20T01:49:59Z

superset-frontend/src/explore/components/controls/DateFilterControl/DateFilterControl.tsx

  name: string;
  onChange: (timeRange: string) => void;
  value?: string;
  endpoints?: TimeRangeEndpoints;
-  datasource?: string;
+  datasource?: DatasourceMeta;


Will update the shared control to pass in datasource object instead of datasource string from form_data.

ktmud · 2021-02-20T01:51:24Z

superset-frontend/src/explore/components/controls/FilterControl/AdhocFilterControl.jsx

@@ -270,7 +262,7 @@ class AdhocFilterControl extends React.Component {
  optionsForSelect(props) {
    const options = [
      ...props.columns,
-      ...[...(props.formData.metrics || []), props.formData.metric].map(
+      ...[...(props.selectedMetrics || [])].map(


This is used for allowing selecting adhoc metrics in adhoc filters, but it has stopping working:
#13249

The new selectedMetrics will be added by a superset-ui PR.

ktmud · 2021-02-20T01:53:25Z

superset-frontend/src/explore/components/controls/TextControl.tsx

+    const { value } = event.target as HTMLInputElement;
+    this.setState({ value }, () => {
+      this.debouncedOnChange(value);
+    });


Did more refactoring based on: #13211 (comment)

ktmud · 2021-02-24T01:39:20Z

superset-frontend/.eslintrc.js

@@ -226,6 +227,7 @@ module.exports = {
    'no-mixed-operators': 0,
    'no-multi-assign': 0,
    'no-multi-spaces': 0,
+    'no-nested-ternary': 0,


Disabling as we have seen fairly often nested-ternary in TypeScript extends. It should not be that hard to understand the code with prettier's help.

nested ternary was used in this code:

const controlName = typeof item === 'string' ? item : item && 'name' in item ? item.name : null;

Yeah, I'm a fan of nested ternaries when used sparingly. however, couldn't this be simplified to:

const controlName = typeof item === 'string' ? item : (item || {})?.name ?? null;

or something like that?

Not sure if that's really cleaner, but it doesn't use a nested ternary

'name' in item is needed for type inference so to avoid forced type conversion as item can be of many types, otherwise this could also be as simple as item?.name || item || null.

Here's the error with your approach:

codecov-io · 2021-02-24T09:58:31Z

Codecov Report

Merging #13221 (0af0aab) into master (b4ca39c) will decrease coverage by 4.39%.
The diff coverage is 84.76%.

@@            Coverage Diff             @@
##           master   #13221      +/-   ##
==========================================
- Coverage   77.12%   72.73%   -4.40%     
==========================================
  Files         894      595     -299     
  Lines       45672    21247   -24425     
  Branches     5492     5490       -2     
==========================================
- Hits        35223    15453   -19770     
+ Misses      10325     5668    -4657     
- Partials      124      126       +2

Flag	Coverage Δ
cypress	`57.69% <81.11%> (-0.40%)`	⬇️
hive	`?`
javascript	`62.48% <58.09%> (+0.08%)`	⬆️
mysql	`?`
postgres	`?`
presto	`?`
python	`?`
sqlite	`?`

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
superset-frontend/src/constants.ts	`100.00% <ø> (ø)`
...nd/src/explore/components/ExploreViewContainer.jsx	`78.04% <ø> (-0.14%)`	⬇️
.../explore/components/controls/CollectionControl.jsx	`45.45% <ø> (+3.78%)`	⬆️
.../controls/MetricControl/AdhocMetricEditPopover.jsx	`79.54% <ø> (+0.44%)`	⬆️
...nents/controls/MetricControl/AdhocMetricOption.jsx	`100.00% <ø> (ø)`
...ntrols/MetricControl/AdhocMetricPopoverTrigger.tsx	`94.73% <ø> (ø)`
...uperset-frontend/src/explore/controlUtils/index.js	`95.41% <ø> (ø)`
superset-frontend/src/explore/controls.jsx	`27.16% <0.00%> (+0.33%)`	⬆️
...et-frontend/src/explore/reducers/exploreReducer.js	`36.23% <0.00%> (-8.77%)`	⬇️
...ents/controls/FilterControl/AdhocFilterControl.jsx	`60.00% <50.00%> (+0.90%)`	⬆️
... and 318 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b4ca39c...430874c. Read the comment docs.

ktmud

@villebro @pkdotson @zhaoyongjie @kgabryje could you take a look at this when you have time?

Thanks!

ktmud · 2021-02-24T09:24:03Z

superset-frontend/cypress-base/cypress/integration/explore/visualizations/pivot_table.test.js

@@ -28,7 +28,7 @@ describe('Visualization > Pivot Table', () => {
    adhoc_filters: [],
    groupby: ['name'],
    columns: ['state'],
-    row_limit: 50000,
+    row_limit: 5000,


Reduce row limit to speed up the test (a little bit)

ktmud · 2021-02-24T09:25:00Z

superset-frontend/src/explore/components/ControlPanelsContainer.tsx

+            // for direct column select controls
+            controlState.valueKey === 'column_name' ||
+            // for all other controls
+            'columns' in controlState


Instead of check control types, I check whether the control uses column select.

ktmud · 2021-02-24T09:26:06Z

superset-frontend/src/explore/components/ControlPanelsContainer.tsx

-    controls: explore.controls,
-    exploreState: explore,
-  };
-}


Move to inline function to use inferred types.

ktmud · 2021-02-24T09:28:15Z

superset-frontend/src/explore/components/controls/CollectionControl.jsx

@@ -69,12 +69,6 @@ export default class CollectionControl extends React.Component {
    this.onAdd = this.onAdd.bind(this);
  }

-  componentDidUpdate(prevProps) {
-    if (prevProps.datasource.name !== this.props.datasource.name) {


This one does not work anyway (i.e. change datasource will not reset Time-series Table columns, and the filters collection in FilterBox) since previously props.datasource is passed as a string.

I don't plan to implement this reset because these control values should not be reset when changing datasource.

ktmud · 2021-02-24T09:30:10Z

superset-frontend/src/explore/components/controls/DateFilterControl/DateFilterControl.tsx

-          frame === 'No filter'
+          guessedFrame === 'Common' ||
+          guessedFrame === 'Calendar' ||
+          guessedFrame === 'No filter'


Always use guessedFrame to tooltip and actual time range display because when passing in a new value and the popover is not open, the current frame may be inconsistent with the actual value.

This makes sure Last week is always displayed in the pill instead of tooltip.

ktmud · 2021-02-24T09:34:45Z

superset-frontend/src/explore/components/controls/MetricControl/AdhocMetricEditPopover.jsx

@@ -123,9 +123,6 @@ export default class AdhocMetricEditPopover extends React.PureComponent {
        adhocMetricLabel: this.state.adhocMetric?.getDefaultLabel(),
      });
    }
-    if (prevProps.datasource !== this.props.datasource) {
-      this.props.onChange(null);
-    }


Clean up the datasource prop from adhoc metric popovers as they don't seem to affect the ability of the controls to reset values when changing datasource.

@pkdotson do you remember what was these for when you added them?

ktmud · 2021-02-24T09:36:15Z

superset-frontend/src/explore/constants.ts

@@ -55,12 +55,7 @@ export const HAVING_OPERATORS = [
  OPERATORS['>='],
  OPERATORS['<='],
 ];
-export const MULTI_OPERATORS = new Set([
-  OPERATORS.in,


OPERATORS.in and OPERATORS['not in'] are undefined, as identified by converting to typescript, so cleaning them up. AchocFilter already always convert the operator into uppercase.

ktmud · 2021-02-24T09:36:42Z

superset-frontend/src/explore/controlUtils/getFormDataFromControls.ts

+    formData[controlName] = control.value;
+  });
+  return formData;
+}


Split functions in controlUtils to TypeScript one by one...

ktmud · 2021-02-24T09:37:00Z

superset-frontend/src/explore/reducers/exploreReducer.js

+        action.datasource.type !== state.datasource.type
+      ) {
+        newFormData.time_range = DEFAULT_TIME_RANGE;
+      }


Instead of checking control types, I now check whether a control depends on datasource columns.

ktmud · 2021-02-24T19:06:22Z

superset-frontend/src/explore/components/ControlPanelsContainer.tsx

-        }
-      });
-    }
-  }


Moved the resetting logic to reducers as each of these setControlValue call here will cause another re-rendering.

ktmud · 2021-02-25T04:02:07Z

Close to re-trigger full CI to fix test coverage report.

ktmud · 2021-02-26T04:23:48Z

I also noticed a few usages of type assertion throughout. I assume they were there for a good reason, but it makes me a bit nervous. Maybe go through one more time to ensure they're all needed?

Good call. I did a sweep and managed to find one place that I could optimize: caf3b60

michael-s-molina · 2021-03-01T13:52:58Z

@ktmud I found this typescript error:

It's happening because tooltip is declared as a string in InfoTooltipWithTrigger but the description is declared as ReactNode in ControlPanelSectionConfig.

I was going to fix it, but I was afraid of not having all the necessary context since this touches superset-ui plugins.

Shouldn't CI catch this?

ktmud · 2021-03-01T20:02:55Z

@michael-s-molina Did you run npm install or was your superset-ui/chart-controls linked to a different version? CI didn't catch it because it was actually working?

I'm pretty sure this was updated here: https://github.com/apache-superset/superset-ui/pull/962/files#diff-efc1b32a915b3738e7689b97eab6576ac7b206bc95d46e1ecaac6c8646287a65R27

The superset-ui change was supposed to be added in this PR but another PR has upgraded them anyway.

michael-s-molina · 2021-03-01T20:12:19Z

@ktmud Strangely I ran npm install and it didn't work. It only worked after deleting @superset-ui in node_modules and reinstalling it. At least it's working now. Thanks.

…he#13221)

pull-request-size bot added the size/L label Feb 18, 2021

ktmud commented Feb 18, 2021

View reviewed changes

junlincc added the explore:control Related to the controls panel of Explore label Feb 18, 2021

ktmud force-pushed the ts-control-panel branch from 705b500 to 91cd91c Compare February 19, 2021 19:31

This was referenced Feb 19, 2021

fix(explore): Reset values in TextControl only when datasource changes #13211

Merged

[Explore][Control] Allow selecting ad-hoc metrics in filters #13249

Closed

ktmud force-pushed the ts-control-panel branch from 2679eae to 53964e5 Compare February 20, 2021 00:08

junlincc requested a review from zhaoyongjie February 20, 2021 01:47

ktmud force-pushed the ts-control-panel branch from 53964e5 to 1f4b063 Compare February 20, 2021 01:48

ktmud commented Feb 20, 2021

View reviewed changes

ktmud mentioned this pull request Feb 20, 2021

fix(chart-controls): expose datasource for some controls apache-superset/superset-ui#969

Merged

ktmud force-pushed the ts-control-panel branch 2 times, most recently from 09ebe62 to b8725fb Compare February 24, 2021 01:37

ktmud requested review from villebro, kristw and etr2460 February 24, 2021 01:38

ktmud commented Feb 24, 2021

View reviewed changes

pull-request-size bot added size/XL and removed size/L labels Feb 24, 2021

ktmud force-pushed the ts-control-panel branch from 2dd459a to 71a3b5d Compare February 24, 2021 09:03

ktmud force-pushed the ts-control-panel branch from 718ff04 to a2630e1 Compare February 24, 2021 19:05

ktmud commented Feb 24, 2021

View reviewed changes

ktmud force-pushed the ts-control-panel branch from 9efcc03 to cb1dd58 Compare February 25, 2021 02:22

ktmud closed this Feb 25, 2021

ktmud reopened this Feb 25, 2021

ktmud added 18 commits February 26, 2021 09:15

refactor(explore): convert ControlPanelsContainer to typescript

624f994

Fix lint errors

2f00a4f

keep provideFormDataToProps to avoid re-render

d01a7b9

Fix lint errors

187bcaf

Fix many typing errors

dd8f2cc

Clean up

aa6be68

Clean up dead import

88eac59

move getFormDataFromControls to ts

25c509a

Fix tests

0b1d301

Fix time range filter value reset

1901dde

more clean up for apache#12782

fd18840

Use reducer to reset control values

4a3b616

Fix JS error

3520efa

Reset control value when initializing explore state

634b84c

Refactor with clearer typing

ab8dfce

Simplify the code

e4e6172

Fix annotation layer test

891fadc

Better type enforcement

430874c

ktmud force-pushed the ts-control-panel branch from caf3b60 to 430874c Compare February 26, 2021 19:17

ktmud merged commit 3c62069 into apache:master Feb 28, 2021

ktmud deleted the ts-control-panel branch February 28, 2021 18:10

ktmud mentioned this pull request Feb 28, 2021

chore: convert chartReducer to TypeScript #13374

Merged

9 tasks

ktmud mentioned this pull request Mar 1, 2021

refactor: convert controlUtils to TypeScript (1 of 2) #13401

Merged

6 tasks

allanco91 pushed a commit to allanco91/superset that referenced this pull request May 21, 2021

refactor(explore): convert ControlPanelsContainer to typescript (apac…

b6d7555

…he#13221)

mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 1.2.0 labels Mar 12, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(explore): convert ControlPanelsContainer to typescript #13221

refactor(explore): convert ControlPanelsContainer to typescript #13221

ktmud commented Feb 18, 2021 •

edited

Loading

ktmud Feb 18, 2021

ktmud Feb 18, 2021 •

edited

Loading

junlincc commented Feb 18, 2021

ktmud commented Feb 20, 2021

ktmud Feb 20, 2021

ktmud Feb 20, 2021

ktmud Feb 20, 2021

ktmud Feb 24, 2021

ktmud Feb 24, 2021

etr2460 Feb 26, 2021

etr2460 Feb 26, 2021

ktmud Feb 26, 2021

codecov-io commented Feb 24, 2021 •

edited by codecov bot

Loading

ktmud left a comment

ktmud Feb 24, 2021

ktmud Feb 24, 2021

ktmud Feb 24, 2021

ktmud Feb 24, 2021

ktmud Feb 24, 2021

ktmud Feb 24, 2021

ktmud Feb 24, 2021

ktmud Feb 24, 2021

ktmud Feb 24, 2021

ktmud Feb 24, 2021

ktmud commented Feb 25, 2021

ktmud commented Feb 26, 2021

michael-s-molina commented Mar 1, 2021

ktmud commented Mar 1, 2021 •

edited

Loading

michael-s-molina commented Mar 1, 2021

refactor(explore): convert ControlPanelsContainer to typescript #13221

refactor(explore): convert ControlPanelsContainer to typescript #13221

Conversation

ktmud commented Feb 18, 2021 • edited Loading

SUMMARY

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TEST PLAN

ADDITIONAL INFORMATION

Choose a reason for hiding this comment

ktmud Feb 18, 2021 • edited Loading

Choose a reason for hiding this comment

junlincc commented Feb 18, 2021

ktmud commented Feb 20, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-io commented Feb 24, 2021 • edited by codecov bot Loading

Codecov Report

ktmud left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ktmud commented Feb 25, 2021

ktmud commented Feb 26, 2021

michael-s-molina commented Mar 1, 2021

ktmud commented Mar 1, 2021 • edited Loading

michael-s-molina commented Mar 1, 2021

ktmud commented Feb 18, 2021 •

edited

Loading

ktmud Feb 18, 2021 •

edited

Loading

codecov-io commented Feb 24, 2021 •

edited by codecov bot

Loading

ktmud commented Mar 1, 2021 •

edited

Loading