[WiP] - Dynamically import of viz plugins, take 1

[rusackas]

CATEGORY

Choose one

• Bug Fix
• Enhancement (new features, refinement)
• Refactor
• Add tests
• Build / Development Environment
• Documentation

SUMMARY

Trying to use dynamic loading to pull in plugins. This seems to work fine with using the npm packages as you see in this code. However, if you switch the package name to a relative path pointing at the plugins /src folder, webpack tries to load it but chokes due to not applying the right loaders. Not quite sure what to do with it at this point.

Also note that for this to work, there will be a prerequisite change in superset-ui since dynamic imports return promises. In Preset.ts you need to change:

    this.plugins.forEach(plugin => {
      plugin.register();
    });

to:

    this.plugins.forEach(plugin => {
      if(plugin instanceof Promise) plugin.then(res => res.register())
      else plugin.register();
    });

then:

• cd into /superset-ui/ and yarn build
• cd into /superset-ui/packages/superset-ui-core and npm link
• cd into incubator-superset/superset-frontend and hitnpm link @superset-ui/core
• (re)start your npm dev server as normal.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TEST PLAN

ADDITIONAL INFORMATION

• Has associated issue:
• Changes UI
• Requires DB Migration.
• Confirm DB Migration upgrade and downgrade tested.
• Introduces new feature or API
• Removes existing feature or API

REVIEWERS

@suddjian @kristw It sure seems like this ALMOST works, but is apparently beyond my webpack knowhow. If you think we can get webpack loaders to somehow process the contents of the /src folders of plugins, this might make things a LOT easier than dealing with npm link.

[kristw]

After seeing the PR, I added some more thoughts regarding dynamic plugins in SIP-38
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

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 plugin uses dynamic import for the transformProps and will only load it when it is used.      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.

export default class BarChartPlugin extends ChartPlugin {
  constructor() {
    super({
      // metadata does not use dynamic import since it is so small.
      metadata: createMetadata(),
      // this plugin does not use dynamic import for the Chart, 
      // which could be because it is small and does not require additional dependencies.
      Chart: BarChart,
      // this plugin does not use dynamic import for the transformProps function
      // which could be because it is small and does not require additional dependencies.
      transformProps,
      // this plugin 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) The current architecture requires plugin registration to 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

  register() {
    const { key = isRequired('config.key') } = this.config;
    getChartMetadataRegistry().registerValue(key, this.metadata);
    getChartControlPanelRegistry().registerValue(key, this.controlPanel);
    // notice that these register loader functions, not the actual code
    getChartComponentRegistry().registerLoader(key, this.loadChart);
    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

const metadata = getChartMetadataRegistry().get(vizType);
if (metadata.useLegacyApi) { ... }

None of this will work correctly if the plugin registrations are asynchronous, because the code mentioned above may be called before the dynamic import in this PR returns and enter the then clause.

Example broken behavior will be

• Some charts are broken because the app does not know the vizType (yet).
• Some of the visualization are displayed in the dialog box, some are not.

[rusackas]

Points taken, and thank you for the insight. The approach in #9326 is fantastic for the time being. I'd hoped that this direction could be one small step toward some other means to load plugins via a manifest of sorts (config file or some other state), but that's a mission for another day.