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

[wip] superset-ui-plugins linking tool for local development #8638

Closed
wants to merge 19 commits into from

Conversation

suddjian
Copy link
Member

@suddjian suddjian commented Nov 23, 2019

CATEGORY

Choose one

  • Bug Fix
  • Enhancement (new features, refinement)
  • Refactor
  • Add tests
  • Build / Development Environment
  • Documentation

SUMMARY

This adds a tool to aid local plugin development/debugging by linking all of the packages in the superset-ui-plugins repo. A new npm script, plugin-devmode-on, npm links all the superset-ui-plugins. A new webpack config detects any npm linked superset-ui-plugins, and automatically changes imports for those plugins to import from /src. This makes a whole series of steps unnecessary. You can set the location to use for the repo with the env variable SUPERSET_UI_PLUGINS_PATH.

Another npm script plugin-devmode-off undoes the changes of plugin-devmode-on by running npm ci.

This is not entirely working, just yet. The BoxPlotChartPlugin in @superset-ui/preset-chart-xy plugin is imported by reaching into the /esm/legacy directory. This breaks, because webpack rewrites it to look for src/esm/legacy, which does not exist. We propose to solve this by either modernizing the plugin (no idea how easy/difficult that is), or splitting it into a legacy version and a non-legacy version, so that we don't need to reach inside a build folder to import the chart. Input would be appreciated from anyone with more insight into the history of that plugin.

@suddjian suddjian changed the title Package linking superset-ui-plugins linking tool for local development Nov 23, 2019
@codecov-io
Copy link

codecov-io commented Nov 25, 2019

Codecov Report

Merging #8638 into master will increase coverage by 0.05%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #8638      +/-   ##
==========================================
+ Coverage   65.85%   65.91%   +0.05%     
==========================================
  Files         482      482              
  Lines       24152    24157       +5     
  Branches     2770     2770              
==========================================
+ Hits        15906    15922      +16     
+ Misses       8068     8057      -11     
  Partials      178      178
Impacted Files Coverage Δ
superset/views/base.py 74.74% <0%> (ø) ⬆️
superset/views/core.py 72.16% <0%> (+0.3%) ⬆️
superset/models/sql_lab.py 91.89% <0%> (+0.9%) ⬆️
superset/extensions.py 96.15% <0%> (+1.28%) ⬆️
superset/views/sql_lab.py 60.52% <0%> (+3.94%) ⬆️

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 8a00168...210d681. Read the comment docs.

@etr2460
Copy link
Member

etr2460 commented Nov 25, 2019

@williaster mind taking a look at this in lieu of @kristw since he's on leave?

Copy link
Member

@mistercrunch mistercrunch left a comment

Choose a reason for hiding this comment

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

README docs as to how to use this should be in scope for this PR


const PLUGINS_REPO =
process.env.SUPERSET_UI_PLUGINS_PATH ||
path.resolve('../../../superset-ui-plugins');
Copy link
Member

Choose a reason for hiding this comment

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

future: eventually we'll have to add support for other repos like https://github.com/apache-superset/superset-ui-plugins-deckgl

@suddjian
Copy link
Member Author

suddjian commented Dec 9, 2019

There's an issue with the approach used here: The code initializing webpack aliases is checking for any linked superset-ui-plugins packages, and modifying the imports to import from /src automatically. This makes it effectively impossible to debug problems that only arise after build time.

This can be fixed by introducing a plugin-devmode.json file, which will store the data about which plugins are in devmode instead of using the status of symlinked packages to get that info. I'm hoping to tackle that sometime this week.

@stale
Copy link

stale bot commented Feb 8, 2020

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. For admin, please label this issue .pinned to prevent stale bot from closing the issue.

@stale stale bot added the inactive Inactive for >= 30 days label Feb 8, 2020
@mistercrunch mistercrunch added the .pinned Draws attention label Feb 10, 2020
@stale stale bot removed the inactive Inactive for >= 30 days label Feb 10, 2020
@suddjian suddjian changed the title superset-ui-plugins linking tool for local development [wip] superset-ui-plugins linking tool for local development Feb 10, 2020
@pull-request-size pull-request-size bot added size/L and removed size/M labels Feb 10, 2020
@ktmud
Copy link
Member

ktmud commented Feb 18, 2020

Was anyone tempted to move superset-ui (especially the visualization registry) and superset-ui-plugins (or at least the basic ones) back to the core frontend code? I'm struggling to see the benefit of putting them in separate repos. They are core to Superset's basic functions, not really pluggable, and unlikely to be used by other projects.

Currently we haven't migrated off the legacy plugins and the added overhead of syncing package versions, maintaining separate building processes only makes the migration/iterations slower.

Both Redash and Metabase didn't take this approach. IMO their visualization registries look much cleaner than Superset and are more developer-friendly to new contributors.

CONTRIBUTING.md Outdated
@@ -805,6 +805,12 @@ Here's an example as a Github PR with comments that describe what the
different sections of the code do:
https://github.com/apache/incubator-superset/pull/3013

To run Superset using visualization plugins from your local machine, run `npm run plugin-devmode-on`. This will use `npm link` to connect the Superset frontend to your local copy of [superset-ui-plugins](https://github.com/apache-superset/superset-ui-plugins). This command uses the environment variable `SUPERSET_UI_PLUGINS_PATH` to determine the path, defaulting to `../superset-ui-plugins`. So, for examplem, if the path to your local `superset-ui-plugins` repo, relative to `incubator-superset`, happens to be `../../my-fork-of-superset-ui-plugins`, then run `SUPERSET_UI_PLUGINS_PATH=../../my-fork-of-superset-ui-plugins npm run plugin-devmode-on`
Copy link
Member

Choose a reason for hiding this comment

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

typo: "examplem"

@mistercrunch
Copy link
Member

@ktmud I agree that it's been really hard to work on plugins for quite some time now. I'm really hoping we can make it much easier to work with plugins quickly.

I've been wondering whether it could make sense to bring back some of the core plugins to the main repo too. The long tail and the community-contributed ones could live in other repos, but having the core in the main repo/package may help. PRing on two repos, publishing packages and bumping versions is tricky, and that's on top of the npm link stuff.

It's been many times that I want to work in a plugin and either really struggle or completely fail at it.

@rusackas
Copy link
Member

Closing in favor of #9326

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
.pinned Draws attention size/L
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants