feat: Consolidate and normalize plugin types #1456
Conversation
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #1456 +/- ##
==========================================
+ Coverage 45.74% 45.79% +0.04%
==========================================
Files 516 518 +2
Lines 35095 35132 +37
Branches 8785 8803 +18
==========================================
+ Hits 16055 16087 +32
- Misses 18989 18994 +5
Partials 51 51
Flags with carried forward coverage won't be shown. Click here to find out more.
☔ View full report in Codecov by Sentry. 📢 Have feedback on the report? Share it here. |
mattrunyon
force-pushed
the
plugin-config
branch
from
0bea64c to
967f0b7
Compare
There was a problem hiding this comment.
Will need to update template: deephaven/deephaven-js-plugin-template#1
mattrunyon
force-pushed
the
plugin-config
branch
from
967f0b7 to
6e78212
Compare
|
I'd like to pull out DashboardPlugin too, but the dependency there on PanelManager is tricky w/ redux and that change feels like it would warrant its own PR |
packages/plugin/package.json
Outdated
| "devDependencies": {}, | ||
| "dependencies": {}, | ||
| "peerDependencies": {}, |
There was a problem hiding this comment.
Looks like you're missing some dependencies here? This package definitely references @deephaven/iris-grid, @deephaven/components, @deephaven/jsapi-types from TablePlugin.
That's one thing kind of annoying about this - creating an AuthPlugin, you don't care about @deephaven/iris-grid at all, but you need to import this package which will have a dependency on that.
There was a problem hiding this comment.
Ya they're technically dev dependencies since they aren't needed to run, but the types are required for someone else to install the package and get type support. So that would make them regular dependencies
I can't think of a better way that doesn't require you to install iris-grid or components. Unless we want to duplicate the types across both or pull them into a separate, shared types package which doesn't really make sense for these types IMO.
If we want to consolidate the definitions, I think it's just something we have to accept as a trade-off. We could make packages for each plugin type, but I'm not really a big fan of adding more packages here just to save a few MB of disk space for a development environment
Fixes #1454. Needs #1451
Still some cleanup that could be done around
DashboardPlugin. I think instead of static metadata on the component (displayNameis ok since it's part of React) those items should probably be part of the config object.