fix(explore): Prevent shared controls from checking feature flags outside React render #21315

codyml · 2022-09-02T22:16:55Z

SUMMARY

Feature flags are saved to a global window object and read using the isFeatureEnabled utility function, which returns true or false after checking the global window object. As noted in #21201, this process is prone to error because feature flags are set at JS module execution time and sometimes also read at JS module execution time, creating a race condition that can be triggered by a change in webpack bundling. This PR takes steps towards resolving one instance of this issue by moving the feature flag check to component render. Previously, shared chart controls were exported as an object of control config objects, each of which contained the control's React component to render, params used by the control-rendering engine, and props that the control's React component would be rendered with. For each shared control that had drag-and-drop functionality, a different config object would be exported at JS module execution time if isFeatureEnabled checks indicated that drag-and-drop functionality was enabled. This PR moves the isFeatureEnabled check to inside a wrapper component, and uses a single config to render that wrapper component regardless of feature flag status. While there was some overlap between config objects, the DnD and non-DnD versions of some control components required different props, so some refactoring was required to make it possible to merge the two config objects' props to form one unified config object.

TESTING INSTRUCTIONS

Confirm that every chart type's control panel works as expected when the ENABLE_EXPLORE_DRAG_AND_DROP feature flag is enabled and disabled. When ENABLE_EXPLORE_DRAG_AND_DROP is enabled, also ensure that everything works when ENABLE_DND_WITH_CLICK_UX is enabled and disabled.

ADDITIONAL INFORMATION

Has associated issue:
Required feature flags: ENABLE_EXPLORE_DRAG_AND_DROP, ENABLE_DND_WITH_CLICK_UX
Changes UI
Includes DB Migration (follow approval process in SIP-59)
- Migration is atomic, supports rollback & is backwards-compatible
- Confirm DB migration upgrade and downgrade tested
- Runtime estimates and downtime expectations provided
Introduces new feature or API
Removes existing feature or API

codecov · 2022-09-02T23:02:45Z

Codecov Report

Merging #21315 (3d47b7d) into master (cf7c420) will increase coverage by 0.06%.
The diff coverage is 61.01%.

@@            Coverage Diff             @@
##           master   #21315      +/-   ##
==========================================
+ Coverage   66.56%   66.62%   +0.06%     
==========================================
  Files        1791     1791              
  Lines       68591    68520      -71     
  Branches     7319     7287      -32     
==========================================
- Hits        45656    45651       -5     
+ Misses      21046    21002      -44     
+ Partials     1889     1867      -22

Flag	Coverage Δ
javascript	`52.83% <61.01%> (+0.09%)`	⬆️

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

Impacted Files	Coverage Δ
...d/packages/superset-ui-chart-controls/src/index.ts	`100.00% <ø> (ø)`
...et-ui-chart-controls/src/shared-controls/index.tsx	`60.78% <ø> (+12.34%)`	⬆️
...d/packages/superset-ui-chart-controls/src/types.ts	`100.00% <ø> (ø)`
...i-chart-controls/src/utils/expandControlConfig.tsx	`100.00% <ø> (ø)`
...s/legacy-plugin-chart-heatmap/src/controlPanel.tsx	`57.14% <ø> (ø)`
...ns/legacy-plugin-chart-map-box/src/controlPanel.ts	`30.00% <ø> (ø)`
...t-chart-deckgl/src/utilities/sharedDndControls.jsx	`100.00% <ø> (ø)`
...s/legacy-preset-chart-nvd3/src/Bar/controlPanel.ts	`14.28% <ø> (ø)`
...charts/src/Timeseries/Regular/Bar/controlPanel.tsx	`31.25% <ø> (ø)`
...n-chart-handlebars/src/plugin/controls/columns.tsx	`15.00% <0.00%> (ø)`
... and 14 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

kgabryje · 2022-09-07T14:34:22Z

/testenv up

kgabryje · 2022-09-07T14:35:43Z

Wow that's a tremendous piece of work @codyml!
@jinghua-qa Could you please help testing this? The PR basically affects all controls in Explore - we need to make sure that drag and dropping works as it did before

github-actions · 2022-09-07T16:49:53Z

@kgabryje Ephemeral environment spinning up at http://54.202.40.35:8080. Credentials are admin/admin. Please allow several minutes for bootstrapping and startup.

