feat: allow chart plugin to be unregistered (apache#168)

* feat: allow chart plugin to be unregistered * test: address edge cases
graceguo-supercat · Jun 12, 2019 · 32f8dd3 · 32f8dd3
1 parent a405572
commit 32f8dd3
Show file tree

Hide file tree

Showing 4 changed files with 91 additions and 13 deletions.
diff --git a/packages/superset-ui-chart/src/models/ChartPlugin.ts b/packages/superset-ui-chart/src/models/ChartPlugin.ts
@@ -17,17 +17,17 @@ type ValueOrModuleWithValue<T> = T | { default: T };
 
 interface ChartPluginConfig<T extends ChartFormData> {
   metadata: ChartMetadata;
-  // use buildQuery for immediate value
+  /** Use buildQuery for immediate value. For lazy-loading, use loadBuildQuery. */
   buildQuery?: BuildQueryFunction<T>;
-  // use loadBuildQuery for dynamic import (lazy-loading)
+  /** Use loadBuildQuery for dynamic import (lazy-loading) */
   loadBuildQuery?: PromiseOrValueLoader<ValueOrModuleWithValue<BuildQueryFunction<T>>>;
-  // use transformProps for immediate value
+  /** Use transformProps for immediate value. For lazy-loading, use loadTransformProps.  */
   transformProps?: TransformProps;
-  // use loadTransformProps for dynamic import (lazy-loading)
+  /** Use loadTransformProps for dynamic import (lazy-loading) */
   loadTransformProps?: PromiseOrValueLoader<ValueOrModuleWithValue<TransformProps>>;
-  // use Chart for immediate value
+  /** Use Chart for immediate value. For lazy-loading, use loadChart. */
   Chart?: ChartType;
-  // use loadChart for dynamic import (lazy-loading)
+  /** Use loadChart for dynamic import (lazy-loading) */
   loadChart?: PromiseOrValueLoader<ValueOrModuleWithValue<ChartType>>;
 }
 
@@ -92,6 +92,16 @@ export default class ChartPlugin<T extends ChartFormData = ChartFormData> extend
     return this;
   }
 
+  unregister() {
+    const { key = isRequired('config.key') } = this.config;
+    getChartMetadataRegistry().remove(key);
+    getChartComponentRegistry().remove(key);
+    getChartTransformPropsRegistry().remove(key);
+    getChartBuildQueryRegistry().remove(key);
+
+    return this;
+  }
+
   configure(config: { [key: string]: any }, replace?: boolean) {
     super.configure(config, replace);
 

diff --git a/packages/superset-ui-chart/test/models/ChartPlugin.test.tsx b/packages/superset-ui-chart/test/models/ChartPlugin.test.tsx
@@ -8,6 +8,10 @@ import {
   ChartProps,
   BuildQueryFunction,
   TransformProps,
+  getChartMetadataRegistry,
+  getChartComponentRegistry,
+  getChartTransformPropsRegistry,
+  getChartBuildQueryRegistry,
 } from '../../src';
 
 describe('ChartPlugin', () => {
@@ -18,7 +22,7 @@ describe('ChartPlugin', () => {
     thumbnail: '',
   });
 
-  const buildQuery = (_: ChartFormData) => ({
+  const buildQuery = () => ({
     datasource: { id: 1, type: DatasourceType.Table },
     queries: [{ granularity: 'day' }],
   });
@@ -132,17 +136,70 @@ describe('ChartPlugin', () => {
   });
 
   describe('.register()', () => {
-    const plugin = new ChartPlugin({
-      metadata,
-      Chart: FakeChart,
-      buildQuery,
+    let plugin: ChartPlugin;
+
+    beforeEach(() => {
+      plugin = new ChartPlugin({
+        metadata,
+        Chart: FakeChart,
+        buildQuery,
+      });
     });
+
     it('throws an error if key is not provided', () => {
       expect(() => plugin.register()).toThrowError(Error);
-      expect(() => plugin.configure({ key: 'abc' }).register()).not.toThrowError(Error);
+      expect(() => plugin.configure({ key: 'ab' }).register()).not.toThrowError(Error);
+    });
+    it('add the plugin to the registries', () => {
+      plugin.configure({ key: 'cd' }).register();
+      expect(getChartMetadataRegistry().get('cd')).toBe(metadata);
+      expect(getChartComponentRegistry().get('cd')).toBe(FakeChart);
+      expect(getChartTransformPropsRegistry().has('cd')).toEqual(true);
+      expect(getChartBuildQueryRegistry().get('cd')).toBe(buildQuery);
+    });
+    it('does not register buildQuery when it is not specified in the ChartPlugin', () => {
+      new ChartPlugin({
+        metadata,
+        Chart: FakeChart,
+      })
+        .configure({ key: 'ef' })
+        .register();
+      expect(getChartBuildQueryRegistry().has('ef')).toEqual(false);
+    });
+    it('returns itself', () => {
+      expect(plugin.configure({ key: 'gh' }).register()).toBe(plugin);
+    });
+  });
+
+  describe('.unregister()', () => {
+    let plugin: ChartPlugin;
+
+    beforeEach(() => {
+      plugin = new ChartPlugin({
+        metadata,
+        Chart: FakeChart,
+        buildQuery,
+      });
+    });
+
+    it('throws an error if key is not provided', () => {
+      expect(() => plugin.unregister()).toThrowError(Error);
+      expect(() => plugin.configure({ key: 'abc' }).unregister()).not.toThrowError(Error);
+    });
+    it('removes the chart from the registries', () => {
+      plugin.configure({ key: 'def' }).register();
+      expect(getChartMetadataRegistry().get('def')).toBe(metadata);
+      expect(getChartComponentRegistry().get('def')).toBe(FakeChart);
+      expect(getChartTransformPropsRegistry().has('def')).toEqual(true);
+      expect(getChartBuildQueryRegistry().get('def')).toBe(buildQuery);
+      plugin.unregister();
+      expect(getChartMetadataRegistry().has('def')).toEqual(false);
+      expect(getChartComponentRegistry().has('def')).toEqual(false);
+      expect(getChartTransformPropsRegistry().has('def')).toEqual(false);
+      expect(getChartBuildQueryRegistry().has('def')).toEqual(false);
     });
     it('returns itself', () => {
-      expect(plugin.configure({ key: 'abc' }).register()).toBe(plugin);
+      expect(plugin.configure({ key: 'xyz' }).unregister()).toBe(plugin);
     });
   });
 });
diff --git a/packages/superset-ui-core/src/models/Plugin.ts b/packages/superset-ui-core/src/models/Plugin.ts
@@ -26,4 +26,8 @@ export default class Plugin {
   register() {
     return this;
   }
+
+  unregister() {
+    return this;
+  }
 }
diff --git a/packages/superset-ui-core/test/models/Plugin.test.ts b/packages/superset-ui-core/test/models/Plugin.test.ts
@@ -50,4 +50,11 @@ describe('Plugin', () => {
       expect(plugin.register()).toBe(plugin);
     });
   });
+
+  describe('.unregister()', () => {
+    it('returns the plugin itself', () => {
+      const plugin = new Plugin();
+      expect(plugin.unregister()).toBe(plugin);
+    });
+  });
 });