Add sql lab localStorage config #6257

graceguo-supercat · 2018-11-01T04:24:49Z

Sql lab used redux-localStorage to persist sql lab redux state into localStorage. by default all the state is persisted. Example of sql lab localStorage:

we should persist user queries data, but not server-side configuration like common section, messageToasts, etc. localStorage will override server-side configuration when page related. this will cause big problem when we try to set values dynamically from server-side. Also we pass whole translation package in common section. it's a waste of localStorage resource.

this PR will

move common out of sqlLab, and set it as root level in redux state.
only persist sqlLab into localStorage.

@jeffreythewang @timifasubaa @kristw @williaster

codecov-io · 2018-11-01T04:42:26Z

Codecov Report

Merging #6257 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #6257   +/-   ##
=======================================
  Coverage   77.01%   77.01%           
=======================================
  Files          64       64           
  Lines        9508     9508           
=======================================
  Hits         7323     7323           
  Misses       2185     2185

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1a4199a...c152b37. Read the comment docs.

kristw

Overall LGTM.

I only see changes to reducers.
Double-checking if there is any part that of the code (container?) that use sqlLab.common that need to be updated too?

kristw · 2018-11-01T18:28:32Z

superset/assets/src/reduxUtils.js

-  let enhancer = persist ? compose(persistState()) : compose();
+export function initEnhancer(persist = true, persistConfig = {}) {
+  const { paths, config } = persistConfig;
+  let enhancer = persist ? compose(persistState(paths, config)) : compose();
  if (process.env.WEBPACK_MODE === 'development') {


Perhaps move the dev mode logic up to reduce duplicate code and mutation.

const composeEnhancers = process.env.WEBPACK_MODE === 'development' ? (window.__REDUX_DEVTOOLS_EXTENSION_COMPOSE__ || compose) : compose; const { paths, config } = persistConfig; return persist ? composeEnhancers(persistState(paths, config)) : composeEnhancers();

jeffreythewang

Overall this lgtm, and makes localStorage more organized when loading values from the backend, but I don't think it makes adding a new field in TabbedSqlEditors.jsx any cleaner.

jeffreythewang · 2018-11-01T18:22:40Z

superset/assets/src/SqlLab/getInitialState.js

@@ -12,6 +12,7 @@ export default function getInitialState({ defaultDbId, ...restBootstrapData }) {
    autorun: false,
    dbId: defaultDbId,
  };
+  const { common } = restBootstrapData;

  return {
    sqlLab: {


What if we ever want to add a field here? We'd have to add it outside of this object (like in common), and then store it back in localStorage by adding it somewhere else right (like in TabbedSqlEditors.jsx)?

as long as new field is part of sqlLab, it will be persisted into localStorage.
After this PR, for all other root level keys in redux state, will not be persisted.

jeffreythewang · 2018-11-01T18:31:57Z

superset/assets/src/SqlLab/App.jsx

+    slicer: paths => (state) => {
+      const subset = {};
+      paths.forEach((path) => {
+        delete state[path].common; // eslint-disable-line no-param-reassign


This is just a temporary solution for migrating over existing localStorage, and eventually it will be dead code. Do you foresee this happening for any other field currently under sqlLab if we decide to change the schema?

yes. this line is to fix the screwed localStorage.

williaster · 2018-11-01T21:24:25Z

superset/assets/src/SqlLab/reducers.js

@@ -278,7 +278,12 @@ export const sqlLabReducer = function (state = {}, action) {
  return state;
 };

+export const commonReducer = function (state = {}) {


can you put this in its own file so we define the reducer for one part of the state tree per file?

sql lab used single actions and reducers file. one line above is sqlLabReducer. is it really necessary to split actions/reducers into different files?

I'd say yes, the number of files in Superset that have thousands of lines is crazy makes them all harder to read. it's really trivial to add a new file imo. up to you tho.

ok. I split reducers into different files.

williaster · 2018-11-01T21:29:13Z

superset/assets/src/SqlLab/App.jsx

+    slicer: paths => (state) => {
+      const subset = {};
+      paths.forEach((path) => {
+        delete state[path].common; // eslint-disable-line no-param-reassign


can you add a comment explaining what is going on here so people that don't have context can understand it?

also, what's the plan for removing this once it becomes dead code? can we safely delete it in a month? 2 months?

this is a good question..we don't have many options on browser's localStorage if user didn't open sql lab. but if user didn't open sql lab for 2 months he probably didn't critically depend on localStorage.

this PR will help @jeffreythewang's feature: #4941. So I am thinking after a few new features in Sql Lab (a month long is also good), we can remove this cleanup line.

graceguo-supercat · 2018-11-01T21:50:35Z

For redux root level key common, i followed the same practice for dashboard and explore view:

https://github.com/apache/incubator-superset/blob/5f1eaa49f26d7088f8fa0f84c9e6d3467117557f/superset/assets/src/dashboard/reducers/getInitialState.js#L160

https://github.com/apache/incubator-superset/blob/6ec9e1a4e94beb1d6a1f49e25569473ba99715c1/superset/assets/src/explore/reducers/getInitialState.js#L13

williaster

LGTM

graceguo-supercat force-pushed the gg-AddSqlLabLocalStorageConfig branch from dbb9860 to 67b970d Compare November 1, 2018 17:53

kristw reviewed Nov 1, 2018

View reviewed changes

jeffreythewang approved these changes Nov 1, 2018

View reviewed changes

graceguo-supercat force-pushed the gg-AddSqlLabLocalStorageConfig branch 2 times, most recently from c7c5868 to d364de1 Compare November 1, 2018 19:32

williaster reviewed Nov 1, 2018

View reviewed changes

graceguo-supercat force-pushed the gg-AddSqlLabLocalStorageConfig branch from d364de1 to 67b9e92 Compare November 1, 2018 21:40

graceguo-supercat force-pushed the gg-AddSqlLabLocalStorageConfig branch 2 times, most recently from 8a54415 to 502b6e1 Compare November 2, 2018 01:01

williaster approved these changes Nov 6, 2018

View reviewed changes

kristw approved these changes Nov 6, 2018

View reviewed changes

graceguo-supercat force-pushed the gg-AddSqlLabLocalStorageConfig branch from 502b6e1 to fdf6177 Compare November 7, 2018 06:25

sql lab localStorage config

c152b37

graceguo-supercat force-pushed the gg-AddSqlLabLocalStorageConfig branch from fdf6177 to c152b37 Compare November 7, 2018 06:54

graceguo-supercat merged commit 69e8df4 into apache:master Nov 7, 2018

graceguo-supercat deleted the gg-AddSqlLabLocalStorageConfig branch November 7, 2018 07:12

bipinsoniguavus pushed a commit to ThalesGroup/incubator-superset that referenced this pull request Dec 26, 2018

sql lab localStorage config (apache#6257)

76aaf11

mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 0.34.0 labels Feb 27, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add sql lab localStorage config #6257

Add sql lab localStorage config #6257

graceguo-supercat commented Nov 1, 2018 •

edited

Loading

codecov-io commented Nov 1, 2018 •

edited

Loading

kristw left a comment •

edited

Loading

kristw Nov 1, 2018

jeffreythewang left a comment

jeffreythewang Nov 1, 2018

graceguo-supercat Nov 1, 2018

jeffreythewang Nov 1, 2018

graceguo-supercat Nov 1, 2018

williaster Nov 1, 2018 •

edited

Loading

graceguo-supercat Nov 1, 2018

williaster Nov 1, 2018

graceguo-supercat Nov 2, 2018

williaster Nov 1, 2018

graceguo-supercat Nov 2, 2018 •

edited

Loading

graceguo-supercat commented Nov 1, 2018

williaster left a comment

Add sql lab localStorage config #6257

Add sql lab localStorage config #6257

Conversation

graceguo-supercat commented Nov 1, 2018 • edited Loading

codecov-io commented Nov 1, 2018 • edited Loading

Codecov Report

kristw left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeffreythewang left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

williaster Nov 1, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

graceguo-supercat Nov 2, 2018 • edited Loading

Choose a reason for hiding this comment

graceguo-supercat commented Nov 1, 2018

williaster left a comment

Choose a reason for hiding this comment

graceguo-supercat commented Nov 1, 2018 •

edited

Loading

codecov-io commented Nov 1, 2018 •

edited

Loading

kristw left a comment •

edited

Loading

williaster Nov 1, 2018 •

edited

Loading

graceguo-supercat Nov 2, 2018 •

edited

Loading