You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by GitBox <gi...@apache.org> on 2018/10/09 18:47:18 UTC
[GitHub] williaster closed pull request #6028: Add data structures for chart
plugin system
williaster closed pull request #6028: Add data structures for chart plugin system
URL: https://github.com/apache/incubator-superset/pull/6028
This is a PR merged from a forked repository.
As GitHub hides the original diff on merge, it is displayed below for
the sake of provenance:
As this is a foreign pull request (from a fork), the diff is supplied
below (as it won't show otherwise due to GitHub magic):
diff --git a/superset/assets/spec/javascripts/modules/Registry_spec.js b/superset/assets/spec/javascripts/modules/Registry_spec.js
new file mode 100644
index 0000000000..47cdb21ca9
--- /dev/null
+++ b/superset/assets/spec/javascripts/modules/Registry_spec.js
@@ -0,0 +1,175 @@
+import { describe, it } from 'mocha';
+import { expect } from 'chai';
+import Registry from '../../../src/modules/Registry';
+
+describe('Registry', () => {
+ it('exists', () => {
+ expect(Registry !== undefined).to.equal(true);
+ });
+
+ describe('new Registry(name)', () => {
+ it('can create a new registry when name is not given', () => {
+ const registry = new Registry();
+ expect(registry).to.be.instanceOf(Registry);
+ });
+ it('can create a new registry when name is given', () => {
+ const registry = new Registry('abc');
+ expect(registry).to.be.instanceOf(Registry);
+ expect(registry.name).to.equal('abc');
+ });
+ });
+
+ describe('.has(key)', () => {
+ it('returns true if an item with the given key exists', () => {
+ const registry = new Registry();
+ registry.registerValue('a', 'testValue');
+ expect(registry.has('a')).to.equal(true);
+ registry.registerLoader('b', () => 'testValue2');
+ expect(registry.has('b')).to.equal(true);
+ });
+ it('returns false if an item with the given key does not exist', () => {
+ const registry = new Registry();
+ expect(registry.has('a')).to.equal(false);
+ });
+ });
+
+ describe('.registerValue(key, value)', () => {
+ it('registers the given value with the given key', () => {
+ const registry = new Registry();
+ registry.registerValue('a', 'testValue');
+ expect(registry.has('a')).to.equal(true);
+ expect(registry.get('a')).to.equal('testValue');
+ });
+ it('returns the registry itself', () => {
+ const registry = new Registry();
+ expect(registry.registerValue('a', 'testValue')).to.equal(registry);
+ });
+ });
+
+ describe('.registerLoader(key, loader)', () => {
+ it('registers the given loader with the given key', () => {
+ const registry = new Registry();
+ registry.registerLoader('a', () => 'testValue');
+ expect(registry.has('a')).to.equal(true);
+ expect(registry.get('a')).to.equal('testValue');
+ });
+ it('returns the registry itself', () => {
+ const registry = new Registry();
+ expect(registry.registerLoader('a', () => 'testValue')).to.equal(registry);
+ });
+ });
+
+ describe('.get(key)', () => {
+ it('given the key, returns the value if the item is a value', () => {
+ const registry = new Registry();
+ registry.registerValue('a', 'testValue');
+ expect(registry.get('a')).to.equal('testValue');
+ });
+ it('given the key, returns the result of the loader function if the item is a loader', () => {
+ const registry = new Registry();
+ registry.registerLoader('b', () => 'testValue2');
+ expect(registry.get('b')).to.equal('testValue2');
+ });
+ it('returns null if the item with specified key does not exist', () => {
+ const registry = new Registry();
+ expect(registry.get('a')).to.equal(null);
+ });
+ it('If the key was registered multiple times, returns the most recent item.', () => {
+ const registry = new Registry();
+ registry.registerValue('a', 'testValue');
+ expect(registry.get('a')).to.equal('testValue');
+ registry.registerLoader('a', () => 'newValue');
+ expect(registry.get('a')).to.equal('newValue');
+ });
+ });
+
+ describe('.getAsPromise(key)', () => {
+ it('given the key, returns a promise of item value if the item is a value', () => {
+ const registry = new Registry();
+ registry.registerValue('a', 'testValue');
+ return registry.getAsPromise('a').then((value) => {
+ expect(value).to.equal('testValue');
+ });
+ });
+ it('given the key, returns a promise of result of the loader function if the item is a loader ', () => {
+ const registry = new Registry();
+ registry.registerLoader('a', () => 'testValue');
+ return registry.getAsPromise('a').then((value) => {
+ expect(value).to.equal('testValue');
+ });
+ });
+ it('returns a rejected promise if the item with specified key does not exist', () => {
+ const registry = new Registry();
+ return registry.getAsPromise('a').then(null, (err) => {
+ expect(err).to.equal('Item with key "a" is not registered.');
+ });
+ });
+ it('If the key was registered multiple times, returns a promise of the most recent item.', () => {
+ const registry = new Registry();
+ registry.registerValue('a', 'testValue');
+ const promise1 = registry.getAsPromise('a').then((value) => {
+ expect(value).to.equal('testValue');
+ });
+ registry.registerLoader('a', () => 'newValue');
+ const promise2 = registry.getAsPromise('a').then((value) => {
+ expect(value).to.equal('newValue');
+ });
+ return Promise.all([promise1, promise2]);
+ });
+ });
+
+ describe('.keys()', () => {
+ it('returns an array of keys', () => {
+ const registry = new Registry();
+ registry.registerValue('a', 'testValue');
+ registry.registerLoader('b', () => 'test2');
+ expect(registry.keys()).to.deep.equal(['a', 'b']);
+ });
+ });
+
+ describe('.entries()', () => {
+ it('returns an array of { key, value }', () => {
+ const registry = new Registry();
+ registry.registerValue('a', 'test1');
+ registry.registerLoader('b', () => 'test2');
+ expect(registry.entries()).to.deep.equal([
+ { key: 'a', value: 'test1' },
+ { key: 'b', value: 'test2' },
+ ]);
+ });
+ });
+
+ describe('.entriesAsPromise()', () => {
+ it('returns a Promise of an array { key, value }', () => {
+ const registry = new Registry();
+ registry.registerValue('a', 'test1');
+ registry.registerLoader('b', () => 'test2');
+ registry.registerLoader('c', () => Promise.resolve('test3'));
+ return registry.entriesAsPromise().then((entries) => {
+ expect(entries).to.deep.equal([
+ { key: 'a', value: 'test1' },
+ { key: 'b', value: 'test2' },
+ { key: 'c', value: 'test3' },
+ ]);
+ });
+ });
+ });
+
+ describe('.remove(key)', () => {
+ it('removes the item with given key', () => {
+ const registry = new Registry();
+ registry.registerValue('a', 'testValue');
+ registry.remove('a');
+ expect(registry.get('a')).to.equal(null);
+ });
+ it('does not throw error if the key does not exist', () => {
+ const registry = new Registry();
+ expect(() => registry.remove('a')).to.not.throw();
+ });
+ it('returns itself', () => {
+ const registry = new Registry();
+ registry.registerValue('a', 'testValue');
+ expect(registry.remove('a')).to.equal(registry);
+ });
+ });
+});
diff --git a/superset/assets/spec/javascripts/utils/isRequired_spec.js b/superset/assets/spec/javascripts/utils/isRequired_spec.js
new file mode 100644
index 0000000000..9a41663246
--- /dev/null
+++ b/superset/assets/spec/javascripts/utils/isRequired_spec.js
@@ -0,0 +1,9 @@
+import { it, describe } from 'mocha';
+import { expect } from 'chai';
+import isRequired from '../../../src/utils/isRequired';
+
+describe('isRequired(field)', () => {
+ it('should throw error with the given field in the message', () => {
+ expect(() => isRequired('myField')).to.throw(Error, 'myField is required.');
+ });
+});
diff --git a/superset/assets/spec/javascripts/utils/makeSingleton_spec.js b/superset/assets/spec/javascripts/utils/makeSingleton_spec.js
new file mode 100644
index 0000000000..686a89a6fc
--- /dev/null
+++ b/superset/assets/spec/javascripts/utils/makeSingleton_spec.js
@@ -0,0 +1,38 @@
+import { describe, it } from 'mocha';
+import { expect } from 'chai';
+import makeSingleton from '../../../src/utils/makeSingleton';
+
+describe('makeSingleton()', () => {
+ class Dog {
+ constructor(name) {
+ this.name = name;
+ }
+ sit() {
+ this.isSitting = true;
+ }
+ }
+ describe('makeSingleton(BaseClass)', () => {
+ const getInstance = makeSingleton(Dog);
+
+ it('returns a function for getting singleton instance of a given base class', () => {
+ expect(getInstance).to.be.a('Function');
+ expect(getInstance()).to.be.instanceOf(Dog);
+ });
+ it('returned function returns same instance across all calls', () => {
+ expect(getInstance()).to.equal(getInstance());
+ });
+ });
+ describe('makeSingleton(BaseClass, ...args)', () => {
+ const getInstance = makeSingleton(Dog, 'Doug');
+
+ it('returns a function for getting singleton instance of a given base class constructed with the given arguments', () => {
+ expect(getInstance).to.be.a('Function');
+ expect(getInstance()).to.be.instanceOf(Dog);
+ expect(getInstance().name).to.equal('Doug');
+ });
+ it('returned function returns same instance across all calls', () => {
+ expect(getInstance()).to.equal(getInstance());
+ });
+ });
+
+});
diff --git a/superset/assets/spec/javascripts/visualizations/models/ChartPlugin_spec.js b/superset/assets/spec/javascripts/visualizations/models/ChartPlugin_spec.js
new file mode 100644
index 0000000000..fb1c35f28e
--- /dev/null
+++ b/superset/assets/spec/javascripts/visualizations/models/ChartPlugin_spec.js
@@ -0,0 +1,42 @@
+import { describe, it } from 'mocha';
+import { expect } from 'chai';
+import ChartPlugin from '../../../../src/visualizations/core/models/ChartPlugin';
+import ChartMetadata from '../../../../src/visualizations/core/models/ChartMetadata';
+
+describe('ChartPlugin', () => {
+ const metadata = new ChartMetadata({});
+
+ it('exists', () => {
+ expect(ChartPlugin).to.not.equal(undefined);
+ });
+
+ describe('new ChartPlugin()', () => {
+ it('creates a new plugin', () => {
+ const plugin = new ChartPlugin({
+ metadata,
+ Chart() {},
+ });
+ expect(plugin).to.be.instanceof(ChartPlugin);
+ });
+ it('throws an error if metadata is not specified', () => {
+ expect(() => new ChartPlugin()).to.throw(Error);
+ });
+ it('throws an error if none of Chart or loadChart is specified', () => {
+ expect(() => new ChartPlugin({ metadata })).to.throw(Error);
+ });
+ });
+
+ describe('.register(key)', () => {
+ const plugin = new ChartPlugin({
+ metadata,
+ Chart() {},
+ });
+ it('throws an error if key is not provided', () => {
+ expect(() => plugin.register()).to.throw(Error);
+ expect(() => plugin.configure({ key: 'abc' }).register()).to.not.throw(Error);
+ });
+ it('returns itself', () => {
+ expect(plugin.configure({ key: 'abc' }).register()).to.equal(plugin);
+ });
+ });
+});
diff --git a/superset/assets/spec/javascripts/visualizations/models/Plugin_spec.js b/superset/assets/spec/javascripts/visualizations/models/Plugin_spec.js
new file mode 100644
index 0000000000..c40aeb6460
--- /dev/null
+++ b/superset/assets/spec/javascripts/visualizations/models/Plugin_spec.js
@@ -0,0 +1,48 @@
+import { describe, it } from 'mocha';
+import { expect } from 'chai';
+import Plugin from '../../../../src/visualizations/core/models/Plugin';
+
+describe('Plugin', () => {
+ it('exists', () => {
+ expect(Plugin).to.not.equal(undefined);
+ });
+
+ describe('new Plugin()', () => {
+ it('creates a new plugin', () => {
+ const plugin = new Plugin();
+ expect(plugin).to.be.instanceof(Plugin);
+ });
+ });
+
+ describe('.configure(config, replace)', () => {
+ it('extends the default config with given config when replace is not set or false', () => {
+ const plugin = new Plugin();
+ plugin.configure({ key: 'abc', foo: 'bar' });
+ plugin.configure({ key: 'def' });
+ expect(plugin.config).to.deep.equal({ key: 'def', foo: 'bar' });
+ });
+ it('replaces the default config with given config when replace is true', () => {
+ const plugin = new Plugin();
+ plugin.configure({ key: 'abc', foo: 'bar' });
+ plugin.configure({ key: 'def' }, true);
+ expect(plugin.config).to.deep.equal({ key: 'def' });
+ });
+ it('returns the plugin itself', () => {
+ const plugin = new Plugin();
+ expect(plugin.configure({ key: 'abc' })).to.equal(plugin);
+ });
+ });
+
+ describe('.resetConfig()', () => {
+ it('resets config back to default', () => {
+ const plugin = new Plugin();
+ plugin.configure({ key: 'abc', foo: 'bar' });
+ plugin.resetConfig();
+ expect(plugin.config).to.deep.equal({});
+ });
+ it('returns the plugin itself', () => {
+ const plugin = new Plugin();
+ expect(plugin.resetConfig()).to.equal(plugin);
+ });
+ });
+});
diff --git a/superset/assets/spec/javascripts/visualizations/models/Preset_spec.js b/superset/assets/spec/javascripts/visualizations/models/Preset_spec.js
new file mode 100644
index 0000000000..4e5f7d2ae5
--- /dev/null
+++ b/superset/assets/spec/javascripts/visualizations/models/Preset_spec.js
@@ -0,0 +1,65 @@
+import { describe, it } from 'mocha';
+import { expect } from 'chai';
+import Preset from '../../../../src/visualizations/core/models/Preset';
+import Plugin from '../../../../src/visualizations/core/models/Plugin';
+
+describe('Preset', () => {
+ it('exists', () => {
+ expect(Preset).to.not.equal(undefined);
+ });
+
+ describe('new Preset()', () => {
+ it('creates new preset', () => {
+ const preset = new Preset();
+ expect(preset).to.be.instanceOf(Preset);
+ });
+ });
+
+ describe('.register()', () => {
+ it('register all listed presets then plugins', () => {
+ const values = [];
+ class Plugin1 extends Plugin {
+ register() {
+ values.push(1);
+ }
+ }
+ class Plugin2 extends Plugin {
+ register() {
+ values.push(2);
+ }
+ }
+ class Plugin3 extends Plugin {
+ register() {
+ values.push(3);
+ }
+ }
+ class Plugin4 extends Plugin {
+ register() {
+ const { key } = this.config;
+ values.push(key);
+ }
+ }
+
+ const preset1 = new Preset({
+ plugins: [new Plugin1()],
+ });
+ const preset2 = new Preset({
+ plugins: [new Plugin2()],
+ });
+ const preset3 = new Preset({
+ presets: [preset1, preset2],
+ plugins: [
+ new Plugin3(),
+ new Plugin4().configure({ key: 'abc' }),
+ ],
+ });
+ preset3.register();
+ expect(values).to.deep.equal([1, 2, 3, 'abc']);
+ });
+
+ it('returns itself', () => {
+ const preset = new Preset();
+ expect(preset.register()).to.equal(preset);
+ });
+ });
+});
diff --git a/superset/assets/src/modules/Registry.js b/superset/assets/src/modules/Registry.js
new file mode 100644
index 0000000000..f39a0c5d99
--- /dev/null
+++ b/superset/assets/src/modules/Registry.js
@@ -0,0 +1,72 @@
+export default class Registry {
+ constructor(name = '') {
+ this.name = name;
+ this.items = {};
+ this.promises = {};
+ }
+
+ has(key) {
+ const item = this.items[key];
+ return item !== null && item !== undefined;
+ }
+
+ registerValue(key, value) {
+ this.items[key] = { value };
+ delete this.promises[key];
+ return this;
+ }
+
+ registerLoader(key, loader) {
+ this.items[key] = { loader };
+ delete this.promises[key];
+ return this;
+ }
+
+ get(key) {
+ const item = this.items[key];
+ if (item) {
+ return item.loader ? item.loader() : item.value;
+ }
+ return null;
+ }
+
+ getAsPromise(key) {
+ const promise = this.promises[key];
+ if (promise) {
+ return promise;
+ }
+ const item = this.get(key);
+ if (item) {
+ const newPromise = Promise.resolve(item);
+ this.promises[key] = newPromise;
+ return newPromise;
+ }
+ return Promise.reject(`Item with key "${key}" is not registered.`);
+ }
+
+ keys() {
+ return Object.keys(this.items);
+ }
+
+ entries() {
+ return this.keys().map(key => ({
+ key,
+ value: this.get(key),
+ }));
+ }
+
+ entriesAsPromise() {
+ const keys = this.keys();
+ return Promise.all(keys.map(key => this.getAsPromise(key)))
+ .then(values => values.map((value, i) => ({
+ key: keys[i],
+ value,
+ })));
+ }
+
+ remove(key) {
+ delete this.items[key];
+ delete this.promises[key];
+ return this;
+ }
+}
diff --git a/superset/assets/src/utils/isRequired.js b/superset/assets/src/utils/isRequired.js
new file mode 100644
index 0000000000..988c8cebf9
--- /dev/null
+++ b/superset/assets/src/utils/isRequired.js
@@ -0,0 +1,3 @@
+export default function isRequired(field) {
+ throw new Error(`${field} is required.`);
+}
diff --git a/superset/assets/src/utils/makeSingleton.js b/superset/assets/src/utils/makeSingleton.js
new file mode 100644
index 0000000000..3eee475788
--- /dev/null
+++ b/superset/assets/src/utils/makeSingleton.js
@@ -0,0 +1,10 @@
+export default function makeSingleton(BaseClass, ...args) {
+ let singleton;
+
+ return function getInstance() {
+ if (!singleton) {
+ singleton = new BaseClass(...args);
+ }
+ return singleton;
+ };
+}
diff --git a/superset/assets/src/visualizations/core/models/ChartMetadata.js b/superset/assets/src/visualizations/core/models/ChartMetadata.js
new file mode 100644
index 0000000000..3d528b8e1d
--- /dev/null
+++ b/superset/assets/src/visualizations/core/models/ChartMetadata.js
@@ -0,0 +1,13 @@
+export default class ChartMetadata {
+ constructor({
+ name,
+ description,
+ thumbnail,
+ show = true,
+ }) {
+ this.name = name;
+ this.description = description;
+ this.thumbnail = thumbnail;
+ this.show = show;
+ }
+}
diff --git a/superset/assets/src/visualizations/core/models/ChartPlugin.js b/superset/assets/src/visualizations/core/models/ChartPlugin.js
new file mode 100644
index 0000000000..a910185cf3
--- /dev/null
+++ b/superset/assets/src/visualizations/core/models/ChartPlugin.js
@@ -0,0 +1,43 @@
+import Plugin from './Plugin';
+import isRequired from '../../../utils/isRequired';
+import getChartMetadataRegistry from '../registries/ChartMetadataRegistrySingleton';
+import getChartComponentRegistry from '../registries/ChartComponentRegistrySingleton';
+import getChartTransformPropsRegistry from '../registries/ChartTransformPropsRegistrySingleton';
+
+const IDENTITY = x => x;
+
+export default class ChartPlugin extends Plugin {
+ constructor({
+ metadata = isRequired('metadata'),
+
+ // use transformProps for immediate value
+ transformProps = IDENTITY,
+ // use loadTransformProps for dynamic import (lazy-loading)
+ loadTransformProps,
+
+ // use Chart for immediate value
+ Chart,
+ // use loadChart for dynamic import (lazy-loading)
+ loadChart,
+ } = {}) {
+ super();
+ this.metadata = metadata;
+ this.loadTransformProps = loadTransformProps || (() => transformProps);
+
+ if (loadChart) {
+ this.loadChart = loadChart;
+ } else if (Chart) {
+ this.loadChart = () => Chart;
+ } else {
+ throw new Error('Chart or loadChart is required');
+ }
+ }
+
+ register() {
+ const { key = isRequired('config.key') } = this.config;
+ getChartMetadataRegistry().registerValue(key, this.metadata);
+ getChartComponentRegistry().registerLoader(key, this.loadChart);
+ getChartTransformPropsRegistry().registerLoader(key, this.loadTransformProps);
+ return this;
+ }
+}
diff --git a/superset/assets/src/visualizations/core/models/Plugin.js b/superset/assets/src/visualizations/core/models/Plugin.js
new file mode 100644
index 0000000000..33609f6da0
--- /dev/null
+++ b/superset/assets/src/visualizations/core/models/Plugin.js
@@ -0,0 +1,25 @@
+export default class Plugin {
+ constructor() {
+ this.resetConfig();
+ }
+
+ resetConfig() {
+ // The child class can set default config
+ // by overriding this function.
+ this.config = {};
+ return this;
+ }
+
+ configure(config, replace = false) {
+ if (replace) {
+ this.config = config;
+ } else {
+ this.config = { ...this.config, ...config };
+ }
+ return this;
+ }
+
+ register() {
+ return this;
+ }
+}
diff --git a/superset/assets/src/visualizations/core/models/Preset.js b/superset/assets/src/visualizations/core/models/Preset.js
new file mode 100644
index 0000000000..557351d6d2
--- /dev/null
+++ b/superset/assets/src/visualizations/core/models/Preset.js
@@ -0,0 +1,23 @@
+export default class Preset {
+ constructor({
+ name = '',
+ description = '',
+ presets = [],
+ plugins = [],
+ } = {}) {
+ this.name = name;
+ this.description = description;
+ this.presets = presets;
+ this.plugins = plugins;
+ }
+
+ register() {
+ this.presets.forEach((preset) => {
+ preset.register();
+ });
+ this.plugins.forEach((plugin) => {
+ plugin.register();
+ });
+ return this;
+ }
+}
diff --git a/superset/assets/src/visualizations/core/registries/ChartComponentRegistrySingleton.js b/superset/assets/src/visualizations/core/registries/ChartComponentRegistrySingleton.js
new file mode 100644
index 0000000000..df7d74356a
--- /dev/null
+++ b/superset/assets/src/visualizations/core/registries/ChartComponentRegistrySingleton.js
@@ -0,0 +1,12 @@
+import Registry from '../../../modules/Registry';
+import makeSingleton from '../../../utils/makeSingleton';
+
+class ChartComponentRegistry extends Registry {
+ constructor() {
+ super('ChartComponent');
+ }
+}
+
+const getInstance = makeSingleton(ChartComponentRegistry);
+
+export default getInstance;
diff --git a/superset/assets/src/visualizations/core/registries/ChartMetadataRegistrySingleton.js b/superset/assets/src/visualizations/core/registries/ChartMetadataRegistrySingleton.js
new file mode 100644
index 0000000000..e1c569bc05
--- /dev/null
+++ b/superset/assets/src/visualizations/core/registries/ChartMetadataRegistrySingleton.js
@@ -0,0 +1,12 @@
+import Registry from '../../../modules/Registry';
+import makeSingleton from '../../../utils/makeSingleton';
+
+class ChartMetadataRegistry extends Registry {
+ constructor() {
+ super('ChartMetadata');
+ }
+}
+
+const getInstance = makeSingleton(ChartMetadataRegistry);
+
+export default getInstance;
diff --git a/superset/assets/src/visualizations/core/registries/ChartTransformPropsRegistrySingleton.js b/superset/assets/src/visualizations/core/registries/ChartTransformPropsRegistrySingleton.js
new file mode 100644
index 0000000000..a26fab58d5
--- /dev/null
+++ b/superset/assets/src/visualizations/core/registries/ChartTransformPropsRegistrySingleton.js
@@ -0,0 +1,12 @@
+import Registry from '../../../modules/Registry';
+import makeSingleton from '../../../utils/makeSingleton';
+
+class ChartTransformPropsRegistry extends Registry {
+ constructor() {
+ super('ChartTransformProps');
+ }
+}
+
+const getInstance = makeSingleton(ChartTransformPropsRegistry);
+
+export default getInstance;
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org