Skip to content

perf: use shouldComponentUpdate instead of PureComponent in TableRow #289

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

forivall
Copy link

the style prop was always a different object, so PureComponent didnt
have any effect

I tested it in my project using patch-package

patch-package diff
diff --git a/node_modules/react-base-table/es/TableRow.js b/node_modules/react-base-table/es/TableRow.js
index a1fd205..3c4d895 100644
--- a/node_modules/react-base-table/es/TableRow.js
+++ b/node_modules/react-base-table/es/TableRow.js
@@ -4,20 +4,20 @@ import _assertThisInitialized from "@babel/runtime/helpers/assertThisInitialized
 import _inheritsLoose from "@babel/runtime/helpers/inheritsLoose";
 import React from 'react';
 import PropTypes from 'prop-types';
-import { renderElement } from './utils';
+import { isPropsShallowEqual, renderElement } from './utils';
 /**
  * Row component for BaseTable
  */
 
 var TableRow =
 /*#__PURE__*/
-function (_React$PureComponent) {
-  _inheritsLoose(TableRow, _React$PureComponent);
+function (_React$Component) {
+  _inheritsLoose(TableRow, _React$Component);
 
   function TableRow(props) {
     var _this;
 
-    _this = _React$PureComponent.call(this, props) || this;
+    _this = _React$Component.call(this, props) || this;
     _this.state = {
       measured: false
     };
@@ -28,6 +28,10 @@ function (_React$PureComponent) {
 
   var _proto = TableRow.prototype;
 
+  _proto.shouldComponentUpdate = function shouldComponentUpdate(newProps) {
+    return !isPropsShallowEqual(this.props, newProps);
+  };
+
   _proto.componentDidMount = function componentDidMount() {
     this.props.estimatedRowHeight && this.props.rowIndex >= 0 && this._measureHeight(true);
   };
@@ -211,7 +215,7 @@ function (_React$PureComponent) {
   };
 
   return TableRow;
-}(React.PureComponent);
+}(React.Component);
 
 TableRow.defaultProps = {
   tagName: 'div'
diff --git a/node_modules/react-base-table/es/utils.js b/node_modules/react-base-table/es/utils.js
index e18545a..2f72230 100644
--- a/node_modules/react-base-table/es/utils.js
+++ b/node_modules/react-base-table/es/utils.js
@@ -72,6 +72,26 @@ export function isObjectEqual(objA, objB, ignoreFunction) {
 
   return true;
 }
+var hasOwnProperty = Object.prototype.hasOwnProperty;
+export function isPropsShallowEqual(objA, objB) {
+  if (objA === objB) return true;
+  if (objA === null || objB === null) return false;
+  if (typeof objA !== 'object' || typeof objB !== 'object') return false;
+  var keysA = Object.keys(objA);
+  var keysB = Object.keys(objB);
+  if (keysA.length !== keysB.length) return false;
+
+  for (var i = 0; i < keysA.length; i++) {
+    var key = keysA[i];
+    var valueA = objA[key];
+
+    if ((!hasOwnProperty.call(objB, key) || valueA !== objB[key]) && (key !== 'style' || !isObjectEqual(valueA, objB[key]))) {
+      return false;
+    }
+  }
+
+  return true;
+}
 export function callOrReturn(funcOrValue) {
   for (var _len = arguments.length, args = new Array(_len > 1 ? _len - 1 : 0), _key = 1; _key < _len; _key++) {
     args[_key - 1] = arguments[_key];
diff --git a/node_modules/react-base-table/lib/TableRow.js b/node_modules/react-base-table/lib/TableRow.js
index 32ce512..287ff81 100644
--- a/node_modules/react-base-table/lib/TableRow.js
+++ b/node_modules/react-base-table/lib/TableRow.js
@@ -34,8 +34,8 @@ var _utils = require("./utils");
  */
 var TableRow =
 /*#__PURE__*/
-function (_React$PureComponent) {
-  (0, _inherits2["default"])(TableRow, _React$PureComponent);
+function (_React$Component) {
+  (0, _inherits2["default"])(TableRow, _React$Component);
 
   function TableRow(props) {
     var _this;
@@ -51,6 +51,11 @@ function (_React$PureComponent) {
   }
 
   (0, _createClass2["default"])(TableRow, [{
+    key: "shouldComponentUpdate",
+    value: function shouldComponentUpdate(newProps) {
+      return !(0, _utils.isPropsShallowEqual)(this.props, newProps);
+    }
+  }, {
     key: "componentDidMount",
     value: function componentDidMount() {
       this.props.estimatedRowHeight && this.props.rowIndex >= 0 && this._measureHeight(true);
@@ -236,7 +241,7 @@ function (_React$PureComponent) {
     }
   }]);
   return TableRow;
-}(_react["default"].PureComponent);
+}(_react["default"].Component);
 
 TableRow.defaultProps = {
   tagName: 'div'
diff --git a/node_modules/react-base-table/lib/utils.js b/node_modules/react-base-table/lib/utils.js
index 4b7daed..cef2e6e 100644
--- a/node_modules/react-base-table/lib/utils.js
+++ b/node_modules/react-base-table/lib/utils.js
@@ -8,6 +8,7 @@ Object.defineProperty(exports, "__esModule", {
 exports.renderElement = renderElement;
 exports.normalizeColumns = normalizeColumns;
 exports.isObjectEqual = isObjectEqual;
+exports.isPropsShallowEqual = isPropsShallowEqual;
 exports.callOrReturn = callOrReturn;
 exports.hasChildren = hasChildren;
 exports.unflatten = unflatten;
@@ -101,6 +102,28 @@ function isObjectEqual(objA, objB) {
   return true;
 }
 
+var hasOwnProperty = Object.prototype.hasOwnProperty;
+
+function isPropsShallowEqual(objA, objB) {
+  if (objA === objB) return true;
+  if (objA === null || objB === null) return false;
+  if ((0, _typeof2["default"])(objA) !== 'object' || (0, _typeof2["default"])(objB) !== 'object') return false;
+  var keysA = Object.keys(objA);
+  var keysB = Object.keys(objB);
+  if (keysA.length !== keysB.length) return false;
+
+  for (var i = 0; i < keysA.length; i++) {
+    var key = keysA[i];
+    var valueA = objA[key];
+
+    if ((!hasOwnProperty.call(objB, key) || valueA !== objB[key]) && (key !== 'style' || !isObjectEqual(valueA, objB[key]))) {
+      return false;
+    }
+  }
+
+  return true;
+}
+
 function callOrReturn(funcOrValue) {
   for (var _len = arguments.length, args = new Array(_len > 1 ? _len - 1 : 0), _key = 1; _key < _len; _key++) {
     args[_key - 1] = arguments[_key];

An alternate monkey patching workaround:

class FasterBaseTable<T> extends BaseTable<T> {
  rowStyles: RowStyles = {};
  renderRow(params: RenderRowParams) {
    const rowKeyProp = this.props.rowKey;
    const rowKey = rowKeyProp ? params.rowData[rowKeyProp] : params.rowIndex;
    let newStyle = params.style;
    if (this.rowStyles) {
      const prevStyle = this.rowStyles[rowKey];
      if (prevStyle && shallowEqual(newStyle, prevStyle)) {
        newStyle = prevStyle!;
        params.style = newStyle;
      }
    }
    this.rowStyles[rowKey] = newStyle;
    return super.renderRow(params);
  }
  render() {
    this.rowStyles = {};
    return super.render();
  }
}

the `style` prop was always a different object, so PureComponent didnt
have any effect
@nihgwu
Copy link
Contributor

nihgwu commented Mar 31, 2021

do you have performance issue? style is an object, so I don't allow custom style from rowProps, it will be override by the one from react-window which would cache style while scrolling

@forivall
Copy link
Author

forivall commented Apr 5, 2021

yes, i am having performance issues, and i'm noticing that the row is re-rendered on every scroll, seen using the react-devtools "highlight re-renders" option. react-window doesn't seem to be caching the style then...

@showme79
Copy link

@forivall : I have tried your patch, but it is killing BaseTable.forceUpdateTable method. To make it work again you need to make a "rowGeneration" state (an empty object or a number) which is changed in forceUpdateTable and passed to TableRow in rowRenderer.

@@ -18,6 +18,10 @@ class TableRow extends React.PureComponent {
this._handleExpand = this._handleExpand.bind(this);
}

shouldComponentUpdate(newProps) {
return !isPropsShallowEqual(this.props, newProps);
Copy link

@showme79 showme79 Mar 2, 2022

Choose a reason for hiding this comment

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

state change (eg. row measuring progress) is not detected here:

shouldComponentUpdate(newProps, newState) {
    return newState !== this.state && !isPropsShallowEqual(this.props, newProps);
}

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants