[sqllab] optimizing React #1438
Conversation
|
Looks good to me. |
mistercrunch
force-pushed
the
optimiz_sqllab
branch
from
eb869e2
to
bf0813a
Compare
| @@ -27,7 +27,7 @@ const defaultProps = { | |||
| }; | |||
|
|
|||
|
|
|||
| class QueryTable extends React.Component { | |||
| class QueryTable extends React.PureComponent { | |||
There was a problem hiding this comment.
what happens when we extend PureComponent rather than Component and don't use componentShouldUpdate method?
There was a problem hiding this comment.
PureComponent implements its own shouldComponentUpdate that checks for shallow difference in this.state.* and this.props.* against the next values.
| @@ -27,7 +27,7 @@ const defaultProps = { | |||
| }; | |||
|
|
|||
|
|
|||
| class QueryTable extends React.Component { | |||
| class QueryTable extends React.PureComponent { | |||
| }; | ||
|
|
||
| const defaultProps = { | ||
| sql: '', |
There was a problem hiding this comment.
since sql is required we don't need a default prop here
| @@ -74,3 +74,22 @@ export function enhancer() { | |||
| } | |||
| return enhancerWithPersistState; | |||
| } | |||
|
|
|||
| export function areArrayDifferent(arr1, arr2) { | |||
There was a problem hiding this comment.
i might name this areArraysEqual rather than different. i think testing for equality is a little more clear/common pattern.
There was a problem hiding this comment.
I went with different because it matches shouldComponentUpdate's bool. I can flip it.
There was a problem hiding this comment.
going with areArraysShallowEqual
| } | ||
| const length = arr1.length; | ||
| for (let i = 0; i < length; i++) { | ||
| if (arr1[i] !== arr2[i]) { |
There was a problem hiding this comment.
will the arrays to be compared ever be nested arrays?
There was a problem hiding this comment.
if we need to compare deep equality, might be useful to use either https://github.com/substack/node-deep-equal or https://github.com/lodash/lodash/tree/4.4.0-npm-packages/lodash.isequal
There was a problem hiding this comment.
the new name areArraysShallowEqual makes it clear that this function doesn't recurse
| editorProps={{ $blockScrolling: true }} | ||
| enableBasicAutocompletion | ||
| value={this.props.queryEditor.sql} | ||
| <AceEditorWrapper |
There was a problem hiding this comment.
it's common in react to use container rather than wrapper, but nbd.
| textChange(text) { | ||
| this.setState({ sql: text }); | ||
| this.props.actions.queryEditorSetSql(this.props.queryEditor, text); | ||
| sqlToStore(sql) { |
There was a problem hiding this comment.
could this be called setQueryEditorSql for clarity?
|
+1 to performance improvements! |
|
Addressed all comments, mergin'! |
|
💯 |
* Update controlPanel.tsx * Update index.tsx * Update dndControls.tsx
* Update controlPanel.tsx * Update index.tsx * Update dndControls.tsx
* Update controlPanel.tsx * Update index.tsx * Update dndControls.tsx
* Update controlPanel.tsx * Update index.tsx * Update dndControls.tsx
mistercrunch
added
🏷️ bot
@ascott @vera-liu @bkyryliuk
The UI is much more snappy after these changes. I noticed keystrokes in the editor were getting quite laggy, incrementally as the query history would grow or when the result set was large.
The big win is to not re-render the whole page at each keystroke in the AceEditor by wrapping it in its own component and managing local state there: affecting the store only onBlur events.
I also made pretty much every component a PureComponent which reduces the amount of rendering dramatically.
@ascott I'm doing this trick with
shouldComponentUpdateto work around the fact that denormalizing the query array in the "controller"'s rendering method (to go from a global object in the store, to editor-specific array of queries) always creates a new array object. I made this functionareArraysDifferentwhich looks inside the array for comparison. Any cleaner way to do this? It seems like it would be a common problem when denormalizing. Maybe keeping the array in the controller's state and handling the mutation there only if necessary?