graceguo-supercat commented on a change in pull request #4459: [Explore] highlighting run query when chart is stale on explore view URL: https://github.com/apache/incubator-superset/pull/4459#discussion_r170382897
########## File path: superset/assets/javascripts/explore/components/ExploreViewContainer.jsx ########## @@ -121,14 +126,22 @@ class ExploreViewContainer extends React.Component { return `${window.innerHeight - navHeight}px`; } - instantControlChanged(prevControls, currentControls) { + hasDisplayControlChanged(prevControls, currentControls) { return Object.keys(currentControls).some(key => ( currentControls[key].renderTrigger && typeof prevControls[key] !== 'undefined' && !areObjectsEqual(currentControls[key].value, prevControls[key].value) )); } + hasQueryControlChanged(prevControls, currentControls) { Review comment: `hasDisplayControlChange` and `hasQueryControlChange` has very similar logic, can you consolidate this 2 functions? Notice both 2 functions called `areObjectsEqual`, which is a function that stringify 2 JS object and then find diff. Similar to JSON.parse, this function call is not cheap, so we need to use it with caution. Also in componentWillReceiveProps we have a few logic block, it checks if viz_type changed, if data source changed, if control props changed, etc. The call to action creator, this.props.actions.ABCD, is not exit or return. This means every time there is a props change, **all** of these logic will be checked. So if we do not consolidate 2 functions, the expensive compare function will be called twice every time there is a props change, no matter what kind of prop change. ---------------------------------------------------------------- This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services