Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

build: use manifest hooks for dev server proxy and fix hot reload for charts #9333

Merged
merged 6 commits into from
Mar 26, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
38 changes: 30 additions & 8 deletions CONTRIBUTING.md
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -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
```

Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -803,11 +803,31 @@ 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
```

ktmud marked this conversation as resolved.
Show resolved Hide resolved
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

Expand Down Expand Up @@ -905,12 +925,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
```
Expand Down
115 changes: 46 additions & 69 deletions superset-frontend/package-lock.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion superset-frontend/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -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"
},
Expand Down
1 change: 1 addition & 0 deletions superset-frontend/src/chart/Chart.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,7 @@ const defaultProps = {
setControlValue() {},
triggerRender: false,
dashboardId: null,
chartStackTrace: null,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this change intended? I didn't see any reference within this PR.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

};

class Chart extends React.PureComponent {
Expand Down
22 changes: 2 additions & 20 deletions superset-frontend/src/dashboard/App.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -16,36 +16,18 @@
* specific language governing permissions and limitations
* under the License.
*/
import { hot } from 'react-hot-loader/root';
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>
Expand Down
21 changes: 20 additions & 1 deletion superset-frontend/src/dashboard/index.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -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'));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh nice. so this will get rid of the hot loader warning about store, right?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes.

Original file line number Diff line number Diff line change
Expand Up @@ -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}
/>
);
}
Expand Down
2 changes: 1 addition & 1 deletion superset-frontend/src/visualizations/presets/MainPreset.js
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,7 @@ import {
LineMultiChartPlugin,
PieChartPlugin,
TimePivotChartPlugin,
} from '@superset-ui/legacy-preset-chart-nvd3/lib';
} from '@superset-ui/legacy-preset-chart-nvd3';
ktmud marked this conversation as resolved.
Show resolved Hide resolved
import { BoxPlotChartPlugin } from '@superset-ui/preset-chart-xy/esm/legacy';
import { DeckGLChartPreset } from '@superset-ui/legacy-preset-chart-deckgl';

Expand Down
Loading