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 2020/03/18 22:03:56 UTC
[GitHub] [incubator-superset] rusackas opened a new pull request #9324:
[WiP] - Dynamically import of viz plugins, take 1
rusackas opened a new pull request #9324: [WiP] - Dynamically import of viz plugins, take 1
URL: https://github.com/apache/incubator-superset/pull/9324
### 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 hit`npm link @superset-ui/core`
* (re)start your `npm dev` server as normal.
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
<!--- Skip this if not applicable -->
### TEST PLAN
<!--- What steps should be taken to verify the changes -->
### ADDITIONAL INFORMATION
<!--- Check any relevant boxes with "x" -->
<!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue -->
- [ ] 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
----------------------------------------------------------------
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
[GitHub] [incubator-superset] kristw edited a comment on issue #9324: [WiP]
- Dynamically import of viz plugins, take 1
Posted by 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 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.
----------------------------------------------------------------
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
[GitHub] [incubator-superset] kristw edited a comment on issue #9324: [WiP]
- Dynamically import of viz plugins, take 1
Posted by 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 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.
```ts
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) 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 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.
----------------------------------------------------------------
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
[GitHub] [incubator-superset] kristw edited a comment on issue #9324: [WiP]
- Dynamically import of viz plugins, take 1
Posted by 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
[GitHub] [incubator-superset] kristw commented on issue #9324: [WiP] -
Dynamically import of viz plugins, take 1
Posted by GitBox <gi...@apache.org>.
kristw commented 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` like
----------------------------------------------------------------
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
[GitHub] [incubator-superset] kristw edited a comment on issue #9324: [WiP]
- Dynamically import of viz plugins, take 1
Posted by 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 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.
```ts
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](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);
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`
```ts
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.
----------------------------------------------------------------
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
[GitHub] [incubator-superset] rusackas commented on issue #9324: [WiP] -
Dynamically import of viz plugins, take 1
Posted by GitBox <gi...@apache.org>.
rusackas commented on issue #9324: [WiP] - Dynamically import of viz plugins, take 1
URL: https://github.com/apache/incubator-superset/pull/9324#issuecomment-601407285
Points taken, and thank you for the insight. The approach in https://github.com/apache/incubator-superset/pull/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.
----------------------------------------------------------------
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
[GitHub] [incubator-superset] kristw edited a comment on issue #9324: [WiP]
- Dynamically import of viz plugins, take 1
Posted by 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 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.
----------------------------------------------------------------
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
[GitHub] [incubator-superset] kristw edited a comment on issue #9324: [WiP]
- Dynamically import of viz plugins, take 1
Posted by 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 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.
```ts
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) 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);
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`
```ts
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.
----------------------------------------------------------------
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
[GitHub] [incubator-superset] rusackas closed pull request #9324: [WiP] -
Dynamically import of viz plugins, take 1
Posted by GitBox <gi...@apache.org>.
rusackas closed pull request #9324: [WiP] - Dynamically import of viz plugins, take 1
URL: https://github.com/apache/incubator-superset/pull/9324
----------------------------------------------------------------
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