-
Notifications
You must be signed in to change notification settings - Fork 16.5k
[wip] superset-ui-plugins linking tool for local development #8638
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
Conversation
Codecov Report
@@ 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
Continue to review full report at Codecov.
|
|
@williaster mind taking a look at this in lieu of @kristw since he's on leave? |
mistercrunch
left a comment
There was a problem hiding this 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
superset/assets/plugin-devmode.js
Outdated
There was a problem hiding this comment.
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
|
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 This can be fixed by introducing a |
|
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 |
|
Was anyone tempted to move 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. |
210d681 to
c44d735
Compare
CONTRIBUTING.md
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
typo: "examplem"
f630cc5 to
da45f7b
Compare
|
@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 It's been many times that I want to work in a plugin and either really struggle or completely fail at it. |
|
Closing in favor of #9326 |
CATEGORY
Choose one
SUMMARY
This adds a tool to aid local plugin development/debugging by linking all of the packages in the
superset-ui-pluginsrepo. 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 variableSUPERSET_UI_PLUGINS_PATH.Another npm script
plugin-devmode-offundoes the changes ofplugin-devmode-onby runningnpm ci.This is not entirely working, just yet. The
BoxPlotChartPluginin@superset-ui/preset-chart-xyplugin is imported by reaching into the/esm/legacydirectory. This breaks, because webpack rewrites it to look forsrc/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.