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

Moved queriesArray from render() to local state #1505

Merged
merged 3 commits into from
Nov 2, 2016

Conversation

vera-liu
Copy link
Contributor

@vera-liu vera-liu commented Nov 1, 2016

Before:

  • queriesArray is reloaded during each render, but for some re-rendering caused by prop-change like tables, queriesArray doesn't change.

After:

  • queriesArray is moved to local state of TabbedSqlEditors, and is only reloaded during switching tabs or queries object is updated.

needs-review @mistercrunch

is only reloaded only during switching tabs or queries object is updated.
@@ -94,3 +94,18 @@ export function areArraysShallowEqual(arr1, arr2) {
}
return true;
}

export function areObjectsEqual(obj1, obj2) {
Copy link
Member

Choose a reason for hiding this comment

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

I think there's a missing case here where obj2 has properties that are not in obj1.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added length comparison in latest commit

queriesArray.push(this.props.queries[id]);
}
}
this.setState({ queriesArray });
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if it would be kosher to mutate nextProps in componentWillReceiveProps the docs don't seem to say whether it is...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I remember that props are immutable, why would we want to mutate nextProps?

Copy link
Member

Choose a reason for hiding this comment

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

nevermind, that was bad idea!

@mistercrunch
Copy link
Member

LGTM!

@vera-liu vera-liu merged commit 26318f9 into apache:master Nov 2, 2016
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 0.13.0 labels Feb 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 0.13.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants