chore(chart): remove faux @superset-ui/core TS declaration (#121)

* chore(chart): remove faux @superset-ui/core TS declaration * test(chart): test all ChartPlugin.register() branches * refactor(chart): support loaders that return esmodules * refactor(chart): rename ChartMetaDataConfig => ChartMetadataConfig * test(chart): fix loader test + branch coverage * test(chart): hit all branches in sanitizeLoader * refactor(chart): use ChartMetadata in registry
zhaoyongjie · Nov 26, 2021 · fea7c58 · fea7c58
1 parent 30cc661
commit fea7c58
Show file tree

Hide file tree

Showing 15 changed files with 133 additions and 94 deletions.
diff --git a/...d/temporary_superset_ui/superset-ui/packages/superset-ui-chart/src/clients/ChartClient.ts b/...d/temporary_superset_ui/superset-ui/packages/superset-ui-chart/src/clients/ChartClient.ts
@@ -74,12 +74,14 @@ export default class ChartClient {
       : Promise.reject(new Error('At least one of sliceId or formData must be specified'));
   }
 
-  loadQueryData(formData: ChartFormData, options?: Partial<RequestConfig>): Promise<object> {
+  async loadQueryData(formData: ChartFormData, options?: Partial<RequestConfig>): Promise<object> {
     const { viz_type: visType } = formData;
+    const metaDataRegistry = getChartMetadataRegistry();
+    const buildQueryRegistry = getChartBuildQueryRegistry();
 
-    if (getChartMetadataRegistry().has(visType)) {
-      const { useLegacyApi } = getChartMetadataRegistry().get(visType);
-      const buildQuery = useLegacyApi ? () => formData : getChartBuildQueryRegistry().get(visType);
+    if (metaDataRegistry.has(visType)) {
+      const { useLegacyApi } = metaDataRegistry.get(visType)!;
+      const buildQuery = (await buildQueryRegistry.get(visType)) || (() => formData);
 
       return this.client
         .post({

diff --git a/...emporary_superset_ui/superset-ui/packages/superset-ui-chart/src/components/SuperChart.tsx b/...emporary_superset_ui/superset-ui/packages/superset-ui-chart/src/components/SuperChart.tsx
@@ -4,6 +4,8 @@ import getChartComponentRegistry from '../registries/ChartComponentRegistrySingl
 import getChartTransformPropsRegistry from '../registries/ChartTransformPropsRegistrySingleton';
 import ChartProps from '../models/ChartProps';
 import createLoadableRenderer, { LoadableRenderer } from './createLoadableRenderer';
+import { ChartType } from '../models/ChartPlugin';
+import { PreTransformProps, TransformProps, PostTransformProps } from '../types/Query';
 
 const IDENTITY = (x: any) => x;
 
@@ -21,26 +23,21 @@ const defaultProps = {
 };
 /* eslint-enable sort-keys */
 
-type TransformFunction<Input = PlainProps, Output = PlainProps> = (x: Input) => Output;
 type HandlerFunction = (...args: any[]) => void;
 
 interface LoadingProps {
   error: any;
 }
 
-interface PlainProps {
-  [key: string]: any;
-}
-
 interface LoadedModules {
-  Chart: React.Component | { default: React.Component };
-  transformProps: TransformFunction | { default: TransformFunction };
+  Chart: ChartType;
+  transformProps: TransformProps;
 }
 
 interface RenderProps {
   chartProps: ChartProps;
-  preTransformProps?: TransformFunction<ChartProps>;
-  postTransformProps?: TransformFunction;
+  preTransformProps?: PreTransformProps;
+  postTransformProps?: PostTransformProps;
 }
 
 const BLANK_CHART_PROPS = new ChartProps();
@@ -50,17 +47,13 @@ export interface SuperChartProps {
   className?: string;
   chartProps?: ChartProps | null;
   chartType: string;
-  preTransformProps?: TransformFunction<ChartProps>;
-  overrideTransformProps?: TransformFunction;
-  postTransformProps?: TransformFunction;
+  preTransformProps?: PreTransformProps;
+  overrideTransformProps?: TransformProps;
+  postTransformProps?: PostTransformProps;
   onRenderSuccess?: HandlerFunction;
   onRenderFailure?: HandlerFunction;
 }
 
-function getModule<T>(value: any): T {
-  return (value.default ? value.default : value) as T;
-}
-
 export default class SuperChart extends React.PureComponent<SuperChartProps, {}> {
   static defaultProps = defaultProps;
 
@@ -125,19 +118,18 @@ export default class SuperChart extends React.PureComponent<SuperChartProps, {}>
 
   processChartProps: (input: {
     chartProps: ChartProps;
-    preTransformProps?: TransformFunction<ChartProps>;
-    transformProps?: TransformFunction;
-    postTransformProps?: TransformFunction;
+    preTransformProps?: PreTransformProps;
+    transformProps?: TransformProps;
+    postTransformProps?: PostTransformProps;
   }) => any;
 
   createLoadableRenderer: (input: {
     chartType: string;
-    overrideTransformProps?: TransformFunction;
+    overrideTransformProps?: TransformProps;
   }) => LoadableRenderer<RenderProps, LoadedModules> | (() => null);
 
   renderChart(loaded: LoadedModules, props: RenderProps) {
-    const Chart = getModule<typeof React.Component>(loaded.Chart);
-    const transformProps = getModule<TransformFunction>(loaded.transformProps);
+    const { Chart, transformProps } = loaded;
     const { chartProps, preTransformProps, postTransformProps } = props;
 
     return (

diff --git a/.../temporary_superset_ui/superset-ui/packages/superset-ui-chart/src/models/ChartMetadata.ts b/.../temporary_superset_ui/superset-ui/packages/superset-ui-chart/src/models/ChartMetadata.ts
@@ -2,6 +2,17 @@ interface LookupTable {
   [key: string]: boolean;
 }
 
+export interface ChartMetadataConfig {
+  name: string;
+  canBeAnnotationTypes?: string[];
+  credits?: string[];
+  description?: string;
+  show?: boolean;
+  supportedAnnotationTypes?: string[];
+  thumbnail: string;
+  useLegacyApi?: boolean;
+}
+
 export default class ChartMetadata {
   name: string;
   canBeAnnotationTypes?: string[];
@@ -13,16 +24,7 @@ export default class ChartMetadata {
   thumbnail: string;
   useLegacyApi: boolean;
 
-  constructor(config: {
-    name: string;
-    canBeAnnotationTypes?: string[];
-    credits?: string[];
-    description?: string;
-    show?: boolean;
-    supportedAnnotationTypes?: string[];
-    thumbnail: string;
-    useLegacyApi?: boolean;
-  }) {
+  constructor(config: ChartMetadataConfig) {
     const {
       name,
       canBeAnnotationTypes = [],

diff --git a/...nd/temporary_superset_ui/superset-ui/packages/superset-ui-chart/src/models/ChartPlugin.ts b/...nd/temporary_superset_ui/superset-ui/packages/superset-ui-chart/src/models/ChartPlugin.ts
@@ -1,47 +1,57 @@
+import { FunctionComponent, ComponentType } from 'react';
 import { isRequired, Plugin } from '@superset-ui/core';
 import ChartMetadata from './ChartMetadata';
-import ChartProps from './ChartProps';
 import getChartMetadataRegistry from '../registries/ChartMetadataRegistrySingleton';
 import getChartBuildQueryRegistry from '../registries/ChartBuildQueryRegistrySingleton';
 import getChartComponentRegistry from '../registries/ChartComponentRegistrySingleton';
 import getChartTransformPropsRegistry from '../registries/ChartTransformPropsRegistrySingleton';
 import { ChartFormData } from '../types/ChartFormData';
-import { QueryContext } from '../types/Query';
+import { BuildQueryFunction, TransformProps } from '../types/Query';
 
 const IDENTITY = (x: any) => x;
 
-type PromiseOrValue<T> = Promise<T> | T;
-type PromiseOrValueLoader<T> = () => PromiseOrValue<T> | PromiseOrValue<{ default: T }>;
-
-export type BuildQueryFunction<T extends ChartFormData> = (formData: T) => QueryContext;
-
-export type TransformPropsFunction = (
-  chartProps: ChartProps,
-) => {
-  [key: string]: any;
-};
+export type PromiseOrValue<T> = Promise<T> | T;
+export type PromiseOrValueLoader<T> = () => PromiseOrValue<T>;
+export type ChartType = ComponentType | FunctionComponent;
+type ValueOrModuleWithValue<T> = T | { default: T };
 
 interface ChartPluginConfig<T extends ChartFormData> {
   metadata: ChartMetadata;
   // use buildQuery for immediate value
   buildQuery?: BuildQueryFunction<T>;
   // use loadBuildQuery for dynamic import (lazy-loading)
-  loadBuildQuery?: PromiseOrValueLoader<BuildQueryFunction<T>>;
+  loadBuildQuery?: PromiseOrValueLoader<ValueOrModuleWithValue<BuildQueryFunction<T>>>;
   // use transformProps for immediate value
-  transformProps?: TransformPropsFunction;
+  transformProps?: TransformProps;
   // use loadTransformProps for dynamic import (lazy-loading)
-  loadTransformProps?: PromiseOrValueLoader<TransformPropsFunction>;
+  loadTransformProps?: PromiseOrValueLoader<ValueOrModuleWithValue<TransformProps>>;
   // use Chart for immediate value
-  Chart?: Function;
+  Chart?: ChartType;
   // use loadChart for dynamic import (lazy-loading)
-  loadChart?: PromiseOrValueLoader<Function>;
+  loadChart?: PromiseOrValueLoader<ValueOrModuleWithValue<ChartType>>;
+}
+
+/**
+ * Loaders of the form `() => import('foo')` may return esmodules
+ * which require the value to be extracted as `module.default`
+ * */
+function sanitizeLoader<T>(
+  loader: PromiseOrValueLoader<ValueOrModuleWithValue<T>>,
+): PromiseOrValueLoader<T> {
+  return () => {
+    const loaded = loader();
+
+    return loaded instanceof Promise
+      ? (loaded.then(module => ('default' in module && module.default) || module) as Promise<T>)
+      : (loaded as T);
+  };
 }
 
 export default class ChartPlugin<T extends ChartFormData = ChartFormData> extends Plugin {
   metadata: ChartMetadata;
   loadBuildQuery?: PromiseOrValueLoader<BuildQueryFunction<T>>;
-  loadTransformProps: PromiseOrValueLoader<TransformPropsFunction>;
-  loadChart: PromiseOrValueLoader<Function>;
+  loadTransformProps: PromiseOrValueLoader<TransformProps>;
+  loadChart: PromiseOrValueLoader<ChartType>;
 
   constructor(config: ChartPluginConfig<T>) {
     super();
@@ -55,11 +65,14 @@ export default class ChartPlugin<T extends ChartFormData = ChartFormData> extend
       loadChart,
     } = config;
     this.metadata = metadata;
-    this.loadBuildQuery = loadBuildQuery || (buildQuery ? () => buildQuery : undefined);
-    this.loadTransformProps = loadTransformProps || (() => transformProps);
+    this.loadBuildQuery =
+      (loadBuildQuery && sanitizeLoader(loadBuildQuery)) ||
+      (buildQuery && sanitizeLoader(() => buildQuery)) ||
+      undefined;
+    this.loadTransformProps = sanitizeLoader(loadTransformProps || (() => transformProps));
 
     if (loadChart) {
-      this.loadChart = loadChart;
+      this.loadChart = sanitizeLoader<ChartType>(loadChart);
     } else if (Chart) {
       this.loadChart = () => Chart;
     } else {
@@ -70,9 +83,11 @@ export default class ChartPlugin<T extends ChartFormData = ChartFormData> extend
   register() {
     const { key = isRequired('config.key') } = this.config;
     getChartMetadataRegistry().registerValue(key, this.metadata);
-    getChartBuildQueryRegistry().registerLoader(key, this.loadBuildQuery);
     getChartComponentRegistry().registerLoader(key, this.loadChart);
     getChartTransformPropsRegistry().registerLoader(key, this.loadTransformProps);
+    if (this.loadBuildQuery) {
+      getChartBuildQueryRegistry().registerLoader(key, this.loadBuildQuery);
+    }
 
     return this;
   }

diff --git a/...superset-ui/packages/superset-ui-chart/src/registries/ChartBuildQueryRegistrySingleton.ts b/...superset-ui/packages/superset-ui-chart/src/registries/ChartBuildQueryRegistrySingleton.ts
@@ -1,6 +1,10 @@
 import { Registry, makeSingleton, OverwritePolicy } from '@superset-ui/core';
+import { QueryContext } from '../types/Query';
 
-class ChartBuildQueryRegistry extends Registry {
+// Ideally this would be <T extends ChartFormData>
+type BuildQuery = (formData: any) => QueryContext;
+
+class ChartBuildQueryRegistry extends Registry<BuildQuery> {
   constructor() {
     super({ name: 'ChartBuildQuery', overwritePolicy: OverwritePolicy.WARN });
   }

diff --git a/.../superset-ui/packages/superset-ui-chart/src/registries/ChartComponentRegistrySingleton.ts b/.../superset-ui/packages/superset-ui-chart/src/registries/ChartComponentRegistrySingleton.ts
@@ -1,6 +1,7 @@
 import { Registry, makeSingleton, OverwritePolicy } from '@superset-ui/core';
+import { ChartType } from '../models/ChartPlugin';
 
-class ChartComponentRegistry extends Registry {
+class ChartComponentRegistry extends Registry<ChartType> {
   constructor() {
     super({ name: 'ChartComponent', overwritePolicy: OverwritePolicy.WARN });
   }

diff --git a/...i/superset-ui/packages/superset-ui-chart/src/registries/ChartMetadataRegistrySingleton.ts b/...i/superset-ui/packages/superset-ui-chart/src/registries/ChartMetadataRegistrySingleton.ts
@@ -1,6 +1,7 @@
 import { Registry, makeSingleton, OverwritePolicy } from '@superset-ui/core';
+import ChartMetadata from '../models/ChartMetadata';
 
-class ChartMetadataRegistry extends Registry {
+class ChartMetadataRegistry extends Registry<ChartMetadata, ChartMetadata> {
   constructor() {
     super({ name: 'ChartMetadata', overwritePolicy: OverwritePolicy.WARN });
   }

diff --git a/...rset-ui/packages/superset-ui-chart/src/registries/ChartTransformPropsRegistrySingleton.ts b/...rset-ui/packages/superset-ui-chart/src/registries/ChartTransformPropsRegistrySingleton.ts
@@ -1,6 +1,7 @@
 import { Registry, makeSingleton, OverwritePolicy } from '@superset-ui/core';
+import { TransformProps } from '../types/Query';
 
-class ChartTransformPropsRegistry extends Registry {
+class ChartTransformPropsRegistry extends Registry<TransformProps> {
   constructor() {
     super({ name: 'ChartTransformProps', overwritePolicy: OverwritePolicy.WARN });
   }

diff --git a/...-frontend/temporary_superset_ui/superset-ui/packages/superset-ui-chart/src/types/Query.ts b/...-frontend/temporary_superset_ui/superset-ui/packages/superset-ui-chart/src/types/Query.ts
@@ -1,8 +1,8 @@
 /* eslint camelcase: 0 */
-import ChartProps from '../models/ChartProps';
 import { DatasourceType } from './Datasource';
 import { ChartFormData } from './ChartFormData';
 import { Metric } from './Metric';
+import ChartProps from '../models/ChartProps';
 
 export interface QueryObject {
   granularity: string;
@@ -32,10 +32,14 @@ export interface QueryContext {
   queries: QueryObject[];
 }
 
-export type BuildQueryFunction<T extends ChartFormData> = (formData: T) => QueryContext;
-
-export type TransformPropsFunction = (
-  chartProps: ChartProps,
-) => {
+export interface PlainProps {
   [key: string]: any;
-};
+}
+
+type TransformFunction<Input = PlainProps, Output = PlainProps> = (x: Input) => Output;
+
+export type PreTransformProps = TransformFunction<ChartProps>;
+export type TransformProps = TransformFunction;
+export type PostTransformProps = TransformFunction;
+
+export type BuildQueryFunction<T extends ChartFormData> = (formData: T) => QueryContext;
diff --git a/...orary_superset_ui/superset-ui/packages/superset-ui-chart/test/clients/ChartClient.test.ts b/...orary_superset_ui/superset-ui/packages/superset-ui-chart/test/clients/ChartClient.test.ts
@@ -7,6 +7,7 @@ import {
   buildQueryContext,
   ChartFormData,
   getChartMetadataRegistry,
+  ChartMetadata,
 } from '../../src';
 
 import { SliceIdAndOrFormData } from '../../src/clients/ChartClient';
@@ -97,7 +98,10 @@ describe('ChartClient', () => {
 
   describe('.loadQueryData(formData, options)', () => {
     it('returns a promise of query data for known chart type', () => {
-      getChartMetadataRegistry().registerValue('word_cloud', { name: 'Word Cloud' });
+      getChartMetadataRegistry().registerValue(
+        'word_cloud',
+        new ChartMetadata({ name: 'Word Cloud', thumbnail: '' }),
+      );
 
       getChartBuildQueryRegistry().registerValue('word_cloud', (formData: ChartFormData) =>
         buildQueryContext(formData),
@@ -129,10 +133,14 @@ describe('ChartClient', () => {
 
     it('fetches data from the legacy API if ChartMetadata has useLegacyApi=true,', () => {
       // note legacy charts do not register a buildQuery function in the registry
-      getChartMetadataRegistry().registerValue('word_cloud_legacy', {
-        name: 'Legacy Word Cloud',
-        useLegacyApi: true,
-      });
+      getChartMetadataRegistry().registerValue(
+        'word_cloud_legacy',
+        new ChartMetadata({
+          name: 'Legacy Word Cloud',
+          thumbnail: '.png',
+          useLegacyApi: true,
+        }),
+      );
 
       fetchMock.post('glob:*/api/v1/query/', () =>
         Promise.reject(Error('Unexpected all to v1 API')),
@@ -233,7 +241,10 @@ describe('ChartClient', () => {
         amet: true,
       });
 
-      getChartMetadataRegistry().registerValue('line', { name: 'Line' });
+      getChartMetadataRegistry().registerValue(
+        'line',
+        new ChartMetadata({ name: 'Line', thumbnail: '.gif' }),
+      );
 
       getChartBuildQueryRegistry().registerValue('line', (formData: ChartFormData) =>
         buildQueryContext(formData),

diff --git a/...ry_superset_ui/superset-ui/packages/superset-ui-chart/test/components/SuperChart.test.tsx b/...ry_superset_ui/superset-ui/packages/superset-ui-chart/test/components/SuperChart.test.tsx
@@ -28,8 +28,10 @@ describe('SuperChart', () => {
           name: 'second-chart',
           thumbnail: '',
         }),
+        // this mirrors `() => import(module)` syntax
         loadChart: () => Promise.resolve({ default: TestComponent }),
-        transformProps: x => x,
+        // promise without .default
+        loadTransformProps: () => Promise.resolve((x: any) => x),
       });
     }
   }
@@ -42,7 +44,7 @@ describe('SuperChart', () => {
           thumbnail: '',
         }),
         loadChart: () =>
-          new Promise<Function>(resolve => {
+          new Promise(resolve => {
             setTimeout(() => {
               resolve(TestComponent);
             }, 1000);