Add data structures for chart plugin system #6028

kristw · 2018-10-03T23:55:37Z

Classes

ChartMetadata captures the definition of each visualization: name, description, thumbnail image.
Plugin a generic plugin class. All plugins should derive from this class. A plugin can be .configure() to set/override configuration.
ChartPlugin this is how each type of visualization is defined.
Preset holds one or more presets and or plugins.

Registry is a key-value stores that facilitate plugin installation.

Each item can be a value or loader.
Developer register the value or loader function using .registerValue(key, value) and .registerLoader(key, loader), respectively.
registry.get(key) will returns the value or execute the loader function and returns the result.
registry.getAsPromise(key) wraps the .get() result into a Promise, which is useful for handling lazy-loading imports and treat both sync and async items the same way.

ChartMetadataRegistry, ChartComponentRegistry and ChartTransformPropsRegistry are extended from the registry base classes above to hold metadata, the visualization component and transformProps (function that transform formData into compatible parameters for each chart), respectively.

Utility functions

makeSingleton : create a function to get a singleton of the given class (aka. getInstance)
isRequired : A function that throw error. Can be used for requiring certain function parameter by setting isRequired('fieldName') as default value.

After this PR is in, each visualization can be defined as a plugin using the data structures above.

Unit tests

isRequired(field)
  ✓ should throw error with the given field in the message

makeSingleton()
  makeSingleton(BaseClass)
    ✓ returns a function for getting singleton instance of a given base class
    ✓ returned function returns same instance across all calls
  makeSingleton(BaseClass, ...args)
    ✓ returns a function for getting singleton instance of a given base class constructed with the given arguments
    ✓ returned function returns same instance across all calls

Registry
  ✓ exists
  new Registry(name)
    ✓ can create a new registry when name is not given
    ✓ can create a new registry when name is given
  .has(key)
    ✓ returns true if an item with the given key exists
    ✓ returns false if an item with the given key does not exist
  .registerValue(key, value)
    ✓ registers the given value with the given key
    ✓ returns the registry itself
  .registerLoader(key, loader)
    ✓ registers the given loader with the given key
    ✓ returns the registry itself
  .get(key)
    ✓ given the key, returns the value if the item is a value
    ✓ given the key, returns the result of the loader function if the item is a loader
    ✓ returns null if the item with specified key does not exist
    ✓ If the key was registered multiple times, returns the most recent item.
  .getAsPromise(key)
    ✓ given the key, returns a promise of item value if the item is a value
    ✓ given the key, returns a promise of result of the loader function if the item is a loader
    ✓ returns a rejected promise if the item with specified key does not exist
    ✓ If the key was registered multiple times, returns a promise of the most recent item.
  .keys()
    ✓ returns an array of keys
  .entries()
    ✓ returns an array of { key, value }
  .entriesAsPromise()
    ✓ returns a Promise of an array { key, value }
  .remove(key)
    ✓ removes the item with given key
    ✓ does not throw error if the key does not exist
    ✓ returns itself
Plugin
  ✓ exists
  new Plugin()
    ✓ creates a new plugin
  .configure(config, replace)
    ✓ extends the default config with given config when replace is not set or false
    ✓ replaces the default config with given config when replace is true
    ✓ returns the plugin itself
  .resetConfig()
    ✓ resets config back to default
    ✓ returns the plugin itself

ChartPlugin
  ✓ exists
  new ChartPlugin()
    ✓ creates a new plugin
    ✓ throws an error if metadata is not specified
    ✓ throws an error if none of Chart or loadChart is specified
  .register(key)
    ✓ throws an error if key is not provided
    ✓ returns itself

Preset
  ✓ exists
  new Preset()
    ✓ creates new preset
  .register()
    ✓ register all listed presets then plugins
    ✓ returns itself

@williaster @conglei @graceguo-supercat @michellethomas @mistercrunch @xtinec

superset/assets/src/utils/makeSingleton.js

superset/assets/src/visualizations/core/models/ChartPlugin.js

conglei · 2018-10-05T15:29:10Z

superset/assets/src/visualizations/core/models/Preset.js

