[GitHub] [incubator-superset] kristw edited a comment on issue #9324: [WiP] - Dynamically import of viz plugins, take 1

[GitBox <gi...@apache.org>]

kristw edited a comment on issue #9324: [WiP] - Dynamically import of viz plugins, take 1
URL: https://github.com/apache/incubator-superset/pull/9324#issuecomment-600912882
 
 
   After seeing the PR, I added some more thoughts regarding dynamic plugins in [SIP-38](https://github.com/apache/incubator-superset/issues/9187)
    for question about high-level motivation and if dynamic import is the right path.
   
   For this PR, itself:
   
   ### 1) The plugins are already using dynamic import under the hood. Adding dynamic import on the entire plugin level may not save much bundle size.
   
   Each `ChartPlugin` has 5 parts 
   * `metadata` - very small js object
   * `buildQuery` - could be big or small, **support dynamic import**
   * `Chart` - could be big or small, **support dynamic import**
   * `transformProps` - could be big or small, **support dynamic import**
   * `controlPanel` - small js object.
   
   For example
   
   ```ts
   export default class LineChartPlugin extends ChartPlugin {
     constructor() {
       super({
         // metadata does not use dynamic import since it is so small.
         metadata: createMetadata(),
         // this plugin uses dynamic import for the Chart part and will only load it when it is used.
         loadChart: () => import('./LineChart'),
         // this plugins does not use dynamic import for the transformProps function because it is not complex and does not require additional dependencies.
         loadTransformProps: () => import('./transformProps'),
         // this plugin uses dynamic import for the buildQuery part and will only load it when it is used. 
         loadBuildQuery: () => import('./buildQuery'),
         // this is the plain object we want to move from incubator-superset
         controlPanel: {...},
       });
     }
   }
   ```
   
   It is also flexible for opt-out. If the part is so small. 
   
   ```ts
   export default class BarChartPlugin extends ChartPlugin {
     constructor() {
       super({
         // metadata does not use dynamic import since it is so small.
         metadata: createMetadata(),
         // this plugins does not use dynamic import for the Chart, 
         // which could be because it is small and does not require additional dependencies.
         Chart: BarChart,
         // this plugins does not use dynamic import for the transformProps function
         // which could be because it is small and does not require additional dependencies.
         transformProps,
         // this plugins does not use dynamic import for the buildQuery function
         // which could be because it is small and does not require additional dependencies.
         buildQuery,
         // this is the plain object we want to move from incubator-superset
         controlPanel: {...},
       });
     }
   }
   ```
   
   ### 2) There are reasons why plugin registration must be synchronous.
   
   What happen during the `.register()` call is each plugin call the 5 singleton to tell that a visualization with this `key` exists.
   
   from [ChartPlugin.ts](https://github.com/apache-superset/superset-ui/blob/master/packages/superset-ui-chart/src/models/ChartPlugin.ts)
   
   ```ts
     register() {
       const { key = isRequired('config.key') } = this.config;
       getChartMetadataRegistry().registerValue(key, this.metadata);
       getChartComponentRegistry().registerLoader(key, this.loadChart);
       getChartControlPanelRegistry().registerValue(key, this.controlPanel);
       getChartTransformPropsRegistry().registerLoader(key, this.loadTransformProps);
       if (this.loadBuildQuery) {
         getChartBuildQueryRegistry().registerLoader(key, this.loadBuildQuery);
       }
   
       return this;
     }
   ```
   
   All of these registration happens during the **setup** phase of the app (see all the files under `setup` directory). All of these operations are executed **before any of the application (react, ui) code is called**. 
   
   Now in the application code, there are a few places that utilizes these registries to figure out what it should do based on the `vizType`. 
   
   * `SuperChart` uses these singleton registries to load the corresponding chart code.
   * `VizTypeControl.jsx` get the list of all vis types registered to the singleton to figure out what visualization to show in the dialog box.
   * The code for deciding whether to call legacy api or `/api/v1/query` will need to look up `metadata` from the `ChartMetadataRegistry` 
   
   ```ts
   const metadata = getChartMetadataRegistry().get(vizType);
   if (metadata.useLegacyApi) { ... }
   ```
   
   None of this will work if the plugin registrations are asynchronous. 

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to 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