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/20 02:01:40 UTC
[GitHub] [incubator-superset] ktmud opened a new pull request #9333: build:
use manifest hooks for dev server proxy
ktmud opened a new pull request #9333: build: use manifest hooks for dev server proxy
URL: https://github.com/apache/incubator-superset/pull/9333
### CATEGORY
- [x] Build / Development Environment
### SUMMARY
This updates how `wepback-dev-server` proxy reads asset manifest. Instead of reading from the generated `manifest.json` file, we use webpack hooks.
This is needed because sometimes proxy server will read `manifest.json` before it is fully updated, resulting the proxy to return the old code for an updated module on a hard page refresh.
Also added some docs for how to edit plugins as a followup to https://github.com/apache/incubator-superset/pull/9326 .
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
N/A
### TEST PLAN
Before the change:
- Start the dev server, go to any page
- Edit a React component in that page, hot load should work, you component is updated
- Refresh the page, you component is reverted to the old status
After the change:
- You component should reflect the updated code.
### ADDITIONAL INFORMATION
N/A
### REVIEWERS
@rusackas @kristw
----------------------------------------------------------------
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 a change in pull request
#9333: build: use manifest hooks for dev server proxy
Posted by GitBox <gi...@apache.org>.
rusackas commented on a change in pull request #9333: build: use manifest hooks for dev server proxy
URL: https://github.com/apache/incubator-superset/pull/9333#discussion_r395441095
##########
File path: CONTRIBUTING.md
##########
@@ -803,11 +803,27 @@ Then, [extract strings for the new language](#extracting-new-strings-for-transla
This means it'll register MyDatasource and MyOtherDatasource in superset.my_models module in the source registry.
-### Creating a new visualization type
+### Improve visualizations
+
+Superset is working towards a plugin system where new visualizations can be installed as optional npm packages. To achieve this goal,
+we are not accepting pull requests for new community-contributed visualization types at the moment. However, bugfixes for current visualizations are welcome. To edit the frontend code for visualizations, you will have to check out a copy of [apache-superset/superset-ui-plugins](https://github.com/apache-superset/superset-ui-plugins):
+
+```bash
+git clone https://github.com/apache-superset/superset-ui-plugins.git
+yarn && yarn build
+```
+
+Then use `npm link` to create a symlink of the source code in `superset-frontend/node_modules`:
+
+```bash
+cd incubator-superset/superset-frontend
+npm link ../superset-ui-plugins/packages/superset-ui-[PLUGIN NAME]
+
+# Start developing
+npm run dev-server
+```
-Here's an example as a Github PR with comments that describe what the
-different sections of the code do:
-https://github.com/apache/incubator-superset/pull/3013
+Note that every time you do `npm install`, you will lose the symlink and may have to run `npm link` again.
Review comment:
```suggestion
Note that every time you do `npm install`, you will lose the symlink(s) and may have to run `npm link` again.
```
----------------------------------------------------------------
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] codecov-io edited a comment on issue #9333:
build: use manifest hooks for dev server proxy
Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on issue #9333: build: use manifest hooks for dev server proxy
URL: https://github.com/apache/incubator-superset/pull/9333#issuecomment-602775834
# [Codecov](https://codecov.io/gh/apache/incubator-superset/pull/9333?src=pr&el=h1) Report
> Merging [#9333](https://codecov.io/gh/apache/incubator-superset/pull/9333?src=pr&el=desc) into [master](https://codecov.io/gh/apache/incubator-superset/commit/f4087d2ad2801e3974ed715f32e6d17f991fcaa7&el=desc) will **decrease** coverage by `0.03%`.
> The diff coverage is `6.97%`.
[](https://codecov.io/gh/apache/incubator-superset/pull/9333?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #9333 +/- ##
==========================================
- Coverage 58.97% 58.93% -0.04%
==========================================
Files 374 374
Lines 12124 12155 +31
Branches 2987 2998 +11
==========================================
+ Hits 7150 7164 +14
- Misses 4795 4812 +17
Partials 179 179
```
| [Impacted Files](https://codecov.io/gh/apache/incubator-superset/pull/9333?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [superset-frontend/src/chart/Chart.jsx](https://codecov.io/gh/apache/incubator-superset/pull/9333/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NoYXJ0L0NoYXJ0LmpzeA==) | `10.41% <ø> (ø)` | |
| [superset-frontend/src/dashboard/App.jsx](https://codecov.io/gh/apache/incubator-superset/pull/9333/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC9BcHAuanN4) | `0.00% <0.00%> (ø)` | |
| [.../src/dashboard/components/gridComponents/Chart.jsx](https://codecov.io/gh/apache/incubator-superset/pull/9333/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC9jb21wb25lbnRzL2dyaWRDb21wb25lbnRzL0NoYXJ0LmpzeA==) | `67.41% <0.00%> (ø)` | |
| [superset-frontend/src/dashboard/index.jsx](https://codecov.io/gh/apache/incubator-superset/pull/9333/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC9pbmRleC5qc3g=) | `0.00% <0.00%> (ø)` | |
| [...nd/src/explore/components/ExploreViewContainer.jsx](https://codecov.io/gh/apache/incubator-superset/pull/9333/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2V4cGxvcmUvY29tcG9uZW50cy9FeHBsb3JlVmlld0NvbnRhaW5lci5qc3g=) | `46.47% <ø> (ø)` | |
| [...-frontend/src/visualizations/presets/MainPreset.js](https://codecov.io/gh/apache/incubator-superset/pull/9333/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL3Zpc3VhbGl6YXRpb25zL3ByZXNldHMvTWFpblByZXNldC5qcw==) | `0.00% <ø> (ø)` | |
| [superset-frontend/src/chart/ChartRenderer.jsx](https://codecov.io/gh/apache/incubator-superset/pull/9333/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NoYXJ0L0NoYXJ0UmVuZGVyZXIuanN4) | `34.88% <8.57%> (-5.42%)` | :arrow_down: |
| [...ntend/src/explore/components/AdhocFilterOption.jsx](https://codecov.io/gh/apache/incubator-superset/pull/9333/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2V4cGxvcmUvY29tcG9uZW50cy9BZGhvY0ZpbHRlck9wdGlvbi5qc3g=) | `57.69% <0.00%> (-0.65%)` | :arrow_down: |
| [superset-frontend/src/explore/controls.jsx](https://codecov.io/gh/apache/incubator-superset/pull/9333/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2V4cGxvcmUvY29udHJvbHMuanN4) | `40.14% <0.00%> (ø)` | |
| [...et-frontend/src/explore/controlPanels/EventFlow.js](https://codecov.io/gh/apache/incubator-superset/pull/9333/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2V4cGxvcmUvY29udHJvbFBhbmVscy9FdmVudEZsb3cuanM=) | `0.00% <0.00%> (ø)` | |
| ... and [4 more](https://codecov.io/gh/apache/incubator-superset/pull/9333/diff?src=pr&el=tree-more) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-superset/pull/9333?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/incubator-superset/pull/9333?src=pr&el=footer). Last update [f4087d2...5c8b973](https://codecov.io/gh/apache/incubator-superset/pull/9333?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
----------------------------------------------------------------
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] ktmud edited a comment on issue #9333: build:
use manifest hooks for dev server proxy
Posted by GitBox <gi...@apache.org>.
ktmud edited a comment on issue #9333: build: use manifest hooks for dev server proxy
URL: https://github.com/apache/incubator-superset/pull/9333#issuecomment-601933033
> but see if anyone else wants to weigh in on the bug reproduction issue before merging.
To be specific, I saw the issue (updated code getting reverted to initial build) on Chart Explore page both when I edited the source file of the symlinked Big Number plugin and that of `src/chart/Chart.jsx`.
Also still seeing hot reload not working for symlinked components and was not able to fix it. Would be great if someone can help confirm whether this is reproducible, too.
----------------------------------------------------------------
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 a change in pull request
#9333: build: use manifest hooks for dev server proxy and fix hot reload
for charts
Posted by GitBox <gi...@apache.org>.
kristw commented on a change in pull request #9333: build: use manifest hooks for dev server proxy and fix hot reload for charts
URL: https://github.com/apache/incubator-superset/pull/9333#discussion_r398833042
##########
File path: superset-frontend/src/chart/ChartRenderer.jsx
##########
@@ -195,8 +242,12 @@ class ChartRenderer extends React.Component {
? `superset-chart-${snakeCaseVizType}`
: snakeCaseVizType;
+ // Use this line to make sure charts do not render unnecessarily.
+ // console.log('>>> %s rendered', this.props.chartId);
Review comment:
remove
----------------------------------------------------------------
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 a change in pull request
#9333: build: use manifest hooks for dev server proxy and fix hot reload
for charts
Posted by GitBox <gi...@apache.org>.
kristw commented on a change in pull request #9333: build: use manifest hooks for dev server proxy and fix hot reload for charts
URL: https://github.com/apache/incubator-superset/pull/9333#discussion_r398831599
##########
File path: superset-frontend/webpack.config.js
##########
@@ -20,17 +20,17 @@
const fs = require('fs');
const path = require('path');
const webpack = require('webpack');
-const BundleAnalyzerPlugin = require('webpack-bundle-analyzer')
- .BundleAnalyzerPlugin;
+const { BundleAnalyzerPlugin } = require('webpack-bundle-analyzer');
const { CleanWebpackPlugin } = require('clean-webpack-plugin');
const CopyPlugin = require('copy-webpack-plugin');
const MiniCssExtractPlugin = require('mini-css-extract-plugin');
const OptimizeCSSAssetsPlugin = require('optimize-css-assets-webpack-plugin');
const SpeedMeasurePlugin = require('speed-measure-webpack-plugin');
const TerserPlugin = require('terser-webpack-plugin');
-const WebpackAssetsManifest = require('webpack-assets-manifest');
+const ManifestPlugin = require('webpack-manifest-plugin');
Review comment:
sgtm
----------------------------------------------------------------
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] ktmud commented on a change in pull request
#9333: build: use manifest hooks for dev server proxy
Posted by GitBox <gi...@apache.org>.
ktmud commented on a change in pull request #9333: build: use manifest hooks for dev server proxy
URL: https://github.com/apache/incubator-superset/pull/9333#discussion_r395409966
##########
File path: superset-frontend/webpack.config.js
##########
@@ -20,17 +20,17 @@
const fs = require('fs');
const path = require('path');
const webpack = require('webpack');
-const BundleAnalyzerPlugin = require('webpack-bundle-analyzer')
- .BundleAnalyzerPlugin;
+const { BundleAnalyzerPlugin } = require('webpack-bundle-analyzer');
const { CleanWebpackPlugin } = require('clean-webpack-plugin');
const CopyPlugin = require('copy-webpack-plugin');
const MiniCssExtractPlugin = require('mini-css-extract-plugin');
const OptimizeCSSAssetsPlugin = require('optimize-css-assets-webpack-plugin');
const SpeedMeasurePlugin = require('speed-measure-webpack-plugin');
const TerserPlugin = require('terser-webpack-plugin');
-const WebpackAssetsManifest = require('webpack-assets-manifest');
+const ManifestPlugin = require('webpack-manifest-plugin');
Review comment:
Replaced [WebpackAssetsManafestPlugin](https://github.com/webdeveric/webpack-assets-manifest) with [WebpackManifestPlugin](https://github.com/danethurber/webpack-manifest-plugin) because the latter is recommended by Webpack's official docs and seem to have more users.
----------------------------------------------------------------
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] ktmud commented on a change in pull request
#9333: build: use manifest hooks for dev server proxy
Posted by GitBox <gi...@apache.org>.
ktmud commented on a change in pull request #9333: build: use manifest hooks for dev server proxy
URL: https://github.com/apache/incubator-superset/pull/9333#discussion_r395776246
##########
File path: superset-frontend/webpack.config.js
##########
@@ -60,14 +60,39 @@ if (isDevMode) {
const plugins = [
// creates a manifest.json mapping of name to hashed output used in template files
- new WebpackAssetsManifest({
- publicPath: true,
+ new ManifestPlugin({
+ publicPath: output.publicPath,
+ seed: { app: 'superset' },
// This enables us to include all relevant files for an entry
- entrypoints: true,
+ generate: (seed, files, entrypoints) => {
+ // Each entrypoint's chunk files in the format of
+ // {
+ // entry: {
+ // css: [],
+ // js: []
+ // }
+ // }
+ const entryFiles = {};
+ for (const [entry, chunks] of Object.entries(entrypoints)) {
+ entryFiles[entry] = {
+ css: chunks
+ .filter(x => x.endsWith('.css'))
+ .map(x => path.join(output.publicPath, x)),
+ js: chunks
+ .filter(x => x.endsWith('.js'))
+ .map(x => path.join(output.publicPath, x)),
+ };
+ }
+ return {
+ ...seed,
+ // files: filePaths,
+ entrypoints: entryFiles,
+ };
Review comment:
Note that the new `manifest.json` will now not emit file entries at the root, e.g.:
```json
{
"filename.js": "filename.[hash].js"
}
```
It will now only contain an `entrypoints` section (the only thing that's been used by both Flask and `webpack-dev-server`), a list of CSS/JS chunk files for each bundle.
----------------------------------------------------------------
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 a change in pull request
#9333: build: use manifest hooks for dev server proxy and fix hot reload
for charts
Posted by GitBox <gi...@apache.org>.
kristw commented on a change in pull request #9333: build: use manifest hooks for dev server proxy and fix hot reload for charts
URL: https://github.com/apache/incubator-superset/pull/9333#discussion_r398831365
##########
File path: superset-frontend/src/dashboard/index.jsx
##########
@@ -18,6 +18,25 @@
*/
import React from 'react';
import ReactDOM from 'react-dom';
+import thunk from 'redux-thunk';
+import { createStore, applyMiddleware, compose } from 'redux';
+import { initFeatureFlags } from 'src/featureFlags';
+import { initEnhancer } from '../reduxUtils';
+import getInitialState from './reducers/getInitialState';
+import rootReducer from './reducers/index';
+import logger from '../middleware/loggerMiddleware';
+
import App from './App';
-ReactDOM.render(<App />, document.getElementById('app'));
+const appContainer = document.getElementById('app');
+const bootstrapData = JSON.parse(appContainer.getAttribute('data-bootstrap'));
+initFeatureFlags(bootstrapData.common.feature_flags);
+const initState = getInitialState(bootstrapData);
+
+const store = createStore(
+ rootReducer,
+ initState,
+ compose(applyMiddleware(thunk, logger), initEnhancer(false)),
+);
+
+ReactDOM.render(<App store={store} />, document.getElementById('app'));
Review comment:
oh nice. so this will get rid of the hot loader warning about store, right?
----------------------------------------------------------------
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 a change in pull request
#9333: build: use manifest hooks for dev server proxy and fix hot reload
for charts
Posted by GitBox <gi...@apache.org>.
kristw commented on a change in pull request #9333: build: use manifest hooks for dev server proxy and fix hot reload for charts
URL: https://github.com/apache/incubator-superset/pull/9333#discussion_r398919043
##########
File path: superset-frontend/src/dashboard/App.jsx
##########
@@ -16,36 +16,19 @@
* specific language governing permissions and limitations
* under the License.
*/
+import { hot } from 'react-hot-loader/root';
+import { setConfig } from 'react-hot-loader';
Review comment:
This seems to be unused import.
----------------------------------------------------------------
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 a change in pull request
#9333: build: use manifest hooks for dev server proxy and fix hot reload
for charts
Posted by GitBox <gi...@apache.org>.
kristw commented on a change in pull request #9333: build: use manifest hooks for dev server proxy and fix hot reload for charts
URL: https://github.com/apache/incubator-superset/pull/9333#discussion_r398831846
##########
File path: superset-frontend/webpack.config.js
##########
@@ -60,14 +60,39 @@ if (isDevMode) {
const plugins = [
// creates a manifest.json mapping of name to hashed output used in template files
- new WebpackAssetsManifest({
- publicPath: true,
+ new ManifestPlugin({
+ publicPath: output.publicPath,
+ seed: { app: 'superset' },
// This enables us to include all relevant files for an entry
- entrypoints: true,
+ generate: (seed, files, entrypoints) => {
+ // Each entrypoint's chunk files in the format of
+ // {
+ // entry: {
+ // css: [],
+ // js: []
+ // }
+ // }
+ const entryFiles = {};
+ for (const [entry, chunks] of Object.entries(entrypoints)) {
+ entryFiles[entry] = {
+ css: chunks
+ .filter(x => x.endsWith('.css'))
+ .map(x => path.join(output.publicPath, x)),
+ js: chunks
+ .filter(x => x.endsWith('.js'))
+ .map(x => path.join(output.publicPath, x)),
+ };
+ }
+ return {
+ ...seed,
+ // files: filePaths,
Review comment:
keep or remove?
----------------------------------------------------------------
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] ktmud commented on issue #9333: build: use
manifest hooks for dev server proxy
Posted by GitBox <gi...@apache.org>.
ktmud commented on issue #9333: build: use manifest hooks for dev server proxy
URL: https://github.com/apache/incubator-superset/pull/9333#issuecomment-602134435
FINALLY managed to fix hot-reload issue on the Explore page. The problem is multifold.
1. `shouldComponentUpdate` is probably too strict. I [changed it](https://github.com/apache/incubator-superset/pull/9333/commits/7589b46bf127979b2dc24e518d94ecc5d4e52a6d#diff-b8d51b9ed2063863add2f7509be1b719R91) to only return false when `chartStatus` is updated from `success` to `rendered`.
2. Because `SuperChart` uses [memoized async loaders](https://github.com/apache-superset/superset-ui/blob/26ed9e31c6fd8950b01f8170836695cc9823192a/packages/superset-ui-chart/src/components/SuperChartCore.tsx#L93-L135) to load chart component, sometimes it confuses `react-hot-loader` when the module should be reloaded. The hack is to [add a unique key](https://github.com/apache/incubator-superset/pull/9333/commits/7589b46bf127979b2dc24e518d94ecc5d4e52a6d#diff-b8d51b9ed2063863add2f7509be1b719R233) to `SuperChart` component in `ChartRenderer` so it will always be rerendered.
Unfortunately, sometimes hot reload still doesn't work for charts in the dashboards. Considering the Explore page is normally where developers test + develop the charts, and they can always do a "cold" refresh, I'll stop digging and leave it as is. I did, however, [rewrite](https://github.com/apache/incubator-superset/pull/9333/commits/d5c8fd1caf482ea7ebd6f53bc00152cddb83fd99) `dashboard/App.jsx` and `dashboard/index.jsx` to avoid the same "<Provider> does not support change store on the fly" error as seen [in the Explore page](https://github.com/apache/incubator-superset/pull/9326#discussion_r394777930).
----------------------------------------------------------------
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] ktmud commented on a change in pull request
#9333: build: use manifest hooks for dev server proxy
Posted by GitBox <gi...@apache.org>.
ktmud commented on a change in pull request #9333: build: use manifest hooks for dev server proxy
URL: https://github.com/apache/incubator-superset/pull/9333#discussion_r396948951
##########
File path: superset-frontend/src/chart/ChartRenderer.jsx
##########
@@ -78,28 +119,35 @@ class ChartRenderer extends React.Component {
}
shouldComponentUpdate(nextProps) {
- const resultsReady =
- nextProps.queryResponse &&
- ['success', 'rendered'].indexOf(nextProps.chartStatus) > -1 &&
- !nextProps.queryResponse.error &&
- !nextProps.refreshOverlayVisible;
-
- if (resultsReady) {
- this.hasQueryResponseChange =
- nextProps.queryResponse !== this.props.queryResponse;
-
- if (
- this.hasQueryResponseChange ||
- nextProps.annotationData !== this.props.annotationData ||
- nextProps.height !== this.props.height ||
- nextProps.width !== this.props.width ||
- nextProps.triggerRender ||
- nextProps.formData.color_scheme !== this.props.formData.color_scheme
- ) {
- return true;
- }
+ this.hasQueryResponseChange =
+ this.props.queryResponse !== nextProps.queryResponse;
+
+ // if no results loaded, don't render
+ if (
+ !nextProps.queryResponse ||
+ nextProps.queryResponse.error ||
+ nextProps.refreshOverlayVisible
+ )
+ return false;
+
+ // current chart status
+ const chartStatus = this.props.chartStatus;
+ // `rendered` status is set right after `success` or `loading`,
+ // don't trigger rerender here (see `actions.chartRenderingSucceeded`).
+ // note that even though render is not triggered, the props will still be
+ // updated.
Review comment:
Not sure what we are using the `rendered` status for, but I'm keeping it here just in case.
----------------------------------------------------------------
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] ktmud commented on a change in pull request
#9333: build: use manifest hooks for dev server proxy and fix hot reload
for charts
Posted by GitBox <gi...@apache.org>.
ktmud commented on a change in pull request #9333: build: use manifest hooks for dev server proxy and fix hot reload for charts
URL: https://github.com/apache/incubator-superset/pull/9333#discussion_r398863977
##########
File path: superset-frontend/src/chart/ChartRenderer.jsx
##########
@@ -58,6 +59,46 @@ const defaultProps = {
triggerRender: false,
};
+const isDevMode = process.env.WEBPACK_MODE === 'development';
+if (isDevMode) {
+ setConfig({ logLevel: 'debug', trackTailUpdates: false });
Review comment:
I noticed this config is only needed for charts and `isDevMode` is used somewhere down below as well, so it was just convenient to put it here. Will move it to `preamble.js`.
----------------------------------------------------------------
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] codecov-io edited a comment on issue #9333:
build: use manifest hooks for dev server proxy
Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on issue #9333: build: use manifest hooks for dev server proxy
URL: https://github.com/apache/incubator-superset/pull/9333#issuecomment-602775834
# [Codecov](https://codecov.io/gh/apache/incubator-superset/pull/9333?src=pr&el=h1) Report
> Merging [#9333](https://codecov.io/gh/apache/incubator-superset/pull/9333?src=pr&el=desc) into [master](https://codecov.io/gh/apache/incubator-superset/commit/5e6662ab12e4b5d3c392e8cc1bfd37afc5b20b63&el=desc) will **increase** coverage by `0.04%`.
> The diff coverage is `15.78%`.
[](https://codecov.io/gh/apache/incubator-superset/pull/9333?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #9333 +/- ##
==========================================
+ Coverage 58.96% 59.00% +0.04%
==========================================
Files 374 374
Lines 12137 12133 -4
Branches 2992 2985 -7
==========================================
+ Hits 7156 7159 +3
+ Misses 4802 4795 -7
Partials 179 179
```
| [Impacted Files](https://codecov.io/gh/apache/incubator-superset/pull/9333?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [superset-frontend/src/chart/Chart.jsx](https://codecov.io/gh/apache/incubator-superset/pull/9333/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NoYXJ0L0NoYXJ0LmpzeA==) | `10.41% <ø> (ø)` | |
| [superset-frontend/src/dashboard/App.jsx](https://codecov.io/gh/apache/incubator-superset/pull/9333/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC9BcHAuanN4) | `0.00% <0.00%> (ø)` | |
| [superset-frontend/src/dashboard/index.jsx](https://codecov.io/gh/apache/incubator-superset/pull/9333/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC9pbmRleC5qc3g=) | `0.00% <0.00%> (ø)` | |
| [...-frontend/src/visualizations/presets/MainPreset.js](https://codecov.io/gh/apache/incubator-superset/pull/9333/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL3Zpc3VhbGl6YXRpb25zL3ByZXNldHMvTWFpblByZXNldC5qcw==) | `0.00% <ø> (ø)` | |
| [superset-frontend/src/chart/ChartRenderer.jsx](https://codecov.io/gh/apache/incubator-superset/pull/9333/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NoYXJ0L0NoYXJ0UmVuZGVyZXIuanN4) | `47.36% <25.00%> (+6.11%)` | :arrow_up: |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-superset/pull/9333?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/incubator-superset/pull/9333?src=pr&el=footer). Last update [5e6662a...014ff1b](https://codecov.io/gh/apache/incubator-superset/pull/9333?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
----------------------------------------------------------------
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 a change in pull request
#9333: build: use manifest hooks for dev server proxy and fix hot reload
for charts
Posted by GitBox <gi...@apache.org>.
kristw commented on a change in pull request #9333: build: use manifest hooks for dev server proxy and fix hot reload for charts
URL: https://github.com/apache/incubator-superset/pull/9333#discussion_r398843163
##########
File path: superset-frontend/src/chart/Chart.jsx
##########
@@ -74,6 +74,7 @@ const defaultProps = {
setControlValue() {},
triggerRender: false,
dashboardId: null,
+ chartStackTrace: null,
Review comment:
Is this change intended? I didn't see any reference within this PR.
----------------------------------------------------------------
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 a change in pull request
#9333: build: use manifest hooks for dev server proxy
Posted by GitBox <gi...@apache.org>.
rusackas commented on a change in pull request #9333: build: use manifest hooks for dev server proxy
URL: https://github.com/apache/incubator-superset/pull/9333#discussion_r395439719
##########
File path: CONTRIBUTING.md
##########
@@ -75,7 +75,7 @@ little bit helps, and credit will always be given.
- [Creating a new language dictionary](#creating-a-new-language-dictionary)
- [Tips](#tips)
- [Adding a new datasource](#adding-a-new-datasource)
- - [Creating a new visualization type](#creating-a-new-visualization-type)
+ - [Improve visualizations](#improve-visualizations)
Review comment:
Tiny grammar nit :)
```suggestion
- [Improving visualizations](#9234 -visualizations)
```
----------------------------------------------------------------
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] ktmud commented on a change in pull request
#9333: build: use manifest hooks for dev server proxy and fix hot reload
for charts
Posted by GitBox <gi...@apache.org>.
ktmud commented on a change in pull request #9333: build: use manifest hooks for dev server proxy and fix hot reload for charts
URL: https://github.com/apache/incubator-superset/pull/9333#discussion_r398855018
##########
File path: superset-frontend/src/chart/ChartRenderer.jsx
##########
@@ -78,28 +119,35 @@ class ChartRenderer extends React.Component {
}
shouldComponentUpdate(nextProps) {
- const resultsReady =
- nextProps.queryResponse &&
- ['success', 'rendered'].indexOf(nextProps.chartStatus) > -1 &&
- !nextProps.queryResponse.error &&
- !nextProps.refreshOverlayVisible;
-
- if (resultsReady) {
- this.hasQueryResponseChange =
- nextProps.queryResponse !== this.props.queryResponse;
-
- if (
- this.hasQueryResponseChange ||
- nextProps.annotationData !== this.props.annotationData ||
- nextProps.height !== this.props.height ||
- nextProps.width !== this.props.width ||
- nextProps.triggerRender ||
- nextProps.formData.color_scheme !== this.props.formData.color_scheme
- ) {
- return true;
- }
+ // if no results loaded, don't render
+ if (
+ !nextProps.queryResponse ||
+ nextProps.queryResponse.error ||
+ nextProps.refreshOverlayVisible
+ )
+ return false;
Review comment:
I think both are acceptable. Would actually prefer no `{}` if the conditions are simpler.
----------------------------------------------------------------
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 a change in pull request
#9333: build: use manifest hooks for dev server proxy
Posted by GitBox <gi...@apache.org>.
rusackas commented on a change in pull request #9333: build: use manifest hooks for dev server proxy
URL: https://github.com/apache/incubator-superset/pull/9333#discussion_r395441428
##########
File path: CONTRIBUTING.md
##########
@@ -803,11 +803,27 @@ Then, [extract strings for the new language](#extracting-new-strings-for-transla
This means it'll register MyDatasource and MyOtherDatasource in superset.my_models module in the source registry.
-### Creating a new visualization type
+### Improve visualizations
+
+Superset is working towards a plugin system where new visualizations can be installed as optional npm packages. To achieve this goal,
+we are not accepting pull requests for new community-contributed visualization types at the moment. However, bugfixes for current visualizations are welcome. To edit the frontend code for visualizations, you will have to check out a copy of [apache-superset/superset-ui-plugins](https://github.com/apache-superset/superset-ui-plugins):
+
+```bash
+git clone https://github.com/apache-superset/superset-ui-plugins.git
+yarn && yarn build
+```
+
+Then use `npm link` to create a symlink of the source code in `superset-frontend/node_modules`:
+
+```bash
+cd incubator-superset/superset-frontend
+npm link ../superset-ui-plugins/packages/superset-ui-[PLUGIN NAME]
+
+# Start developing
+npm run dev-server
+```
Review comment:
Thought this might also be worth mentioning...
```suggestion
Note that when plugin packages are linked with `npm link` they will automatically load files from the plugin's `/src` directory.
```
----------------------------------------------------------------
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] ktmud commented on a change in pull request
#9333: build: use manifest hooks for dev server proxy
Posted by GitBox <gi...@apache.org>.
ktmud commented on a change in pull request #9333: build: use manifest hooks for dev server proxy
URL: https://github.com/apache/incubator-superset/pull/9333#discussion_r396947315
##########
File path: superset-frontend/src/chart/ChartRenderer.jsx
##########
@@ -58,6 +59,46 @@ const defaultProps = {
triggerRender: false,
};
+const isDevMode = process.env.WEBPACK_MODE === 'development';
+if (isDevMode) {
+ setConfig({ logLevel: 'debug', trackTailUpdates: false });
+}
+
+/**
+ * Compare two objects shallowly, but ignore some keys
+ */
+function deepEqual(obj1, obj2, maxDepth = Infinity, depth = 0) {
+ if (
+ // don't go too deep
+ depth > maxDepth ||
+ // nulls
+ obj1 === null ||
+ obj2 === null ||
+ // primatives
+ typeof obj1 !== 'object' ||
+ typeof obj2 !== 'object'
+ ) {
+ return obj1 === obj2;
+ }
+
+ // arrays
+ if (Array.isArray(obj1) && Array.isArray(obj2)) {
+ return (
+ obj1.length === obj2.length &&
+ obj1.every((val, i) => deepEqual(val, obj2[i], maxDepth, depth + 1))
+ );
+ }
+
+ // objects
+ for (const [key, val] of Object.entries(obj1)) {
+ if (!deepEqual(obj2[key], val, maxDepth, depth + 1)) {
+ // console.log('>> Diff "%s":', key, depth, val, obj2[key]);
+ return false;
+ }
+ }
+ return true;
+}
+
Review comment:
I wrote my own depth-limited `deepEqual` function. Would be happy to use any existing library or util function if it exists.
----------------------------------------------------------------
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 a change in pull request
#9333: build: use manifest hooks for dev server proxy
Posted by GitBox <gi...@apache.org>.
rusackas commented on a change in pull request #9333: build: use manifest hooks for dev server proxy
URL: https://github.com/apache/incubator-superset/pull/9333#discussion_r395439327
##########
File path: superset-frontend/src/visualizations/presets/MainPreset.js
##########
@@ -60,7 +60,7 @@ import {
LineMultiChartPlugin,
PieChartPlugin,
TimePivotChartPlugin,
-} from '@superset-ui/legacy-preset-chart-nvd3/lib';
+} from '@superset-ui/legacy-preset-chart-nvd3';
Review comment:
🎉
----------------------------------------------------------------
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 a change in pull request
#9333: build: use manifest hooks for dev server proxy and fix hot reload
for charts
Posted by GitBox <gi...@apache.org>.
kristw commented on a change in pull request #9333: build: use manifest hooks for dev server proxy and fix hot reload for charts
URL: https://github.com/apache/incubator-superset/pull/9333#discussion_r398843247
##########
File path: superset-frontend/src/chart/ChartRenderer.jsx
##########
@@ -78,28 +119,35 @@ class ChartRenderer extends React.Component {
}
shouldComponentUpdate(nextProps) {
- const resultsReady =
- nextProps.queryResponse &&
- ['success', 'rendered'].indexOf(nextProps.chartStatus) > -1 &&
- !nextProps.queryResponse.error &&
- !nextProps.refreshOverlayVisible;
-
- if (resultsReady) {
- this.hasQueryResponseChange =
- nextProps.queryResponse !== this.props.queryResponse;
-
- if (
- this.hasQueryResponseChange ||
- nextProps.annotationData !== this.props.annotationData ||
- nextProps.height !== this.props.height ||
- nextProps.width !== this.props.width ||
- nextProps.triggerRender ||
- nextProps.formData.color_scheme !== this.props.formData.color_scheme
- ) {
- return true;
- }
+ this.hasQueryResponseChange =
+ this.props.queryResponse !== nextProps.queryResponse;
+
+ // if no results loaded, don't render
+ if (
+ !nextProps.queryResponse ||
+ nextProps.queryResponse.error ||
+ nextProps.refreshOverlayVisible
+ )
+ return false;
+
+ // current chart status
+ const chartStatus = this.props.chartStatus;
+ // `rendered` status is set right after `success` or `loading`,
+ // don't trigger rerender here (see `actions.chartRenderingSucceeded`).
+ // note that even though render is not triggered, the props will still be
+ // updated.
Review comment:
The logic seems to be changed here, since the original code did not check `this.props.chartStatus`.
----------------------------------------------------------------
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 a change in pull request
#9333: build: use manifest hooks for dev server proxy and fix hot reload
for charts
Posted by GitBox <gi...@apache.org>.
kristw commented on a change in pull request #9333: build: use manifest hooks for dev server proxy and fix hot reload for charts
URL: https://github.com/apache/incubator-superset/pull/9333#discussion_r398840169
##########
File path: superset-frontend/src/chart/ChartRenderer.jsx
##########
@@ -78,28 +119,35 @@ class ChartRenderer extends React.Component {
}
shouldComponentUpdate(nextProps) {
- const resultsReady =
- nextProps.queryResponse &&
- ['success', 'rendered'].indexOf(nextProps.chartStatus) > -1 &&
- !nextProps.queryResponse.error &&
- !nextProps.refreshOverlayVisible;
-
- if (resultsReady) {
- this.hasQueryResponseChange =
- nextProps.queryResponse !== this.props.queryResponse;
-
- if (
- this.hasQueryResponseChange ||
- nextProps.annotationData !== this.props.annotationData ||
- nextProps.height !== this.props.height ||
- nextProps.width !== this.props.width ||
- nextProps.triggerRender ||
- nextProps.formData.color_scheme !== this.props.formData.color_scheme
- ) {
- return true;
- }
+ // if no results loaded, don't render
+ if (
+ !nextProps.queryResponse ||
+ nextProps.queryResponse.error ||
+ nextProps.refreshOverlayVisible
+ )
+ return false;
Review comment:
add `{}`. Not sure why `eslint` is not warning.
----------------------------------------------------------------
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 a change in pull request
#9333: build: use manifest hooks for dev server proxy and fix hot reload
for charts
Posted by GitBox <gi...@apache.org>.
kristw commented on a change in pull request #9333: build: use manifest hooks for dev server proxy and fix hot reload for charts
URL: https://github.com/apache/incubator-superset/pull/9333#discussion_r398844130
##########
File path: superset-frontend/src/chart/ChartRenderer.jsx
##########
@@ -58,6 +59,46 @@ const defaultProps = {
triggerRender: false,
};
+const isDevMode = process.env.WEBPACK_MODE === 'development';
+if (isDevMode) {
+ setConfig({ logLevel: 'debug', trackTailUpdates: false });
Review comment:
Could we make this call in one of the setup file instead? It seems a bit odd to call this here.
----------------------------------------------------------------
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] codecov-io commented on issue #9333: build:
use manifest hooks for dev server proxy
Posted by GitBox <gi...@apache.org>.
codecov-io commented on issue #9333: build: use manifest hooks for dev server proxy
URL: https://github.com/apache/incubator-superset/pull/9333#issuecomment-602775834
# [Codecov](https://codecov.io/gh/apache/incubator-superset/pull/9333?src=pr&el=h1) Report
> Merging [#9333](https://codecov.io/gh/apache/incubator-superset/pull/9333?src=pr&el=desc) into [master](https://codecov.io/gh/apache/incubator-superset/commit/c34df6b7b356fc28e3c3a80a975ecdf2de7c0681&el=desc) will **increase** coverage by `0.04%`.
> The diff coverage is `15.78%`.
[](https://codecov.io/gh/apache/incubator-superset/pull/9333?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #9333 +/- ##
==========================================
+ Coverage 58.96% 59.00% +0.04%
==========================================
Files 374 374
Lines 12137 12133 -4
Branches 2992 2985 -7
==========================================
+ Hits 7156 7159 +3
+ Misses 4802 4795 -7
Partials 179 179
```
| [Impacted Files](https://codecov.io/gh/apache/incubator-superset/pull/9333?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [superset-frontend/src/chart/Chart.jsx](https://codecov.io/gh/apache/incubator-superset/pull/9333/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NoYXJ0L0NoYXJ0LmpzeA==) | `10.41% <ø> (ø)` | |
| [superset-frontend/src/dashboard/App.jsx](https://codecov.io/gh/apache/incubator-superset/pull/9333/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC9BcHAuanN4) | `0.00% <0.00%> (ø)` | |
| [superset-frontend/src/dashboard/index.jsx](https://codecov.io/gh/apache/incubator-superset/pull/9333/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC9pbmRleC5qc3g=) | `0.00% <0.00%> (ø)` | |
| [...-frontend/src/visualizations/presets/MainPreset.js](https://codecov.io/gh/apache/incubator-superset/pull/9333/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL3Zpc3VhbGl6YXRpb25zL3ByZXNldHMvTWFpblByZXNldC5qcw==) | `0.00% <ø> (ø)` | |
| [superset-frontend/src/chart/ChartRenderer.jsx](https://codecov.io/gh/apache/incubator-superset/pull/9333/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NoYXJ0L0NoYXJ0UmVuZGVyZXIuanN4) | `47.36% <25.00%> (+6.11%)` | :arrow_up: |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-superset/pull/9333?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/incubator-superset/pull/9333?src=pr&el=footer). Last update [c34df6b...7589b46](https://codecov.io/gh/apache/incubator-superset/pull/9333?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
----------------------------------------------------------------
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 a change in pull request
#9333: build: use manifest hooks for dev server proxy
Posted by GitBox <gi...@apache.org>.
rusackas commented on a change in pull request #9333: build: use manifest hooks for dev server proxy
URL: https://github.com/apache/incubator-superset/pull/9333#discussion_r395439827
##########
File path: CONTRIBUTING.md
##########
@@ -803,11 +803,27 @@ Then, [extract strings for the new language](#extracting-new-strings-for-transla
This means it'll register MyDatasource and MyOtherDatasource in superset.my_models module in the source registry.
-### Creating a new visualization type
+### Improve visualizations
Review comment:
Same teensy tiny grammar nit from above :)
```suggestion
### Improving visualizations
```
----------------------------------------------------------------
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] ktmud commented on a change in pull request
#9333: build: use manifest hooks for dev server proxy and fix hot reload
for charts
Posted by GitBox <gi...@apache.org>.
ktmud commented on a change in pull request #9333: build: use manifest hooks for dev server proxy and fix hot reload for charts
URL: https://github.com/apache/incubator-superset/pull/9333#discussion_r398943780
##########
File path: superset-frontend/src/dashboard/App.jsx
##########
@@ -16,36 +16,19 @@
* specific language governing permissions and limitations
* under the License.
*/
+import { hot } from 'react-hot-loader/root';
+import { setConfig } from 'react-hot-loader';
Review comment:
Cleaned up!
----------------------------------------------------------------
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] ktmud commented on a change in pull request
#9333: build: use manifest hooks for dev server proxy
Posted by GitBox <gi...@apache.org>.
ktmud commented on a change in pull request #9333: build: use manifest hooks for dev server proxy
URL: https://github.com/apache/incubator-superset/pull/9333#discussion_r396045851
##########
File path: superset-frontend/src/chart/ChartRenderer.jsx
##########
@@ -59,6 +60,11 @@ const defaultProps = {
triggerRender: false,
};
+const isDevMode = process.env.WEBPACK_MODE === 'development';
+if (isDevMode) {
+ setConfig({ logLevel: 'debug', trackTailUpdates: false });
+}
Review comment:
This does not change what users saw, but does avoid rendering the chart two times at hot reload. See: https://github.com/gaearon/react-hot-loader#out-of-bound-warning
----------------------------------------------------------------
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] ktmud commented on a change in pull request
#9333: build: use manifest hooks for dev server proxy
Posted by GitBox <gi...@apache.org>.
ktmud commented on a change in pull request #9333: build: use manifest hooks for dev server proxy
URL: https://github.com/apache/incubator-superset/pull/9333#discussion_r395776246
##########
File path: superset-frontend/webpack.config.js
##########
@@ -60,14 +60,39 @@ if (isDevMode) {
const plugins = [
// creates a manifest.json mapping of name to hashed output used in template files
- new WebpackAssetsManifest({
- publicPath: true,
+ new ManifestPlugin({
+ publicPath: output.publicPath,
+ seed: { app: 'superset' },
// This enables us to include all relevant files for an entry
- entrypoints: true,
+ generate: (seed, files, entrypoints) => {
+ // Each entrypoint's chunk files in the format of
+ // {
+ // entry: {
+ // css: [],
+ // js: []
+ // }
+ // }
+ const entryFiles = {};
+ for (const [entry, chunks] of Object.entries(entrypoints)) {
+ entryFiles[entry] = {
+ css: chunks
+ .filter(x => x.endsWith('.css'))
+ .map(x => path.join(output.publicPath, x)),
+ js: chunks
+ .filter(x => x.endsWith('.js'))
+ .map(x => path.join(output.publicPath, x)),
+ };
+ }
+ return {
+ ...seed,
+ // files: filePaths,
+ entrypoints: entryFiles,
+ };
Review comment:
Note that the new `manifest.json` will now not emit file entries at the root, e.g.:
```json
{
"filename.js": "filename.[hash].js"
}
```
It will not only contain an `entrypoints` section (the only thing that's been used by both Flask and `webpack-dev-server`), a list of CSS/JS chunk files for each bundle.
----------------------------------------------------------------
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] ktmud commented on issue #9333: build: use
manifest hooks for dev server proxy
Posted by GitBox <gi...@apache.org>.
ktmud commented on issue #9333: build: use manifest hooks for dev server proxy
URL: https://github.com/apache/incubator-superset/pull/9333#issuecomment-602863955
@rusackas this CI is now green. Can you take another look?
----------------------------------------------------------------
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 a change in pull request
#9333: build: use manifest hooks for dev server proxy and fix hot reload
for charts
Posted by GitBox <gi...@apache.org>.
kristw commented on a change in pull request #9333: build: use manifest hooks for dev server proxy and fix hot reload for charts
URL: https://github.com/apache/incubator-superset/pull/9333#discussion_r398836358
##########
File path: superset-frontend/src/chart/ChartRenderer.jsx
##########
@@ -78,28 +119,35 @@ class ChartRenderer extends React.Component {
}
shouldComponentUpdate(nextProps) {
- const resultsReady =
- nextProps.queryResponse &&
- ['success', 'rendered'].indexOf(nextProps.chartStatus) > -1 &&
- !nextProps.queryResponse.error &&
- !nextProps.refreshOverlayVisible;
-
- if (resultsReady) {
- this.hasQueryResponseChange =
- nextProps.queryResponse !== this.props.queryResponse;
-
- if (
- this.hasQueryResponseChange ||
- nextProps.annotationData !== this.props.annotationData ||
- nextProps.height !== this.props.height ||
- nextProps.width !== this.props.width ||
- nextProps.triggerRender ||
- nextProps.formData.color_scheme !== this.props.formData.color_scheme
- ) {
- return true;
- }
+ // if no results loaded, don't render
+ if (
+ !nextProps.queryResponse ||
+ nextProps.queryResponse.error ||
+ nextProps.refreshOverlayVisible
+ )
+ return false;
+
+ this.hasQueryResponseChange =
+ this.props.queryResponse !== nextProps.queryResponse;
+
+ // current chart status
+ const chartStatus = this.props.chartStatus;
Review comment:
```ts
const { chartStatus } = this.props;
```
----------------------------------------------------------------
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 a change in pull request
#9333: build: use manifest hooks for dev server proxy and fix hot reload
for charts
Posted by GitBox <gi...@apache.org>.
kristw commented on a change in pull request #9333: build: use manifest hooks for dev server proxy and fix hot reload for charts
URL: https://github.com/apache/incubator-superset/pull/9333#discussion_r398811952
##########
File path: superset-frontend/src/dashboard/components/gridComponents/Chart.jsx
##########
@@ -133,7 +133,8 @@ class Chart extends React.Component {
}
}
- return false;
+ // `cacheBusterProp` is jnjected by react-hot-loader
Review comment:
typo `injected`
----------------------------------------------------------------
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] ktmud commented on issue #9333: build: use
manifest hooks for dev server proxy
Posted by GitBox <gi...@apache.org>.
ktmud commented on issue #9333: build: use manifest hooks for dev server proxy
URL: https://github.com/apache/incubator-superset/pull/9333#issuecomment-601933033
> but see if anyone else wants to weigh in on the bug reproduction issue before merging.
To be specific, I saw the issue (updated code getting reverted to initial build) on Chart Explore page when I edited both the source file of the symlinked Big Number plugin and `src/chart/Chart.jsx`.
Also still seeing hot reload not working for symlinked components and was not able to fix it. Would be great if someone can help confirm whether this is reproducible, too.
----------------------------------------------------------------
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 #9333: build: use
manifest hooks for dev server proxy
Posted by GitBox <gi...@apache.org>.
rusackas commented on issue #9333: build: use manifest hooks for dev server proxy
URL: https://github.com/apache/incubator-superset/pull/9333#issuecomment-602764638
Not sure why the PR page here shows Travis build is pending (seemingly forever) but if I click the "Details" link, it looks like it all worked great. Tested this locally, and happy to merge when the light turns green.
----------------------------------------------------------------
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] ktmud commented on a change in pull request
#9333: build: use manifest hooks for dev server proxy and fix hot reload
for charts
Posted by GitBox <gi...@apache.org>.
ktmud commented on a change in pull request #9333: build: use manifest hooks for dev server proxy and fix hot reload for charts
URL: https://github.com/apache/incubator-superset/pull/9333#discussion_r398853996
##########
File path: superset-frontend/src/chart/Chart.jsx
##########
@@ -74,6 +74,7 @@ const defaultProps = {
setControlValue() {},
triggerRender: false,
dashboardId: null,
+ chartStackTrace: null,
Review comment:
Yes. I was seeing `chartStackTrace` being `undefined` on the initial rendering, then immediately updated to `null`. Thought it makes sense to have a consistent value.
----------------------------------------------------------------
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] codecov-io edited a comment on issue #9333:
build: use manifest hooks for dev server proxy
Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on issue #9333: build: use manifest hooks for dev server proxy
URL: https://github.com/apache/incubator-superset/pull/9333#issuecomment-602775834
# [Codecov](https://codecov.io/gh/apache/incubator-superset/pull/9333?src=pr&el=h1) Report
> Merging [#9333](https://codecov.io/gh/apache/incubator-superset/pull/9333?src=pr&el=desc) into [master](https://codecov.io/gh/apache/incubator-superset/commit/634b440c560b161e955f9590c94656785f9189c6&el=desc) will **decrease** coverage by `0.04%`.
> The diff coverage is `6.97%`.
[](https://codecov.io/gh/apache/incubator-superset/pull/9333?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #9333 +/- ##
==========================================
- Coverage 58.99% 58.95% -0.05%
==========================================
Files 375 375
Lines 12136 12155 +19
Branches 2990 3002 +12
==========================================
+ Hits 7160 7166 +6
- Misses 4797 4810 +13
Partials 179 179
```
| [Impacted Files](https://codecov.io/gh/apache/incubator-superset/pull/9333?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [superset-frontend/src/chart/Chart.jsx](https://codecov.io/gh/apache/incubator-superset/pull/9333/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NoYXJ0L0NoYXJ0LmpzeA==) | `10.41% <ø> (ø)` | |
| [superset-frontend/src/dashboard/App.jsx](https://codecov.io/gh/apache/incubator-superset/pull/9333/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC9BcHAuanN4) | `0.00% <0.00%> (ø)` | |
| [.../src/dashboard/components/gridComponents/Chart.jsx](https://codecov.io/gh/apache/incubator-superset/pull/9333/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC9jb21wb25lbnRzL2dyaWRDb21wb25lbnRzL0NoYXJ0LmpzeA==) | `67.41% <0.00%> (ø)` | |
| [superset-frontend/src/dashboard/index.jsx](https://codecov.io/gh/apache/incubator-superset/pull/9333/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC9pbmRleC5qc3g=) | `0.00% <0.00%> (ø)` | |
| [...nd/src/explore/components/ExploreViewContainer.jsx](https://codecov.io/gh/apache/incubator-superset/pull/9333/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2V4cGxvcmUvY29tcG9uZW50cy9FeHBsb3JlVmlld0NvbnRhaW5lci5qc3g=) | `46.47% <ø> (ø)` | |
| [...-frontend/src/visualizations/presets/MainPreset.js](https://codecov.io/gh/apache/incubator-superset/pull/9333/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL3Zpc3VhbGl6YXRpb25zL3ByZXNldHMvTWFpblByZXNldC5qcw==) | `0.00% <ø> (ø)` | |
| [superset-frontend/src/chart/ChartRenderer.jsx](https://codecov.io/gh/apache/incubator-superset/pull/9333/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NoYXJ0L0NoYXJ0UmVuZGVyZXIuanN4) | `34.88% <8.57%> (-5.42%)` | :arrow_down: |
| [superset-frontend/src/explore/exploreUtils.js](https://codecov.io/gh/apache/incubator-superset/pull/9333/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2V4cGxvcmUvZXhwbG9yZVV0aWxzLmpz) | `76.78% <0.00%> (-0.07%)` | :arrow_down: |
| [superset-frontend/src/SqlLab/App.jsx](https://codecov.io/gh/apache/incubator-superset/pull/9333/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi9BcHAuanN4) | `0.00% <0.00%> (ø)` | |
| [...et-frontend/src/SqlLab/reducers/getInitialState.js](https://codecov.io/gh/apache/incubator-superset/pull/9333/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi9yZWR1Y2Vycy9nZXRJbml0aWFsU3RhdGUuanM=) | `0.00% <0.00%> (ø)` | |
| ... and [2 more](https://codecov.io/gh/apache/incubator-superset/pull/9333/diff?src=pr&el=tree-more) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-superset/pull/9333?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/incubator-superset/pull/9333?src=pr&el=footer). Last update [634b440...2f19a50](https://codecov.io/gh/apache/incubator-superset/pull/9333?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
----------------------------------------------------------------
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 a change in pull request
#9333: build: use manifest hooks for dev server proxy and fix hot reload
for charts
Posted by GitBox <gi...@apache.org>.
kristw commented on a change in pull request #9333: build: use manifest hooks for dev server proxy and fix hot reload for charts
URL: https://github.com/apache/incubator-superset/pull/9333#discussion_r398793421
##########
File path: superset-frontend/src/chart/ChartRenderer.jsx
##########
@@ -78,28 +119,35 @@ class ChartRenderer extends React.Component {
}
shouldComponentUpdate(nextProps) {
- const resultsReady =
- nextProps.queryResponse &&
- ['success', 'rendered'].indexOf(nextProps.chartStatus) > -1 &&
- !nextProps.queryResponse.error &&
- !nextProps.refreshOverlayVisible;
-
- if (resultsReady) {
- this.hasQueryResponseChange =
- nextProps.queryResponse !== this.props.queryResponse;
-
- if (
- this.hasQueryResponseChange ||
- nextProps.annotationData !== this.props.annotationData ||
- nextProps.height !== this.props.height ||
- nextProps.width !== this.props.width ||
- nextProps.triggerRender ||
- nextProps.formData.color_scheme !== this.props.formData.color_scheme
- ) {
- return true;
- }
+ this.hasQueryResponseChange =
+ this.props.queryResponse !== nextProps.queryResponse;
+
+ // if no results loaded, don't render
+ if (
+ !nextProps.queryResponse ||
+ nextProps.queryResponse.error ||
+ nextProps.refreshOverlayVisible
+ )
+ return false;
+
+ // current chart status
+ const chartStatus = this.props.chartStatus;
+ // `rendered` status is set right after `success` or `loading`,
+ // don't trigger rerender here (see `actions.chartRenderingSucceeded`).
+ // note that even though render is not triggered, the props will still be
+ // updated.
Review comment:
cc @conglei
----------------------------------------------------------------
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] ktmud commented on issue #9333: build: use
manifest hooks for dev server proxy
Posted by GitBox <gi...@apache.org>.
ktmud commented on issue #9333: build: use manifest hooks for dev server proxy
URL: https://github.com/apache/incubator-superset/pull/9333#issuecomment-602867470
Hold on.. the updated `shouldComponentUpdate` logic is cause some performance issues in dashboards.
----------------------------------------------------------------
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 a change in pull request
#9333: build: use manifest hooks for dev server proxy
Posted by GitBox <gi...@apache.org>.
rusackas commented on a change in pull request #9333: build: use manifest hooks for dev server proxy
URL: https://github.com/apache/incubator-superset/pull/9333#discussion_r395440891
##########
File path: CONTRIBUTING.md
##########
@@ -803,11 +803,27 @@ Then, [extract strings for the new language](#extracting-new-strings-for-transla
This means it'll register MyDatasource and MyOtherDatasource in superset.my_models module in the source registry.
-### Creating a new visualization type
+### Improve visualizations
+
+Superset is working towards a plugin system where new visualizations can be installed as optional npm packages. To achieve this goal,
+we are not accepting pull requests for new community-contributed visualization types at the moment. However, bugfixes for current visualizations are welcome. To edit the frontend code for visualizations, you will have to check out a copy of [apache-superset/superset-ui-plugins](https://github.com/apache-superset/superset-ui-plugins):
+
+```bash
+git clone https://github.com/apache-superset/superset-ui-plugins.git
+yarn && yarn build
+```
+
+Then use `npm link` to create a symlink of the source code in `superset-frontend/node_modules`:
+
+```bash
+cd incubator-superset/superset-frontend
+npm link ../superset-ui-plugins/packages/superset-ui-[PLUGIN NAME]
Review comment:
It's probably two levels back, so adjusting the sample path. Adding the means to link all packages will also help people
```suggestion
npm link ../../superset-ui-plugins/packages/superset-ui-[PLUGIN NAME]
# or to link all plugin packages:
# npm link ../../superset-ui-plugins/packages/*
```
----------------------------------------------------------------
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 merged pull request #9333: build: use
manifest hooks for dev server proxy and fix hot reload for charts
Posted by GitBox <gi...@apache.org>.
kristw merged pull request #9333: build: use manifest hooks for dev server proxy and fix hot reload for charts
URL: https://github.com/apache/incubator-superset/pull/9333
----------------------------------------------------------------
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] ktmud commented on a change in pull request
#9333: build: use manifest hooks for dev server proxy and fix hot reload
for charts
Posted by GitBox <gi...@apache.org>.
ktmud commented on a change in pull request #9333: build: use manifest hooks for dev server proxy and fix hot reload for charts
URL: https://github.com/apache/incubator-superset/pull/9333#discussion_r398852502
##########
File path: superset-frontend/src/dashboard/index.jsx
##########
@@ -18,6 +18,25 @@
*/
import React from 'react';
import ReactDOM from 'react-dom';
+import thunk from 'redux-thunk';
+import { createStore, applyMiddleware, compose } from 'redux';
+import { initFeatureFlags } from 'src/featureFlags';
+import { initEnhancer } from '../reduxUtils';
+import getInitialState from './reducers/getInitialState';
+import rootReducer from './reducers/index';
+import logger from '../middleware/loggerMiddleware';
+
import App from './App';
-ReactDOM.render(<App />, document.getElementById('app'));
+const appContainer = document.getElementById('app');
+const bootstrapData = JSON.parse(appContainer.getAttribute('data-bootstrap'));
+initFeatureFlags(bootstrapData.common.feature_flags);
+const initState = getInitialState(bootstrapData);
+
+const store = createStore(
+ rootReducer,
+ initState,
+ compose(applyMiddleware(thunk, logger), initEnhancer(false)),
+);
+
+ReactDOM.render(<App store={store} />, document.getElementById('app'));
Review comment:
Yes.
----------------------------------------------------------------
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