+      if (plugin instanceof Plugin) {
+        plugin.install();
+      } else {
+        plugin();


Could you explain a bit when plugin will be just a function ?

This I could get some feedback. The issue was when someone needs to call plugin.install(...) with arguments. I couldn't figure out a nice API design to let people pass arguments, so I chose this one.

for example, core chart presets could contains

plugins: [ () => new BarChartPlugin().install('bar'), () => new PieChartPlugin().install('pie'), ]

could you address this by having the Plugin take the config arguments via the constructor instead?

Aha. good idea.

Umm, actually can't, because each vis will declare the ChartPlugin instance and export, so the preset only have access to the instance.

barPlugin.js

const plugin = new ChartPlugin(...); export default plugin;

preset.js

import barPlugin from './barPlugin'; ... plugins: [barPlugin] ...

Umm, on second thought, I can add .configure to the Plugin class

plugins: [ plugin1, plugin2.configure({ key: 'abc' }), ]

hmm, could the pattern instead be to export a class that has extended ChartPlugin?

// BarPlugin.js class BarPlugin extends ChartPlugin { ... } export default BarPlugin; // preset.js import BarPlugin from './BarPlugin'; ... const plugins = [ new BarPlugin({ config: 123 }), ]; ...

That could work too. At first I was like hmm is creating new class an overkill. Perhaps not. Just a bit more typing. and that allows override .register() function + more customization.

class BarPlugin extends ChartPlugin { constructor(config) { super({ config, metadata, Chart, transformProps }); } }

vs

const barPlugin = new ChartPlugin({ metadata, Chart, transformProps }); barPlugin.configure(config);

conglei

LGTM! and thanks for adding all the test!!

williaster · 2018-10-05T17:39:56Z

superset/assets/src/modules/LoaderRegistry.js

@@ -0,0 +1,16 @@
+import Registry from './Registry';
+
+export default class LoaderRegistry extends Registry {


would it simplify things to get rid of this class and just have the parent Registry class have first class support for loaders out of the box?

especially if the Registry class already supports promises

I think simple registry could be useful for sync items like metadata. We can use this for other key-value stores we may have in the future.

Oh I meant have Registry support sync and async all in one class. Like take either a loader or a value, and have the same promise-based API for getting either of them so you don't have to juggle different classes and different methods like get and getAsPromise.

One distinction of the LoaderRegistry from Registry is it overrides .register() to wrap returned value in a function.

Full disclosure. I started out with Registry alone and at some point I encounter value that is a function (e.g. a class) and it gets quite confusing whether the value can be returned immediately or it is a loader that needs to be executed. If both values and loaders can be stored in the same registry, I need another mechanism to indicate which is which. Separating into two classes help simplify that it is either just values or just loaders.

I am revisiting the idea of having both in same Registry again. May have to add explicit flag to keep track if it is a loader or not. You might have convinced me that one class that can do both will be better.

superset/assets/src/visualizations/core/models/ChartPlugin.js

williaster · 2018-10-05T17:54:51Z

superset/assets/src/visualizations/core/models/ChartPlugin.js

+    // use loadTransformProps for dynamic import (lazy-loading)
+    loadTransformProps,
+
+    // use Chart for immediate value


can you explain a little more when you would want to use Chart over loadChart, or transformProps over loadTransformProps? especially since under the hood it seems that they are all transformed to loader funcs anyway?

If the chart is very small. Use Chart, this will include Chart in the bundle instead of spinning it of into a lazy-loaded bundle.

import MyChart from './MyChart'; ... Chart: MyChart, ...

if the chart is gigantic. Use loadChart.

loadChart: () => import('./MyChart.jsx')

superset/assets/src/visualizations/core/registries/ChartComponentLoaderRegistry.js

codecov-io · 2018-10-08T23:26:28Z

Codecov Report

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

@@            Coverage Diff             @@
##           master    #6028      +/-   ##
==========================================
+ Coverage   77.74%   77.81%   +0.06%     
==========================================
  Files          46       46              
  Lines        9412     9446      +34     
==========================================
+ Hits         7317     7350      +33     
- Misses       2095     2096       +1

Impacted Files	Coverage Δ
superset/views/core.py	`73.82% <0%> (-0.1%)`	⬇️
superset/models/core.py	`85.1% <0%> (+0.09%)`	⬆️
superset/cache_util.py	`54.28% <0%> (+0.11%)`	⬆️
superset/connectors/sqla/models.py	`81.05% <0%> (+0.59%)`	⬆️
superset/db_engine_specs.py	`55.87% <0%> (+1.11%)`	⬆️

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 1ee08fc...18eae1b. Read the comment docs.

williaster

this is doooooooope, really like how it turned out! I had one comment about the behavior when registering duplicate values but otherwise LGTM 🙌

williaster · 2018-10-09T01:19:57Z

superset/assets/src/visualizations/core/models/Plugin.js

+  }
+
+  resetConfig() {
+    // The child class can set default config


williaster · 2018-10-09T01:20:37Z

superset/assets/src/visualizations/core/models/Plugin.js

+    return this;
+  }
+
+  configure(config, replace = false) {


I dig the replace 👍

williaster · 2018-10-09T01:24:47Z

superset/assets/src/modules/Registry.js

+
+  registerValue(key, value) {
+    this.items[key] = { value };
+    delete this.promises[key];


I think my only comment at this point is whether we should throw an error if a value already exists, that would require people to consciously call .remove() and possibly raise this to their attn if they weren't aware of it?

or we could just log a warning 🤔 wdyt?

This may happen a lot though for the presets. For example, registering a preset that contains all common charts then override bar chart with a new plugin. Some presets may reference another preset and override certain charts.

I think throwing an error would be too harsh.
Can add warning but then that will mean for every plugin we have to add .remove() call before .register() to avoid the warning. If every plugin does that, would a warning still be useful?

Oh that's a fair point, sgtm 👍

* add unit tests * add test structure * add unit tests for Registry * add LoaderRegistry unit test * add unit test for makeSingleton * add type check * add plugin data structures * simplify API * add preset tests * update test message * fix lint * update makeSingleton * update Plugin, Preset and unit test * revise Registry code * update unit test, add remove function * update test * update unit test * update plugin unit test * add .keys(), .entries() and .entriesAsPromise() * update test description

kristw force-pushed the kristw-loader branch from e794ff7 to a1ef37b Compare October 4, 2018 20:54

conglei reviewed Oct 5, 2018

View reviewed changes

superset/assets/src/utils/makeSingleton.js Outdated Show resolved Hide resolved

conglei reviewed Oct 5, 2018

View reviewed changes

superset/assets/src/visualizations/core/models/ChartPlugin.js Show resolved Hide resolved

conglei reviewed Oct 5, 2018

View reviewed changes

conglei approved these changes Oct 5, 2018

View reviewed changes

williaster reviewed Oct 5, 2018

View reviewed changes

superset/assets/src/visualizations/core/models/ChartPlugin.js Outdated Show resolved Hide resolved

williaster reviewed Oct 5, 2018

View reviewed changes

superset/assets/src/visualizations/core/registries/ChartComponentLoaderRegistry.js Outdated Show resolved Hide resolved

kristw added 18 commits October 8, 2018 14:43

add unit tests

de4e716

add test structure

ed7a53a

add unit tests for Registry

608e160

add LoaderRegistry unit test

1f496d4

add unit test for makeSingleton

92a22ff

add type check

5d1a757

add plugin data structures

288513d

simplify API

dfae36c

add preset tests

585f3f2

update test message

36e1017

fix lint

932bbf4

update makeSingleton

047e2d7

update Plugin, Preset and unit test

1043d4e

revise Registry code

428146f

update unit test, add remove function

de72995

update test

58ad704

update unit test

0904e4a

update plugin unit test

0deeecb

kristw force-pushed the kristw-loader branch from a1ef37b to 0deeecb Compare October 8, 2018 22:14

kristw closed this Oct 8, 2018

kristw reopened this Oct 8, 2018

williaster approved these changes Oct 9, 2018

View reviewed changes

kristw added 2 commits October 8, 2018 22:27

add .keys(), .entries() and .entriesAsPromise()

3f8fff6

update test description

18eae1b

williaster merged commit cd2c46a into apache:master Oct 9, 2018

kristw deleted the kristw-loader branch October 9, 2018 19:14

laxmanraparthi mentioned this pull request Oct 28, 2023

[Snyk] Security upgrade axios from 0.25.0 to 1.6.0 ShareChat/superset#47

Open

laxmanraparthi mentioned this pull request Dec 27, 2023

[Snyk] Security upgrade axios from 0.25.0 to 1.6.3 ShareChat/superset#102

Open

mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 0.28.0 labels Feb 27, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add data structures for chart plugin system #6028

Add data structures for chart plugin system #6028

kristw commented Oct 3, 2018 •

edited

conglei Oct 5, 2018

kristw Oct 5, 2018

williaster Oct 5, 2018 •

edited

kristw Oct 5, 2018

kristw Oct 5, 2018 •

edited

kristw Oct 5, 2018 •

edited

williaster Oct 5, 2018

kristw Oct 5, 2018

conglei left a comment

williaster Oct 5, 2018

williaster Oct 5, 2018

kristw Oct 5, 2018

williaster Oct 5, 2018

kristw Oct 5, 2018 •

edited

kristw Oct 5, 2018

williaster Oct 5, 2018 •

edited

kristw Oct 5, 2018 •

edited

codecov-io commented Oct 8, 2018 •

edited

williaster left a comment

williaster Oct 9, 2018

williaster Oct 9, 2018

williaster Oct 9, 2018

kristw Oct 9, 2018

williaster Oct 9, 2018

		@@ -0,0 +1,16 @@
		import Registry from './Registry';

		export default class LoaderRegistry extends Registry {

Add data structures for chart plugin system #6028

Add data structures for chart plugin system #6028

Conversation

kristw commented Oct 3, 2018 • edited

Classes

Utility functions

Unit tests

Choose a reason for hiding this comment

Choose a reason for hiding this comment

williaster Oct 5, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kristw Oct 5, 2018 • edited

Choose a reason for hiding this comment

kristw Oct 5, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

conglei left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kristw Oct 5, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

williaster Oct 5, 2018 • edited

Choose a reason for hiding this comment

kristw Oct 5, 2018 • edited

Choose a reason for hiding this comment

codecov-io commented Oct 8, 2018 • edited

Codecov Report

williaster left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kristw commented Oct 3, 2018 •

edited

williaster Oct 5, 2018 •

edited

kristw Oct 5, 2018 •

edited

kristw Oct 5, 2018 •

edited

kristw Oct 5, 2018 •

edited

williaster Oct 5, 2018 •

edited

kristw Oct 5, 2018 •

edited

codecov-io commented Oct 8, 2018 •

edited