Skip to content
This repository has been archived by the owner on Jun 26, 2020. It is now read-only.

"Highlight Updates" gives false positives for a performance tuned React app #1023

Closed
nareshbhatia opened this issue Apr 30, 2018 · 3 comments

Comments

@nareshbhatia
Copy link

I have created a minimal application that displays a list of 10 todo's using Redux. One of the todo's is updated every second. The app has been optimized using recommendations in Redux Performance FAQ. Indeed when I run the app, I can see in console logs that the list is rendered only once at application startup, and only one todo is rendered every second. However, React DevTools highlights the entire list every second.

I have created a minimal example to demonstrate the issue: https://github.com/nareshbhatia/redux-performance. Could someone please look at the example and confirm if this is a real issue with React DevTools or just a cockpit error!

@gaearon
Copy link
Contributor

gaearon commented May 1, 2018

You are preventing your components from re-rendering, but connect() wrappers generated by React Redux still re-render. I think you might have misinterpreted the advice (not sure because I haven’t read that FAQ): what you need to do instead is to make sure mapStateToProps result is shallowly equal for things that don‘t change.

Here is a diff to give you an idea:

diff --git a/src/components/todo-view.js b/src/components/todo-view.js
index 9559a04..a3c0ac6 100644
--- a/src/components/todo-view.js
+++ b/src/components/todo-view.js
@@ -2,24 +2,12 @@ import React from 'react';
 import { PropTypes } from 'prop-types';
 
 export class TodoView extends React.Component {
-    static propTypes = {
-        todoId: PropTypes.number.isRequired,
-        todoMap: PropTypes.object.isRequired
-    };
-
-    shouldComponentUpdate(nextProps) {
-        const { todoId, todoMap } = this.props;
-        const { todoMap: nextTodoMap } = nextProps;
-        return todoMap[todoId] !== nextTodoMap[todoId];
-    }
-
     render() {
-        const { todoId, todoMap } = this.props;
-        console.log('-----> TodoView.render():', todoId);
+        const { todo } = this.props;
 
         return (
             <tr>
-                <td >{todoMap[todoId]}</td>
+                <td >{todo}</td>
             </tr>
         );
     }
diff --git a/src/containers/todo-view-container.js b/src/containers/todo-view-container.js
index b3930f0..ad3e37f 100644
--- a/src/containers/todo-view-container.js
+++ b/src/containers/todo-view-container.js
@@ -1,8 +1,8 @@
 import { connect } from 'react-redux';
 import { TodoView } from '../components/todo-view';
 
-const mapStateToProps = state => ({
-    todoMap: state.todoMap
+const mapStateToProps = (state, ownProps) => ({
+    todo: state.todoMap[ownProps.todoId]
 });
 
 export const TodoViewContainer = connect(mapStateToProps)(TodoView);

cc @markerikson in case something in the FAQ could use a clarification

@gaearon gaearon closed this as completed May 1, 2018
@markerikson
Copy link

Thanks for the ping. I don't think there's anything specific that needs to be edited, but @nareshbhatia , feel free to file an issue on the Redux repo if you feel it's insufficiently informative.

@nareshbhatia
Copy link
Author

Thanks @gaearon & @markerikson for your help. I think the docs are very clear. The only trick that I did not think of (as suggested by @gaearon) is to use ownProps to resolve the todo right inside the container (instead of passing the entire map into the view). This was the root cause of the inefficient rendering. I have fixed my repo based on your suggestion. Hopefully it is educational for others in the future.

Thanks again!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants