feat(plugin): add plugin-chart-cartodiagram #25869
Conversation
jansule
requested review from
michael-s-molina,
geido and
kgabryje
as code owners
|
I'm not sure why the linting in the |
|
@jansule Thanks for contributing a new chart plugin. This PR is quite large and includes TypeScript changes. Could you please detach all changes not related to the newly introduced chart plugin? |
superset-frontend/package.json
Outdated
| "geostyler": "^12.0.2", | ||
| "geostyler-data": "^1.0.0", | ||
| "geostyler-openlayers-parser": "^4.2.1", | ||
| "geostyler-style": "^7.4.0", | ||
| "geostyler-wfs-parser": "^2.0.0", |
There was a problem hiding this comment.
Please add these libraries to your plugin directory, not to the root directory.
There was a problem hiding this comment.
I added these to the root directory as they are needed by the <LayerConfigsControl /> which was added to superset-frontend/src/explore and not to the plugin itself.
The only reason the control is not included in the plugin directory is, that the <ControlFormItem /> was moved from @superset-ui/core to superset-frontend/src/explore, so I cannot import it in the plugin directory. This component, however, is needed for a consistent UI of the controls.
| @@ -138,12 +139,18 @@ | |||
| "d3-scale": "^2.1.2", | |||
| "dom-to-image-more": "^2.10.1", | |||
| "dom-to-pdf": "^0.3.2", | |||
| "echarts": "^5.4.1", | |||
There was a problem hiding this comment.
i.e. echarts is included in the plugin-chart-echarts package, and all components that depend on ECharts should also be included in that package.
| "echarts": "^5.4.1", |
superset-frontend/babel.config.js
Outdated
| // After adding transformIgnorePatterns to jest.config.js | ||
| // the tests failed. Following line fixed the problem. See also | ||
| // https://github.com/babel/babel/issues/8731#issuecomment-423845498 | ||
| // core-js seems to be packaged as commonjs modules, so it should | ||
| // be (more or less) save to ignore them with babel. | ||
| ignore: [/^core-js$/], |
There was a problem hiding this comment.
These are likely side effects of changes to package.json. Please clean up package.json as suggested, and these changes should no longer be necessary.
| // After adding transformIgnorePatterns to jest.config.js | |
| // the tests failed. Following line fixed the problem. See also | |
| // https://github.com/babel/babel/issues/8731#issuecomment-423845498 | |
| // core-js seems to be packaged as commonjs modules, so it should | |
| // be (more or less) save to ignore them with babel. | |
| ignore: [/^core-js$/], |
Thanks for the first review @justinpark. I created a PR with only the TypeScript changes a while ago. See #24272. I guess it just got lost in the amount of PRs you are receiving. |
jansule
force-pushed
the
cartodiagram-plugin-2023-11-03
branch
from
b3c31dc to
dc16320
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #25869 +/- ##
==========================================
+ Coverage 67.17% 69.33% +2.16%
==========================================
Files 1899 1895 -4
Lines 74354 74279 -75
Branches 8266 8248 -18
==========================================
+ Hits 49945 51501 +1556
+ Misses 22360 20735 -1625
+ Partials 2049 2043 -6
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
github-actions
bot
added
doc
jansule
force-pushed
the
cartodiagram-plugin-2023-11-03
branch
from
5e554b6 to
b317929
Compare
jansule
force-pushed
the
cartodiagram-plugin-2023-11-03
branch
from
a544528 to
6a774e4
Compare
jansule
force-pushed
the
cartodiagram-plugin-2023-11-03
branch
5 times, most recently
from
083b8d7 to
8b59fc8
Compare
|
@villebro I finally managed to update the PR so that the pipelines succeed. The failing Dependency Review pipeline fails due to a missing license entry in one of our transitive dependencies (BSD-3 Clause). I added a PR there a while ago that fixes the problem. So could you take another look at this PR? |
jansule
force-pushed
the
cartodiagram-plugin-2023-11-03
branch
from
f44ae62 to
1b03c29
Compare
|
Any update on this PR? This would be a very cool feature for Superset. |
|
I would also like to have this PR merged (mapbox is a joke). All my support to @jansule for the patience with updating and conflict solving, etc. I've tried to test it locally, although not being eligible as a possible reviewer. It seems to work. I am not able to load any WMS as the background but this could possibly be due to some of the proxy/reverseproxy black magic here. |
Not a proxy magic. Made it work locally with the default terreatris WMS but not being able to change the WMS to anything else. See the example in the screenshot (no WMS visible, no error message in logs; the service URL is https://eagri.cz/public/app/wms/public_zp.fcgi):
|
|
Looks like |
|
Thanks for the feedback! I will try to get some time to resolve the merge conflicts soon.
@rusackas There is an invalid spdx identifier in
@lcalisto So far I had no problems with loading other WMS. You might want to ensure that your Talisman settings are correct. I will double check as soon as I get time to work on this PR again. |
Solved it today (I guess it was on me and not on @lcalisto ). It was indeed a problem with Talisman. I didn't make it work with the Talisman so simply turning it off helped but this could very probably be connected to some security issues at the server or some obscure thing about the WMS in use. Therefore: Big thumbs up. Tested and working for me. Thanks very much and I hope it will get merged one day. |
Ahh, ok, if it's all good, you can get the Dependency Review CI check to pass by adding the package to |
|
I just saw that @jansule PR bramstein/css-font-parser#11 has been merged. Perhaps that also helps. 🙂 |
That's great. I justed pinged the owner to create a new release. Hopefully this will happen soon. Otherwise I will add the dependeny to the
Happy to hear that it's working for you now! |
jansule
force-pushed
the
cartodiagram-plugin-2023-11-03
branch
from
1b03c29 to
51b6e82
Compare
There was a problem hiding this comment.
Thanks for keeping this PR alive @jansule ❤️ If you can rebase the PR and work with us for a few rounds to iron out the last kinds I think we should be able to get this merged fairly before the new year. FYI: @michael-s-molina @rusackas this is a good example of a plugin that would benefit from the improved FE plugin architecture we've been discussing.
| export const isTimeseriesDataRecord = ( | ||
| item: any, | ||
| ): item is TimeseriesDataRecord => Object.keys(item).includes('__timestamp'); | ||
|
|
||
| export const isTimeseriesDataRecordList = ( | ||
| items: any[], | ||
| ): items is TimeseriesDataRecord[] => items.every(isTimeseriesDataRecord); | ||
|
|
There was a problem hiding this comment.
Are these still needed? I checked out the codebase, but I wasn't able to find any references to these functions. Maybe they were needed before? The reason I ask is because we're slowly deprecating the concept of dedicated Timeseries charts (most charts should just support any type of base axis).
| @@ -31,6 +31,14 @@ export interface TimeseriesDataRecord extends DataRecord { | |||
| __timestamp: number | string | Date | null; | |||
| } | |||
|
|
|||
| export const isTimeseriesDataRecord = ( | |||
| item: any, | |||
| ): item is TimeseriesDataRecord => Object.keys(item).includes('__timestamp'); | |||
There was a problem hiding this comment.
nit: there is DTTM_ALIAS that should be referenced when referring to __timestamp.
| "@types/geojson": "^7946.0.10", | ||
| "geojson": "^0.5.0", | ||
| "lodash": "^4.17.21", | ||
| "ol": "^7.1.0" |
There was a problem hiding this comment.
I see we are already adding ol in the main package.json. Therefore, should this not be in peerDependencies? Or is there some reason we specifically need ^7.1.0 here? It'd be nice if we can reuse the main dependency here, too, as in the future we want to decouple these deps from the core package to the plugins.
| "geostyler": "^12.0.2", | ||
| "geostyler-data": "^1.0.0", | ||
| "geostyler-openlayers-parser": "^4.3.0", | ||
| "geostyler-style": "^7.5.0", | ||
| "geostyler-wfs-parser": "^2.0.3", |
There was a problem hiding this comment.
I agree with @justinpark that these deps (and the ol package below) should ideally exist in the plugin. However, I'm aware that due to limitations in the current plugin architecture, we aren't able to adequately decouple controls from the core codebase. This is something that will be considered when redesigning the plugin architecture.
| const [mapId] = useState( | ||
| `cartodiagram-plugin-${Math.floor(Math.random() * 1000)}`, | ||
| ); |
There was a problem hiding this comment.
nit: if it's all the same, maybe throw a few extra zeros here to minimize the risk of a collision (I can only assume suffixing it with 0-999999 won't cause issues vs 0-999)
| /** | ||
| * The buildQuery function is used to create an instance of QueryContext that's | ||
| * sent to the chart data endpoint. In addition to containing information of which | ||
| * datasource to use, it specifies the type (e.g. full payload, samples, query) and | ||
| * format (e.g. CSV or JSON) of the result and whether or not to force refresh the data from | ||
| * the datasource as opposed to using a cached copy of the data, if available. | ||
| * | ||
| * More importantly though, QueryContext contains a property `queries`, which is an array of | ||
| * QueryObjects specifying individual data requests to be made. A QueryObject specifies which | ||
| * columns, metrics and filters, among others, to use during the query. Usually it will be enough | ||
| * to specify just one query based on the baseQueryObject, but for some more advanced use cases | ||
| * it is possible to define post processing operations in the QueryObject, or multiple queries | ||
| * if a viz needs multiple different result sets. | ||
| */ |
There was a problem hiding this comment.
| /** | |
| * The buildQuery function is used to create an instance of QueryContext that's | |
| * sent to the chart data endpoint. In addition to containing information of which | |
| * datasource to use, it specifies the type (e.g. full payload, samples, query) and | |
| * format (e.g. CSV or JSON) of the result and whether or not to force refresh the data from | |
| * the datasource as opposed to using a cached copy of the data, if available. | |
| * | |
| * More importantly though, QueryContext contains a property `queries`, which is an array of | |
| * QueryObjects specifying individual data requests to be made. A QueryObject specifies which | |
| * columns, metrics and filters, among others, to use during the query. Usually it will be enough | |
| * to specify just one query based on the baseQueryObject, but for some more advanced use cases | |
| * it is possible to define post processing operations in the QueryObject, or multiple queries | |
| * if a viz needs multiple different result sets. | |
| */ |
| /** | ||
| * The control panel is split into two tabs: "Data" and | ||
| * "Customize". The controls that define the inputs to | ||
| * the chart data request, such as columns and metrics, usually | ||
| * reside within "Data", while controls that affect the visual | ||
| * appearance or functionality of the chart are under the | ||
| * "Customize" section. | ||
| */ | ||
|
|
||
| // For control input types, see: superset-frontend/src/explore/components/controls/index.js |
There was a problem hiding this comment.
| /** | |
| * The control panel is split into two tabs: "Data" and | |
| * "Customize". The controls that define the inputs to | |
| * the chart data request, such as columns and metrics, usually | |
| * reside within "Data", while controls that affect the visual | |
| * appearance or functionality of the chart are under the | |
| * "Customize" section. | |
| */ | |
| // For control input types, see: superset-frontend/src/explore/components/controls/index.js |
| /** | ||
| * The constructor is used to pass relevant metadata and callbacks that get | ||
| * registered in respective registries that are used throughout the library | ||
| * and application. A more thorough description of each property is given in | ||
| * the respective imported file. | ||
| * | ||
| * It is worth noting that `buildQuery` and is optional, and only needed for | ||
| * advanced visualizations that require either post processing operations | ||
| * (pivoting, rolling aggregations, sorting etc) or submitting multiple queries. | ||
| */ |
There was a problem hiding this comment.
| /** | |
| * The constructor is used to pass relevant metadata and callbacks that get | |
| * registered in respective registries that are used throughout the library | |
| * and application. A more thorough description of each property is given in | |
| * the respective imported file. | |
| * | |
| * It is worth noting that `buildQuery` and is optional, and only needed for | |
| * advanced visualizations that require either post processing operations | |
| * (pivoting, rolling aggregations, sorting etc) or submitting multiple queries. | |
| */ |
| "lodash": "^4.17.21", | ||
| "moment": "^2.30.1" | ||
| }, | ||
| "peerDependencies": { | ||
| "@superset-ui/chart-controls": "*", | ||
| "@superset-ui/core": "*", | ||
| "echarts": "*", |
There was a problem hiding this comment.
I vaguely remember there having been a discussion about why this was needed as a core dependency. Is this really necessary, or can we leave echarts as a dependency only on the ECharts plugin?
| @@ -16,7 +16,6 @@ | |||
| * specific language governing permissions and limitations | |||
| * under the License. | |||
| */ | |||
|
|
|||
There was a problem hiding this comment.
Nit: I assume this is a mistake - let's avoid touching unrelated files.
SUMMARY
This adds a new plugin
plugin-chart-cartodiagramthat allows creating cartodiagrams, i.e. charts on a map. See also #21758.How does it work?
In order to support as many charts as possible, we created some kind of a meta plugin that takes configurations of any existing chart plugin supported by superset and places it on a map. To do so, we make all charts with a common data source selectable, and inject a group-by property based on a provided geometry column. Then, we use the
buildQuery()andtransformProps()functions of each chart in combination with the respective chart component (via theChartComponentRegistry) to render them on a map. Through this, all configuration options of any chart are supported.Additional configuration options for cartodiagrams are:
<div>element of a chart.fixed,linear,exponential).Default layers can be configured in
MainPreset.jswhich will be shown by default for every configured cartodiagram panel. Users are able to edit/remove these layers for a single panel in the explore view. Default layers currently include a WMS showing OpenStreetMap data in grayscale.Disclaimer: The service providing the OpenStreetMap data is a demo service hosted by terrestris GmbH & Co. KG, which is my employer. On a higher zoomlevel, it shows a watermark for our paid service. The demo server is not recommended to be used in production and we recommend replacing this service. It was just added for others to have a default background map while getting familiar with the plugin and probably should be replaced in the future.
Technical Details/Requirements
Additional libraries:
Both libraries are part of the Open Source Geospatial Foundation, so the chances of future development of those libraries are quite certain.
In order to make use of above libraries, we had to update some dependencies (e.g. typescript, see also #24272), and we also had to make use of
npm dedupein order to resolve version conflicts. Unfortunately, this resulted in a nearly complete rewrite of thepackage-lock.json.Due to some version upgrades, we also had to apply some linting fixes, which results in more changed files than actually directly related to this PR. Sorry for that.
The
<SelectControl>had to be changed a little to be able to add a customized list of options. We use it to display all charts with a common dataset and also check, if a previously selected chart was updated after the selection.Limitations
There are some limitations of the plugin, which probably should be addressed in the future:
EPSG:3857(WGS 84 / Pseudo-Mercator)Currently, filters applied to a dashboard are not reflected in the cartodiagram plugin. We would be grateful, if someone can hint us to the right direction for supporting this.Dashboard filters are now supported.Since the background layers point to external services, the CSP have to be adjusted. Additional information can be found in the README of the plugin.
How can this be improved?
This PR is just the first step into the direction for additional geospatial support. So there are many things that can be added and improved. Besides the points mentioned above, following points might also be useful:
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
Example dashboard with some cartodiagrams:
Explore view of cartodiagram plugin (configuration):
Explore view of cartodiagram plugin (customization):
Explore view of cartodiagram plugin (layer configuration):
Explore view of cartodiagram plugin (layer styling):
TESTING INSTRUCTIONS
ADDITIONAL INFORMATION