Skip to content
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.

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

feat: Dynamic dashboard component #17208

Merged
merged 43 commits into from
Feb 9, 2022

Conversation

simcha90
Copy link
Contributor

@simcha90 simcha90 commented Oct 24, 2021

SUMMARY

This PR is the implementation of SIP -76 #17210 - feat for dynamic loading of dashboard components

Why?

Dependent on product needs we want to take development of Superset to different directions. Dashboard Components can introduce specific needs of company requirements. So we wanted to do this feature more flexible in order to not affect other product visions :)

What?

This PR add opportunity to add new Dashboard Components that can be configured in separate file by every superset instance and not affects main Superset functionality. Commonly needed types relating to the Dashboard are also moved into @superset-ui/core to make it possible to fully decouple dashboard component plugin development from the Superset codebase.

How?

We creating new dashboard components registry that can register new Dashboard Components in /src/setup/setupDasboardComponents.ts. In a follow-up PR, this code will be moved into a Yeoman generator template.

When we drag new component to dashboard we use React.lazy function to load code of component

NOTES:

  • We created new type of registry using functional approach in favor of class approach that we are using now for loading plugin charts
  • We have some basic predefined rules for dynamic components
  • To test existing functionality you need uncomment code in file superset-frontend/src/setup/setupDasboardComponents.ts and it will add demo component to list of components

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

Screen.Recording.2021-10-24.at.15.41.42.mov

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

  • Has associated issue:
  • Required feature flags:
  • 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
Copy link

codecov bot commented Oct 24, 2021

Codecov Report

Merging #17208 (e5b7040) into master (28e729b) will decrease coverage by 0.04%.
The diff coverage is 44.26%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #17208      +/-   ##
==========================================
- Coverage   66.31%   66.26%   -0.05%     
==========================================
  Files        1595     1603       +8     
  Lines       62599    62715     +116     
  Branches     6297     6316      +19     
==========================================
+ Hits        41513    41559      +46     
- Misses      19440    19507      +67     
- Partials     1646     1649       +3     
Flag Coverage Δ
javascript 51.29% <44.26%> (-0.05%) ⬇️

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

Impacted Files Coverage Δ
...et-frontend/src/dashboard/actions/nativeFilters.ts 42.04% <ø> (ø)
.../src/dashboard/components/BuilderComponentPane.tsx 78.94% <0.00%> (-4.39%) ⬇️
...ilterScopingModal/CrossFilterScopingForm/index.tsx 83.33% <ø> (ø)
...components/DashboardBuilder/DashboardContainer.tsx 67.64% <ø> (ø)
...src/dashboard/components/DashboardBuilder/state.ts 68.96% <ø> (ø)
...nd/src/dashboard/components/FiltersBadge/index.tsx 86.11% <ø> (ø)
...src/dashboard/components/FiltersBadge/selectors.ts 70.64% <ø> (ø)
...d/src/dashboard/components/gridComponents/index.js 100.00% <ø> (ø)
...rBar/CascadeFilters/CascadeFilterControl/index.tsx 100.00% <ø> (ø)
.../FilterBar/CascadeFilters/CascadePopover/index.tsx 48.33% <ø> (ø)
... and 58 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 28e729b...e5b7040. Read the comment docs.

@amitmiran137 amitmiran137 changed the title feat: Dynamic dashboard component feat: Dynamic dashboard components Oct 24, 2021
@simcha90
Copy link
Contributor Author

First pass comments. Code looks great, but my main question is shouldn't we move the central building blocks to @superset-ui/core, like the central types/interfaces and registries? If we'd do that, we could

  1. Develop the plugins independently from the Superset codebase.
  2. Add a dashboard component template to the Yeoman generator. See fix(generator): more cleanup to plugin framework #18027 which was merged just recently and made the system work again after the monorepo migration
  3. Publish dashboard component plugins to npm
  4. Pin plugins to specific versions etc

Etc. Also, I think we should add tests as early as possible, which are missing here. By moving the relevant parts to @superset-ui/core we would automatically have 100 % test coverage requirements, but in addition, we should also have at least a few RTL tests that make sure the relevant Dashboard flows work (viewing a dashboard with a plugin + adding and removing a component)

May be functional registry can be moved to superset-core, but meantime I don't see some special need for it because:

  1. we can do it also now, I think this PR enough for our company needs to start develop our plugins
  2. I'm not sure that we have capacity for it, @amitmiran137 do we have?
    3, 4 Can be done also now, we don't need move code to superset-core for these things

P.S. other stuff fixed

@zhaoyongjie zhaoyongjie self-requested a review January 18, 2022 04:06
Copy link
Member

@zhaoyongjie zhaoyongjie left a comment

Choose a reason for hiding this comment

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

Hi @simcha90, in order to local test, could you rebase this PR from the master branch? Thanks!

I got some lint errors in my local. I think if we rebase master might suppress this issue in IDE.
image

@simcha90
Copy link
Contributor Author

@zhaoyongjie done

Copy link
Member

@villebro villebro left a comment

Choose a reason for hiding this comment

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

The code LGTM and in general I think the proposed solution looks great. However, to further decouple this and make it more approachable for plugin developers, I'd like to propose the following:

  1. add a Yeoman template, similar to the viz plugin one, to superset-ui/generator-superset. This would optimally generate the example component. By doing this we would not need the example component in the actual codebase
  2. Add a section to the docs on how to create a dashboard plugin, something similar to https://superset.apache.org/docs/contributing/creating-viz

My long term vision is to keep making it easier to add plugins, hopefully removing the need for manually registering the plugins in the superset-frontend codebase all together (I'm thinking moving more and more in the direction of dynamic plugins). To spread the pain, I'm happy to tackle the docs, and can even help out moving the example plugin to the yeoman generator if needed.

Copy link
Member

@villebro villebro left a comment

Choose a reason for hiding this comment

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

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels size/XXL 🚢 1.5.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[SIP-76] Proposal for custom dashboard component support
5 participants