[fix] reduce content in sql lab localStorage #7998
Conversation
| export function clearQueryEditors(queryEditors) { | ||
| return queryEditors.map(editor => | ||
| // only return selected keys | ||
| Object.keys(editor).reduce((accumulator, key) => { |
There was a problem hiding this comment.
- We could make
PERSISTENT_QUERY_EDITOR_KEYSaSet. filterbeforereducemay look a bit cleaner.
Object.keys(editor)
.filter(key => PERSISTENT_QUERY_EDITOR_KEYS.has(key))
.reduce((acc, key) => {
// reuse the same acc object instead of spreading into a new object every iteration.
// naming a constant from function param will get you pass the eslint rule no param assignment
const output = acc;
output[key] = editor[key];
return output;
}, {});
There was a problem hiding this comment.
const output = acc;
output[key] = editor[key];
this way you will keep changing the original copy of accumulator, which makes no sense to assign to a new variable output. The point of spread function is to make deep copy of the original object, and when you change the deep copy, the original copy is immutable. The purpose of the eslint rule is, do not change the input param right ? even you write in a way that doesn't break the rule, you still changed input param....
There was a problem hiding this comment.
Yes, I agree that the purpose of the eslint rule is promote immutability over mutation, but since nobody will ever use the intermediate objects I think it might be ok to trade off for less memory usage and garbage collection.
This part is not a blocker for me though.
superset/assets/src/SqlLab/App.jsx
Outdated
| @@ -56,7 +56,10 @@ const sqlLabPersistStateConfig = { | |||
| if (path === 'sqlLab') { | |||
| subset[path] = { | |||
| ...state[path], | |||
| databases: {}, | |||
There was a problem hiding this comment.
should we keep databases around? there aren't that many of them usually
graceguo-supercat
force-pushed
the
gg-ReduceLocalStorage
branch
from
0244ae8
to
643d3da
Compare
Codecov Report
@@ Coverage Diff @@
## master #7998 +/- ##
==========================================
+ Coverage 65.58% 65.62% +0.04%
==========================================
Files 469 469
Lines 22413 22419 +6
Branches 2431 2431
==========================================
+ Hits 14699 14712 +13
+ Misses 7593 7587 -6
+ Partials 121 120 -1
Continue to review full report at Codecov.
|
|
@graceguo-supercat I'm finishing my PR moving all the state to the backend, I should have it for review early next week. |
mistercrunch
added
🏷️ bot
CATEGORY
Choose one
SUMMARY
recent new features increase the size of content in sql lab dropdown list (like tables etc). If we store everything in redux store into localStorage, it's very easy reach localStorage upper limit.
Solution:
remove tables/table options from localStorage.
TEST PLAN
Added unit test for this update.
REVIEWERS
@etr2460 @mistercrunch @michellethomas