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

perf: use shouldComponentUpdate instead of PureComponent in TableRow #289

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);
}

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

Successfully merging this pull request may close these issues.

None yet

3 participants