From 40fd252401f47f784a16bcef8fd3e508e4661b6c Mon Sep 17 00:00:00 2001 From: Jesse Yang Date: Thu, 19 Mar 2020 18:49:12 -0700 Subject: [PATCH 1/6] Use manifest hooks for dev server proxy --- CONTRIBUTING.md | 39 ++++-- superset-frontend/package-lock.json | 115 +++++++----------- superset-frontend/package.json | 2 +- .../src/visualizations/presets/MainPreset.js | 2 +- superset-frontend/webpack.config.js | 68 +++++++---- superset-frontend/webpack.proxy-config.js | 78 ++++-------- 6 files changed, 145 insertions(+), 159 deletions(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index f928e052d6f5..7469755f9ff7 100644 --- a/CONTRIBUTING.md +++ b/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) + - [Improving visualizations](#improving-visualizations) - [Adding a DB migration](#adding-a-db-migration) - [Merging DB migrations](#merging-db-migrations) - [SQL Lab Async](#sql-lab-async) @@ -389,7 +389,7 @@ Make sure your machine meets the [OS dependencies](https://superset.incubator.ap Developers should use a virtualenv. -``` +```bash pip install virtualenv ``` @@ -726,7 +726,7 @@ In TypeScript/JavaScript, the technique is similar: we import `t` (simple translation), `tn` (translation containing a number). ```javascript -import { t, tn } from "@superset-ui/translation"; +import { t, tn } from '@superset-ui/translation'; ``` ### Enabling language selection @@ -803,11 +803,32 @@ 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 +### Improving 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] + +# Or to link all plugin packages: +# npm link ../../superset-ui-plugins/packages/* + +# Start developing +npm run dev-server +``` + +When plugin packages are linked with `npm link`, the dev server will automatically load files from the plugin's `/src` directory. -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(s) and may have to run `npm link` again. ### Adding a DB migration @@ -905,12 +926,14 @@ To do this, you'll need to: - Configure a results backend, here's a local `FileSystemCache` example, not recommended for production, but perfect for testing (stores cache in `/tmp`) + ```python from werkzeug.contrib.cache import FileSystemCache RESULTS_BACKEND = FileSystemCache('/tmp/sqllab') ``` -* Start up a celery worker +- Start up a celery worker + ```shell script celery worker --app=superset.tasks.celery_app:app -Ofair ``` diff --git a/superset-frontend/package-lock.json b/superset-frontend/package-lock.json index 845fb4279224..4478e07ee84a 100644 --- a/superset-frontend/package-lock.json +++ b/superset-frontend/package-lock.json @@ -17371,6 +17371,17 @@ "readable-stream": "^2.0.0" } }, + "fs-extra": { + "version": "7.0.1", + "resolved": "https://registry.npmjs.org/fs-extra/-/fs-extra-7.0.1.tgz", + "integrity": "sha512-YJDaCJZEnBmcbw13fvdAM9AwNOJwOzrE4pqMqBq5nFiEqXUqHwlK4B+3pUw6JNvfSPtX05xFHtYy/1ni01eGCw==", + "dev": true, + "requires": { + "graceful-fs": "^4.1.2", + "jsonfile": "^4.0.0", + "universalify": "^0.1.0" + } + }, "fs-readdir-recursive": { "version": "1.1.0", "resolved": "https://registry.npmjs.org/fs-readdir-recursive/-/fs-readdir-recursive-1.1.0.tgz", @@ -25687,6 +25698,15 @@ "resolved": "http://registry.npmjs.org/json5/-/json5-0.5.1.tgz", "integrity": "sha1-Hq3nrMASA0rYTiOWdn6tn6VJWCE=" }, + "jsonfile": { + "version": "4.0.0", + "resolved": "https://registry.npmjs.org/jsonfile/-/jsonfile-4.0.0.tgz", + "integrity": "sha1-h3Gq4HmbZAdrdmQPygWPnBDjPss=", + "dev": true, + "requires": { + "graceful-fs": "^4.1.6" + } + }, "jsprim": { "version": "1.4.1", "resolved": "https://registry.npmjs.org/jsprim/-/jsprim-1.4.1.tgz", @@ -25905,12 +25925,6 @@ "resolved": "https://registry.npmjs.org/lodash.get/-/lodash.get-4.4.2.tgz", "integrity": "sha1-LRd/ZS+jHpObRDjVNBSZ36OCXpk=" }, - "lodash.has": { - "version": "4.5.2", - "resolved": "https://registry.npmjs.org/lodash.has/-/lodash.has-4.5.2.tgz", - "integrity": "sha1-0Z9NwQlQWMzL4rDN9O4P5Ko3yGI=", - "dev": true - }, "lodash.isequal": { "version": "4.5.0", "resolved": "https://registry.npmjs.org/lodash.isequal/-/lodash.isequal-4.5.0.tgz", @@ -33995,6 +34009,12 @@ "resolved": "https://registry.npmjs.org/unist-util-visit-parents/-/unist-util-visit-parents-1.1.2.tgz", "integrity": "sha512-yvo+MMLjEwdc3RhhPYSximset7rwjMrdt9E41Smmvg25UQIenzrN83cRnF1JMzoMi9zZOQeYXHSDf7p+IQkW3Q==" }, + "universalify": { + "version": "0.1.2", + "resolved": "https://registry.npmjs.org/universalify/-/universalify-0.1.2.tgz", + "integrity": "sha512-rBJeI5CXAlmy1pV+617WB9J63U6XcazHHF2f2dbJix4XzpUF0RS3Zbj0FGIOCAva5P/d/GBOYaACQ1w+0azUkg==", + "dev": true + }, "unpipe": { "version": "1.0.0", "resolved": "https://registry.npmjs.org/unpipe/-/unpipe-1.0.0.tgz", @@ -35321,69 +35341,6 @@ } } }, - "webpack-assets-manifest": { - "version": "3.1.1", - "resolved": "https://registry.npmjs.org/webpack-assets-manifest/-/webpack-assets-manifest-3.1.1.tgz", - "integrity": "sha512-JV9V2QKc5wEWQptdIjvXDUL1ucbPLH2f27toAY3SNdGZp+xSaStAgpoMcvMZmqtFrBc9a5pTS1058vxyMPOzRQ==", - "dev": true, - "requires": { - "chalk": "^2.0", - "lodash.get": "^4.0", - "lodash.has": "^4.0", - "mkdirp": "^0.5", - "schema-utils": "^1.0.0", - "tapable": "^1.0.0", - "webpack-sources": "^1.0.0" - }, - "dependencies": { - "ansi-styles": { - "version": "3.2.1", - "resolved": "https://registry.npmjs.org/ansi-styles/-/ansi-styles-3.2.1.tgz", - "integrity": "sha512-VT0ZI6kZRdTh8YyJw3SMbYm/u+NqfsAxEpWO0Pf9sq8/e94WxxOpPKx9FR1FlyCtOVDNOQ+8ntlqFxiRc+r5qA==", - "dev": true, - "requires": { - "color-convert": "^1.9.0" - } - }, - "chalk": { - "version": "2.4.1", - "resolved": "https://registry.npmjs.org/chalk/-/chalk-2.4.1.tgz", - "integrity": "sha512-ObN6h1v2fTJSmUXoS3nMQ92LbDK9be4TV+6G+omQlGJFdcUX5heKi1LZ1YnRMIgwTLEj3E24bT6tYni50rlCfQ==", - "dev": true, - "requires": { - "ansi-styles": "^3.2.1", - "escape-string-regexp": "^1.0.5", - "supports-color": "^5.3.0" - } - }, - "schema-utils": { - "version": "1.0.0", - "resolved": "https://registry.npmjs.org/schema-utils/-/schema-utils-1.0.0.tgz", - "integrity": "sha512-i27Mic4KovM/lnGsy8whRCHhc7VicJajAjTrYg11K9zfZXnYIt4k5F+kZkwjnrhKzLic/HLU4j11mjsz2G/75g==", - "dev": true, - "requires": { - "ajv": "^6.1.0", - "ajv-errors": "^1.0.0", - "ajv-keywords": "^3.1.0" - } - }, - "supports-color": { - "version": "5.5.0", - "resolved": "https://registry.npmjs.org/supports-color/-/supports-color-5.5.0.tgz", - "integrity": "sha512-QjVjwdXIt408MIiAqCX4oUKsgU2EqAGzs2Ppkm4aQYbjm+ZEWEcW4SfFNTr4uMNZma0ey4f5lgLrkB0aX0QMow==", - "dev": true, - "requires": { - "has-flag": "^3.0.0" - } - }, - "tapable": { - "version": "1.1.1", - "resolved": "https://registry.npmjs.org/tapable/-/tapable-1.1.1.tgz", - "integrity": "sha512-9I2ydhj8Z9veORCw5PRm4u9uebCn0mcCa6scWoNcbZ6dAtoo2618u9UUzxgmsCOreJpqDDuv61LvwofW7hLcBA==", - "dev": true - } - } - }, "webpack-bundle-analyzer": { "version": "3.6.1", "resolved": "https://registry.npmjs.org/webpack-bundle-analyzer/-/webpack-bundle-analyzer-3.6.1.tgz", @@ -36815,6 +36772,26 @@ "uuid": "^3.3.2" } }, + "webpack-manifest-plugin": { + "version": "2.2.0", + "resolved": "https://registry.npmjs.org/webpack-manifest-plugin/-/webpack-manifest-plugin-2.2.0.tgz", + "integrity": "sha512-9S6YyKKKh/Oz/eryM1RyLVDVmy3NSPV0JXMRhZ18fJsq+AwGxUY34X54VNwkzYcEmEkDwNxuEOboCZEebJXBAQ==", + "dev": true, + "requires": { + "fs-extra": "^7.0.0", + "lodash": ">=3.5 <5", + "object.entries": "^1.1.0", + "tapable": "^1.0.0" + }, + "dependencies": { + "tapable": { + "version": "1.1.3", + "resolved": "https://registry.npmjs.org/tapable/-/tapable-1.1.3.tgz", + "integrity": "sha512-4WK/bYZmj8xLr+HUCODHGF1ZFzsYffasLUgEiMBY4fgtltdO6B4WJtlSbPaDTLpYTcGVwM2qLnFTICEcNxs3kA==", + "dev": true + } + } + }, "webpack-sources": { "version": "1.4.3", "resolved": "https://registry.npmjs.org/webpack-sources/-/webpack-sources-1.4.3.tgz", diff --git a/superset-frontend/package.json b/superset-frontend/package.json index d37d8c4d80e2..49b6bb7c848a 100644 --- a/superset-frontend/package.json +++ b/superset-frontend/package.json @@ -231,10 +231,10 @@ "typescript": "^3.8.3", "url-loader": "^1.0.1", "webpack": "^4.42.0", - "webpack-assets-manifest": "^3.1.1", "webpack-bundle-analyzer": "^3.6.1", "webpack-cli": "^3.3.11", "webpack-dev-server": "^3.10.3", + "webpack-manifest-plugin": "^2.2.0", "webpack-sources": "^1.4.3", "yargs": "12 - 15" }, diff --git a/superset-frontend/src/visualizations/presets/MainPreset.js b/superset-frontend/src/visualizations/presets/MainPreset.js index f543fe34bc68..12d39c096bae 100644 --- a/superset-frontend/src/visualizations/presets/MainPreset.js +++ b/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'; import { BoxPlotChartPlugin } from '@superset-ui/preset-chart-xy/esm/legacy'; import { DeckGLChartPreset } from '@superset-ui/legacy-preset-chart-deckgl'; diff --git a/superset-frontend/webpack.config.js b/superset-frontend/webpack.config.js index c26ef0351b27..d16f186acbf2 100644 --- a/superset-frontend/webpack.config.js +++ b/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'); const ForkTsCheckerWebpackPlugin = require('fork-ts-checker-webpack-plugin'); const parsedArgs = require('yargs').argv; +const getProxyConfig = require('./webpack.proxy-config'); const packageConfig = require('./package.json'); // input dir @@ -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, + }; + }, // Also write to disk when using devServer // instead of only keeping manifest.json in memory // This is required to make devServer work with flask. - writeToDisk: isDevMode, + writeToFileEmit: isDevMode, }), // create fresh dist/ upon build @@ -127,6 +152,8 @@ const babelLoader = { loader: 'babel-loader', options: { cacheDirectory: true, + // disable gzip compression for cache files + // faster when there are millions of small files cacheCompression: false, }, }; @@ -205,7 +232,7 @@ const config = { { test: /\.jsx?$/, // include source code for plugins, but exclude node_modules within them - exclude: [/superset-ui.*\/node_modules\/.*/], + exclude: [/superset-ui.*\/node_modules\//], include: [new RegExp(`${APP_DIR}/src`), /superset-ui.*\/src/], use: [babelLoader], }, @@ -277,28 +304,17 @@ const config = { devtool: false, }; -let proxyConfig = {}; -const requireModule = module.require; - -function loadProxyConfig() { - try { - delete require.cache[require.resolve('./webpack.proxy-config')]; - proxyConfig = requireModule('./webpack.proxy-config'); - } catch (e) { - if (e.code !== 'ENOENT') { - console.error('\n>> Error loading proxy config:'); - console.trace(e); - } - } -} +let proxyConfig = getProxyConfig(); if (isDevMode) { config.devtool = 'eval-cheap-module-source-map'; config.devServer = { - before() { - loadProxyConfig(); - // hot reloading proxy config - fs.watch('./webpack.proxy-config.js', loadProxyConfig); + before(app, server, compiler) { + // load proxy config when manifest updates + const hook = compiler.hooks.webpackManifestPluginAfterEmit; + hook.tap('ManifestPlugin', manifest => { + proxyConfig = getProxyConfig(manifest); + }); }, historyApiFallback: true, hot: true, diff --git a/superset-frontend/webpack.proxy-config.js b/superset-frontend/webpack.proxy-config.js index a797a5cee56c..1d7999f6a0d3 100644 --- a/superset-frontend/webpack.proxy-config.js +++ b/superset-frontend/webpack.proxy-config.js @@ -1,4 +1,3 @@ -/* eslint-disable no-console */ /** * Licensed to the Apache Software Foundation (ASF) under one * or more contributor license agreements. See the NOTICE file @@ -17,9 +16,7 @@ * specific language governing permissions and limitations * under the License. */ -const fs = require('fs'); const zlib = require('zlib'); -const path = require('path'); // eslint-disable-next-line import/no-extraneous-dependencies const parsedArgs = require('yargs').argv; @@ -28,28 +25,8 @@ const backend = (supersetUrl || `http://localhost:${supersetPort}`).replace( '//+$/', '', ); // strip ending backslash -const MANIFEST_FILE = path.resolve( - __dirname, - '../superset/static/assets/manifest.json', -); -let manifestContent; let manifest; -function loadManifest() { - try { - const newContent = fs.readFileSync(MANIFEST_FILE, { encoding: 'utf-8' }); - if (!newContent || newContent === manifestContent) return; - manifestContent = newContent; - manifest = JSON.parse(manifestContent); - console.log(`${MANIFEST_FILE} loaded.`); - } catch (e) { - if (e.code !== 'ENOENT') { - console.error('\n>> Error loading manifest file:'); - console.trace(e); - } - } -} - function isHTML(res) { const CONTENT_TYPE_HEADER = 'content-type'; const contentType = res.getHeader @@ -63,10 +40,6 @@ function toDevHTML(originalHtml) { /(\s*)([\s\S]*)(<\/title>)/i, '$1[DEV] $2 $3', ); - // load manifest file only when needed - if (!manifest) { - loadManifest(); - } if (manifest) { const loaded = new Set(); // replace bundled asset files, HTML comment tags generated by Jinja macros @@ -152,32 +125,29 @@ function processHTML(proxyResponse, response) { }); } -// make sure the manifest file exists -fs.mkdirSync(path.dirname(MANIFEST_FILE), { recursive: true }); -fs.closeSync(fs.openSync(MANIFEST_FILE, 'as+')); -// watch it as webpack-dev-server updates it -fs.watch(MANIFEST_FILE, loadManifest); - -module.exports = { - context: '/', - target: backend, - hostRewrite: true, - changeOrigin: true, - cookieDomainRewrite: '', // remove cookie domain - selfHandleResponse: true, // so that the onProxyRes takes care of sending the response - onProxyRes(proxyResponse, request, response) { - try { - copyHeaders(proxyResponse, response); - if (isHTML(response)) { - processHTML(proxyResponse, response); - } else { - proxyResponse.pipe(response); +module.exports = newManifest => { + manifest = newManifest; + return { + context: '/', + target: backend, + hostRewrite: true, + changeOrigin: true, + cookieDomainRewrite: '', // remove cookie domain + selfHandleResponse: true, // so that the onProxyRes takes care of sending the response + onProxyRes(proxyResponse, request, response) { + try { + copyHeaders(proxyResponse, response); + if (isHTML(response)) { + processHTML(proxyResponse, response); + } else { + proxyResponse.pipe(response); + } + response.flushHeaders(); + } catch (e) { + response.setHeader('content-type', 'text/plain'); + response.write(`Error requesting ${request.path} from proxy:\n\n`); + response.end(e.stack); } - response.flushHeaders(); - } catch (e) { - response.setHeader('content-type', 'text/plain'); - response.write(`Error requesting ${request.path} from proxy:\n\n`); - response.end(e.stack); - } - }, + }, + }; }; From 7b867e9ca67220735123cd48c5b23cb1737c3899 Mon Sep 17 00:00:00 2001 From: Jesse Yang <jesse.yang@airbnb.com> Date: Sat, 21 Mar 2020 18:03:21 -0700 Subject: [PATCH 2/6] Rewrite dashboard/App.jsx to supress Redux error in hot reload --- superset-frontend/src/dashboard/App.jsx | 23 +++-------------------- superset-frontend/src/dashboard/index.jsx | 21 ++++++++++++++++++++- 2 files changed, 23 insertions(+), 21 deletions(-) diff --git a/superset-frontend/src/dashboard/App.jsx b/superset-frontend/src/dashboard/App.jsx index c30272c12f5e..9c66f89d0def 100644 --- a/superset-frontend/src/dashboard/App.jsx +++ b/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'; import React from 'react'; -import thunk from 'redux-thunk'; -import { createStore, applyMiddleware, compose } from 'redux'; import { Provider } from 'react-redux'; -import { hot } from 'react-hot-loader/root'; -import { initFeatureFlags } from 'src/featureFlags'; -import { initEnhancer } from '../reduxUtils'; -import logger from '../middleware/loggerMiddleware'; import setupApp from '../setup/setupApp'; import setupPlugins from '../setup/setupPlugins'; import DashboardContainer from './containers/Dashboard'; -import getInitialState from './reducers/getInitialState'; -import rootReducer from './reducers/index'; setupApp(); setupPlugins(); -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)), -); - -const App = () => ( +const App = ({ store }) => ( <Provider store={store}> <DashboardContainer /> </Provider> diff --git a/superset-frontend/src/dashboard/index.jsx b/superset-frontend/src/dashboard/index.jsx index c257009e64fd..3937a357dfcc 100644 --- a/superset-frontend/src/dashboard/index.jsx +++ b/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')); From 40c43cd2067e07569a27e6f7f10d9151f7302671 Mon Sep 17 00:00:00 2001 From: Jesse Yang <jesse.yang@airbnb.com> Date: Sat, 21 Mar 2020 18:03:59 -0700 Subject: [PATCH 3/6] Update ChartRenderer to allow hot realod in Explore --- superset-frontend/src/chart/Chart.jsx | 1 + superset-frontend/src/chart/ChartRenderer.jsx | 42 +++++++++---------- 2 files changed, 20 insertions(+), 23 deletions(-) diff --git a/superset-frontend/src/chart/Chart.jsx b/superset-frontend/src/chart/Chart.jsx index 227b7af86b47..044f0d83fe82 100644 --- a/superset-frontend/src/chart/Chart.jsx +++ b/superset-frontend/src/chart/Chart.jsx @@ -74,6 +74,7 @@ const defaultProps = { setControlValue() {}, triggerRender: false, dashboardId: null, + chartStackTrace: null, }; class Chart extends React.PureComponent { diff --git a/superset-frontend/src/chart/ChartRenderer.jsx b/superset-frontend/src/chart/ChartRenderer.jsx index 2adfd33a8278..784be824b741 100644 --- a/superset-frontend/src/chart/ChartRenderer.jsx +++ b/superset-frontend/src/chart/ChartRenderer.jsx @@ -16,6 +16,7 @@ * specific language governing permissions and limitations * under the License. */ +import { setConfig } from 'react-hot-loader'; import dompurify from 'dompurify'; import { snakeCase } from 'lodash'; import PropTypes from 'prop-types'; @@ -58,6 +59,11 @@ const defaultProps = { triggerRender: false, }; +const isDevMode = process.env.WEBPACK_MODE === 'development'; +if (isDevMode) { + setConfig({ logLevel: 'debug', trackTailUpdates: false }); +} + class ChartRenderer extends React.Component { constructor(props) { super(props); @@ -77,29 +83,19 @@ 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; - } + shouldComponentUpdate(nextProps, nextState) { + const props = this.props; + if ( + props.queryResponse === nextProps.queryResponse && + props.chartStatus === 'success' && + nextProps.chartStatus === 'rendered' + ) { + // don't rerender if it's updating from `success` to `rendered`. + // however, do update current props; + this.props = nextProps; + return false; } - return false; + return true; } handleAddFilter(col, vals, merge = true, refresh = true) { @@ -111,7 +107,6 @@ class ChartRenderer extends React.Component { if (['loading', 'rendered'].indexOf(chartStatus) < 0) { actions.chartRenderingSucceeded(chartId); } - // only log chart render time which is triggered by query results change // currently we don't log chart re-render time, like window resize etc if (this.hasQueryResponseChange) { @@ -197,6 +192,7 @@ class ChartRenderer extends React.Component { return ( <SuperChart + key={`${chartId}-${isDevMode ? Date.now() : ''}`} disableErrorBoundary id={`chart-id-${chartId}`} className={chartClassName} From f6cf617692ba6ecf6c3da37bae2fc341f810012f Mon Sep 17 00:00:00 2001 From: Jesse Yang <jesse.yang@airbnb.com> Date: Tue, 24 Mar 2020 00:00:59 -0700 Subject: [PATCH 4/6] Fix hot reload in dashboars as well --- CONTRIBUTING.md | 3 +- superset-frontend/src/chart/ChartRenderer.jsx | 73 ++++++++++++++++--- .../components/gridComponents/Chart.jsx | 3 +- .../components/ExploreViewContainer.jsx | 2 +- 4 files changed, 68 insertions(+), 13 deletions(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 7469755f9ff7..ce69c8ca75f8 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -805,8 +805,7 @@ Then, [extract strings for the new language](#extracting-new-strings-for-transla ### Improving 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): +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 diff --git a/superset-frontend/src/chart/ChartRenderer.jsx b/superset-frontend/src/chart/ChartRenderer.jsx index 784be824b741..11aa91433af6 100644 --- a/superset-frontend/src/chart/ChartRenderer.jsx +++ b/superset-frontend/src/chart/ChartRenderer.jsx @@ -64,6 +64,41 @@ if (isDevMode) { setConfig({ logLevel: 'debug', trackTailUpdates: false }); } +/** + * Compare two objects with DFS (till a maxDepth) + */ +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; +} + class ChartRenderer extends React.Component { constructor(props) { super(props); @@ -83,18 +118,35 @@ class ChartRenderer extends React.Component { }; } - shouldComponentUpdate(nextProps, nextState) { - const props = this.props; + shouldComponentUpdate(nextProps) { + // 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; + // `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. if ( - props.queryResponse === nextProps.queryResponse && - props.chartStatus === 'success' && + (chartStatus === 'success' || chartStatus === 'loading') && nextProps.chartStatus === 'rendered' - ) { - // don't rerender if it's updating from `success` to `rendered`. - // however, do update current props; - this.props = nextProps; + ) return false; + + // already successfully rendered, only rerender when some prop is updated + if (chartStatus === 'rendered' || chartStatus === 'success') { + return !deepEqual(this.props, nextProps, 2); } + return true; } @@ -104,7 +156,7 @@ class ChartRenderer extends React.Component { handleRenderSuccess() { const { actions, chartStatus, chartId, vizType } = this.props; - if (['loading', 'rendered'].indexOf(chartStatus) < 0) { + if (chartStatus !== 'loading' && chartStatus !== 'rendered') { actions.chartRenderingSucceeded(chartId); } // only log chart render time which is triggered by query results change @@ -190,6 +242,9 @@ 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); + return ( <SuperChart key={`${chartId}-${isDevMode ? Date.now() : ''}`} diff --git a/superset-frontend/src/dashboard/components/gridComponents/Chart.jsx b/superset-frontend/src/dashboard/components/gridComponents/Chart.jsx index b3995284b4ef..d9b66d995d3a 100644 --- a/superset-frontend/src/dashboard/components/gridComponents/Chart.jsx +++ b/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 + return this.props.cacheBusterProp !== nextProps.cacheBusterProp; } componentWillUnmount() { diff --git a/superset-frontend/src/explore/components/ExploreViewContainer.jsx b/superset-frontend/src/explore/components/ExploreViewContainer.jsx index f83139503589..4cde0286e00e 100644 --- a/superset-frontend/src/explore/components/ExploreViewContainer.jsx +++ b/superset-frontend/src/explore/components/ExploreViewContainer.jsx @@ -317,7 +317,7 @@ class ExploreViewContainer extends React.Component { errorMessage={this.renderErrorMessage()} refreshOverlayVisible={this.state.refreshOverlayVisible} addHistory={this.addHistory} - onQuery={this.onQuery.bind(this)} + onQuery={this.onQuery} /> ); } From 74c3b021db6ad52e1aea478978fe9ccb4b1125df Mon Sep 17 00:00:00 2001 From: Jesse Yang <jesse.yang@airbnb.com> Date: Thu, 26 Mar 2020 13:07:16 -0700 Subject: [PATCH 5/6] Revert changes to ChartRenderer.jsx Will submit in another PR. --- superset-frontend/src/chart/ChartRenderer.jsx | 97 +++++-------------- .../components/gridComponents/Chart.jsx | 3 +- superset-frontend/webpack.config.js | 1 - 3 files changed, 24 insertions(+), 77 deletions(-) diff --git a/superset-frontend/src/chart/ChartRenderer.jsx b/superset-frontend/src/chart/ChartRenderer.jsx index 11aa91433af6..2adfd33a8278 100644 --- a/superset-frontend/src/chart/ChartRenderer.jsx +++ b/superset-frontend/src/chart/ChartRenderer.jsx @@ -16,7 +16,6 @@ * specific language governing permissions and limitations * under the License. */ -import { setConfig } from 'react-hot-loader'; import dompurify from 'dompurify'; import { snakeCase } from 'lodash'; import PropTypes from 'prop-types'; @@ -59,46 +58,6 @@ const defaultProps = { triggerRender: false, }; -const isDevMode = process.env.WEBPACK_MODE === 'development'; -if (isDevMode) { - setConfig({ logLevel: 'debug', trackTailUpdates: false }); -} - -/** - * Compare two objects with DFS (till a maxDepth) - */ -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; -} - class ChartRenderer extends React.Component { constructor(props) { super(props); @@ -119,35 +78,28 @@ class ChartRenderer extends React.Component { } shouldComponentUpdate(nextProps) { - // 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; - // `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. - if ( - (chartStatus === 'success' || chartStatus === 'loading') && - nextProps.chartStatus === 'rendered' - ) - return false; - - // already successfully rendered, only rerender when some prop is updated - if (chartStatus === 'rendered' || chartStatus === 'success') { - return !deepEqual(this.props, nextProps, 2); + 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; + } } - - return true; + return false; } handleAddFilter(col, vals, merge = true, refresh = true) { @@ -156,9 +108,10 @@ class ChartRenderer extends React.Component { handleRenderSuccess() { const { actions, chartStatus, chartId, vizType } = this.props; - if (chartStatus !== 'loading' && chartStatus !== 'rendered') { + if (['loading', 'rendered'].indexOf(chartStatus) < 0) { actions.chartRenderingSucceeded(chartId); } + // only log chart render time which is triggered by query results change // currently we don't log chart re-render time, like window resize etc if (this.hasQueryResponseChange) { @@ -242,12 +195,8 @@ 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); - return ( <SuperChart - key={`${chartId}-${isDevMode ? Date.now() : ''}`} disableErrorBoundary id={`chart-id-${chartId}`} className={chartClassName} diff --git a/superset-frontend/src/dashboard/components/gridComponents/Chart.jsx b/superset-frontend/src/dashboard/components/gridComponents/Chart.jsx index d9b66d995d3a..b3995284b4ef 100644 --- a/superset-frontend/src/dashboard/components/gridComponents/Chart.jsx +++ b/superset-frontend/src/dashboard/components/gridComponents/Chart.jsx @@ -133,8 +133,7 @@ class Chart extends React.Component { } } - // `cacheBusterProp` is jnjected by react-hot-loader - return this.props.cacheBusterProp !== nextProps.cacheBusterProp; + return false; } componentWillUnmount() { diff --git a/superset-frontend/webpack.config.js b/superset-frontend/webpack.config.js index d16f186acbf2..ba4860cf88bb 100644 --- a/superset-frontend/webpack.config.js +++ b/superset-frontend/webpack.config.js @@ -85,7 +85,6 @@ const plugins = [ } return { ...seed, - // files: filePaths, entrypoints: entryFiles, }; }, From 058fc125ab5906a988802d248f5104c240d1c7bb Mon Sep 17 00:00:00 2001 From: Jesse Yang <jesse.yang@airbnb.com> Date: Thu, 26 Mar 2020 15:02:31 -0700 Subject: [PATCH 6/6] Clean up --- superset-frontend/src/dashboard/App.jsx | 1 - 1 file changed, 1 deletion(-) diff --git a/superset-frontend/src/dashboard/App.jsx b/superset-frontend/src/dashboard/App.jsx index 9c66f89d0def..baadcfcb9600 100644 --- a/superset-frontend/src/dashboard/App.jsx +++ b/superset-frontend/src/dashboard/App.jsx @@ -17,7 +17,6 @@ * under the License. */ import { hot } from 'react-hot-loader/root'; -import { setConfig } from 'react-hot-loader'; import React from 'react'; import { Provider } from 'react-redux';