kgabryje · 2022-09-09T09:52:41Z

superset-frontend/src/explore/components/controls/DndColumnSelectControl/DndMetricSelect.tsx

@@ -125,7 +126,7 @@ const getMetricsMatchingCurrentDataset = (
  }, []);
 };

-export const DndMetricSelect = (props: any) => {
+const DndMetricSelect = (props: any) => {


We definitely should improve typing here... (not in this PR, that's a general note)

kgabryje · 2022-09-09T09:53:56Z

superset-frontend/packages/superset-ui-chart-controls/src/shared-controls/dndControls.tsx

+  default?: unknown;
+};
+
+export const dndGroupByControl: SharedControlConfig<


Could you add a comment that will explain the idea behind using props from non-dnd component in dnd control definition? Might be useful for other contributors in the future

kgabryje · 2022-09-09T09:56:28Z

Looks fantastic! Though I can't wait to revert all this when we fully get rid of non-dnd interface 😄
I've tested viz types with non-standard controls (e.g. Table chart, deckgl charts) in dnd and non-dnd mode and it all seems to work well.
I'm going to hold off with approving though until we get a green light from QA

jinghua-qa · 2022-09-12T17:44:12Z

I found an issue that for world map, after save for changes, the chart viz will be empty, i am not able to reproduce in master branch, it may be this PR releated.

Screen.Recording.2022-09-12.at.10.22.05.AM.mov

andrey-zayats · 2022-09-12T18:41:21Z

I found an issue that for world map, after save for changes, the chart viz will be empty, i am not able to reproduce in master branch, it may be this PR releated.

Screen.Recording.2022-09-12.at.10.22.05.AM.mov

I can reproduce this issue with different types of charts, but only with a certain dataset "wb_health_population"

andrey-zayats · 2022-09-12T19:35:14Z

QA verified. PR LGTM

jinghua-qa · 2022-09-13T15:06:23Z

/testenv up

github-actions · 2022-09-13T15:11:50Z

@jinghua-qa Ephemeral environment spinning up at http://54.203.61.228:8080. Credentials are admin/admin. Please allow several minutes for bootstrapping and startup.

jinghua-qa · 2022-09-14T18:35:53Z

This issue is able to reproduce in other superset ephemeral, but not in superset master. It is not related to this PR. LGTM!

I found an issue that for world map, after save for changes, the chart viz will be empty, i am not able to reproduce in master branch, it may be this PR releated.

Screen.Recording.2022-09-12.at.10.22.05.AM.mov

github-actions · 2022-09-14T18:49:46Z

Ephemeral environment shutdown and build artifacts deleted.

superset-frontend/packages/superset-ui-chart-controls/src/shared-controls/dndControls.tsx

…side React render (apache#21315) (cherry picked from commit 2285ebe)

justinpark · 2022-11-02T22:08:47Z

superset-frontend/packages/superset-ui-chart-controls/src/types.ts

@@ -237,7 +237,7 @@ export interface BaseControlConfig<
  ) => boolean;
  mapStateToProps?: (
    state: ControlPanelState,
-    controlState: ControlState,
+    controlState?: ControlState,


@codyml why do you make this parameter optional?
This breaks all our custom override type checking.

Hmm, I think that was a quick fix to stop the histogram viz plugin from complaining after I updated it to use shared controls. I opened a PR that reverts that change and instead updates the plugin – does that look like a better solution? Would be great to spin up an ephemeral env to check if there are any side-effects for Histogram charts.

(Assuming it passes CI)

#22014 was merged, let me know if that solves the issue!

pull-request-size bot added the size/XL label Sep 2, 2022

codyml force-pushed the fix/dnd-ff branch 5 times, most recently from af98814 to a0612e5 Compare September 7, 2022 04:18

rusackas requested review from kgabryje, villebro and geido September 7, 2022 14:19

kgabryje added the need:qa-review Requires QA review label Sep 7, 2022

kgabryje reviewed Sep 9, 2022

View reviewed changes

codyml added 3 commits September 12, 2022 17:52

WIP on DnD fix.

5964987

Fix Table chart, trigger CI, trigger CI.

ad0b10a

Add explanatory comment.

3d47b7d

codyml force-pushed the fix/dnd-ff branch from a0612e5 to 3d47b7d Compare September 12, 2022 22:00

codyml changed the title ~~[WIP] Prevent shared controls from checking feature flags outside React render~~ Prevent shared controls from checking feature flags outside React render Sep 12, 2022

codyml changed the title ~~Prevent shared controls from checking feature flags outside React render~~ fix(explore): Prevent shared controls from checking feature flags outside React render Sep 12, 2022

jinghua-qa approved these changes Sep 14, 2022

View reviewed changes

kgabryje merged commit 2285ebe into apache:master Sep 14, 2022

zhaoyongjie reviewed Sep 15, 2022

View reviewed changes

superset-frontend/packages/superset-ui-chart-controls/src/shared-controls/dndControls.tsx Show resolved Hide resolved

codyml mentioned this pull request Sep 15, 2022

fix(explore): Fix generic X-axis time grain disappearing #21484

Merged

9 tasks

jinghua-qa pushed a commit to preset-io/superset that referenced this pull request Sep 19, 2022

fix(explore): Prevent shared controls from checking feature flags out…

56e15ce

…side React render (apache#21315) (cherry picked from commit 2285ebe)

jinghua-qa added the preset:2022.37 label Sep 21, 2022

codyml mentioned this pull request Sep 21, 2022

fix(explore): Fix datasource switch for table chart #21544

Merged

9 tasks

jinghua-qa removed the preset:2022.37 label Sep 23, 2022

codyml mentioned this pull request Oct 8, 2022

chore(explore): Add tests for getControlValuesCompatibleWithDatasource #21744

Merged

9 tasks

justinpark reviewed Nov 2, 2022

View reviewed changes

codyml mentioned this pull request Nov 3, 2022

fix: Revert shared controls typing change. #22014

Merged

9 tasks

mistercrunch added the 🚢 2.1.3 label Feb 18, 2024

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(explore): Prevent shared controls from checking feature flags outside React render #21315

fix(explore): Prevent shared controls from checking feature flags outside React render #21315

codyml commented Sep 2, 2022 •

edited

Loading

codecov bot commented Sep 2, 2022 •

edited

Loading

kgabryje commented Sep 7, 2022

kgabryje commented Sep 7, 2022

github-actions bot commented Sep 7, 2022

kgabryje Sep 9, 2022

kgabryje Sep 9, 2022

kgabryje commented Sep 9, 2022

jinghua-qa commented Sep 12, 2022

andrey-zayats commented Sep 12, 2022

andrey-zayats commented Sep 12, 2022

jinghua-qa commented Sep 13, 2022

github-actions bot commented Sep 13, 2022

jinghua-qa commented Sep 14, 2022 •

edited

Loading

github-actions bot commented Sep 14, 2022

justinpark Nov 2, 2022

codyml Nov 3, 2022

codyml Nov 3, 2022

codyml Nov 3, 2022

fix(explore): Prevent shared controls from checking feature flags outside React render #21315

fix(explore): Prevent shared controls from checking feature flags outside React render #21315

Conversation

codyml commented Sep 2, 2022 • edited Loading

SUMMARY

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

codecov bot commented Sep 2, 2022 • edited Loading

Codecov Report

kgabryje commented Sep 7, 2022

kgabryje commented Sep 7, 2022

github-actions bot commented Sep 7, 2022

kgabryje Sep 9, 2022

Choose a reason for hiding this comment

kgabryje Sep 9, 2022

Choose a reason for hiding this comment

kgabryje commented Sep 9, 2022

jinghua-qa commented Sep 12, 2022

andrey-zayats commented Sep 12, 2022

andrey-zayats commented Sep 12, 2022

jinghua-qa commented Sep 13, 2022

github-actions bot commented Sep 13, 2022

jinghua-qa commented Sep 14, 2022 • edited Loading

github-actions bot commented Sep 14, 2022

justinpark Nov 2, 2022

Choose a reason for hiding this comment

codyml Nov 3, 2022

Choose a reason for hiding this comment

codyml Nov 3, 2022

Choose a reason for hiding this comment

codyml Nov 3, 2022

Choose a reason for hiding this comment

codyml commented Sep 2, 2022 •

edited

Loading

codecov bot commented Sep 2, 2022 •

edited

Loading

jinghua-qa commented Sep 14, 2022 •

edited

Loading