feat: add ignoreFunctionInColumnCompare to solve closure problem in renderers (#213)

* feat: add ignoreFunctionInColumnCompare to solve closure problem in renderers

* add example

Author: nihgwu

CHANGELOG.md

@@ -2,6 +2,8 @@
 
 ## NEXT VERSION
 
+- feat: add `ignoreFunctionInColumnCompare` to solve closure problem in renderers
+
 ## v1.10.9 (2020-08-13)
 
 - fix: input loses focus on unmount

README.md

@@ -34,16 +34,32 @@ import 'react-base-table/styles.css'
 
 Learn more at the [website](https://autodesk.github.io/react-base-table/)
 
-**Make sure each item in `data` is unique by a key, the default key is `id`, you can customize it via `rowKey`**
+### unique key
 
-**`key` is required for column definition or the column will be ignored**
+`key` is required for column definition or the column will be ignored
 
-**`width` and `height`(or `maxHeight`) are required to display the table properly**
+Make sure each item in `data` is unique by a key, the default key is `id`, you can customize it via `rowKey`
+
+### size
+
+`width` is required for column definition, but in flex mode(`fixed={false}`), you can set `width={0}` and `flexGrow={1}` to achieve flexible column width, checkout the [Flex Column](https://autodesk.github.io/react-base-table/examples/flex-column) example
+
+`width` and `height`(or `maxHeight`) are required to display the table properly
 
 In the [examples](https://autodesk.github.io/react-base-table/examples)
 we are using a wrapper `const Table = props => <BaseTable width={700} height={400} {...props} />` to do that
 
-If you want it responsive, you can use the [`AutoResizer`](https://autodesk.github.io/react-base-table/api/autoresizer) to make the table fill the container, checkout the [Auto Resize example](https://autodesk.github.io/react-base-table/examples/auto-resize)
+If you want it responsive, you can use the [`AutoResizer`](https://autodesk.github.io/react-base-table/api/autoresizer) to make the table fill the container, checkout the [Auto Resize](https://autodesk.github.io/react-base-table/examples/auto-resize) example
+
+### closure problem in custom renderers
+
+In practice we tend to write inline functions for custom renderers, which would make `shouldUpdateComponent` always true as the inline function will create a new instance on every re-render, to avoid "unnecessary" re-renders, **`BaseTable` ignores functions when comparing column definition by default**, it works well in most cases before, but if we use external data instead of reference state in custom renderers, we always get the staled initial value although the data has changed
+
+It's recommended to inject the external data in column definition to solve the problem, like `<Column foo={foo} bar={bar} cellRenderer={({ column: { foo, bar }}) => { ... } } />`, the column definition will update on external data change, with this pattern we can easily move the custom renderers out of column definition for sharing, the downside is it would bloat the column definition and bug prone
+
+Things getting worse with the introduction of React hooks, we use primitive state instead of `this.state`, so it's easy to encounter the closure problem, but with React hooks, we can easily memoize functions via `useCallback` or `useMemo`, so the implicit optimization could be replaced with user land optimization which is more intuitive, to turn off the implicit optimization, set `ignoreFunctionInColumnCompare` to `false` which is introduced since `v1.11.0`
+
+Here is an [example](https://autodesk.github.io/react-base-table/playground#MYewdgzgLgBKA2BXAtpGBeGBzApmHATgIZQ4DCISqEAFAIwAMAlAFCiSwAmJRG2ehEjgAiPGghSQANDABMDZixY4AHgAcQBLjgBmRRPFg0mGAHwwA3ixhxw0GAG1QiMKQIyIOKBRduAunwASjhEwFAAdIieAMpQQjSKNuz2DgCWWGCaOABiLmGp4B5eAJIZWblg+eABmMGhEVE4sfFQBIg4rEl2sGlgAFY4YRRUYEVQxf2D3pSSNTB1YZExcaQ0evCenTAEXogEYDA01jYwADymxydnnKkAbjDQAJ7wOOgWFjBqRJw3YFgAXDAACwyG4QNTwIiPQEAch0LxUMJkfSiUFSOkeFFceCgsPBoRwAFoAEZeADuODwMJgAF8aRcrldTsTEFAoOAYOAyPBUsAANZvYxmB5eHzYgjiEC+QgwADUMDoTHpstOAHoWWzwAzGTZTpDSfBtTrdakwGpWZdjTYoI81K8AETAAAWgz5xJAKntlqtztdOE4b3SmR2FSqYBp3uNXKdRD+rwsOGFnnGZRDeTR4BoOHCcQIuAivv5-qVkauqqNxtKwcTOnTBQOptsI1syC+O1LZ1V+pwho7eqIBorOtOpvNUA7VxtdvQjpd-PdnonJ0LfP9gcmQxmqAjVqu0djuDeifQ5mTEwGm5GWZzRDzXnCK+LO935aX56mMFUrV43DiMHZTaSDAnC6KaqQZmAfZdgOPZDp2Ny3HBpwACoDi8MA6KkKj+sBPBvL+RA0jAQblHW4ATMMkgUK2t7xiRaaVBB9J9pRqBLhY4ScRI1AOAwfjPlaBEAOJeG4gomCetjSgQAnGs44rrhe0zNgA-FJ4owICLggZh+CcLJZZwbqrEHBxXFbpADh0PxMCvlapwmZYnEPhZEAOLINl2caDkWU55kjG5ADMnlGWcjlmS5AUOECIWRmqqHEi8FZqtqrARkAA) to demonstrate 
 
 ## Browser Support
 
@@ -117,7 +133,7 @@ We are using a advanced table component based on `BaseTable` internally, with mu
 
 ![AdvanceTable](screenshots/advance-table.png)
 
-[In real products](https://blogs.autodesk.com/bim360-release-notes/2019/11/18/bim-360-cost-management-update-november-2019/) 
+[In real products](https://blogs.autodesk.com/bim360-release-notes/2019/11/18/bim-360-cost-management-update-november-2019/)
 
 ## Development
 

src/BaseTable.js

@@ -97,18 +97,21 @@ class BaseTable extends React.PureComponent {
       this._depthMap = {};
       return flattenOnKeys(tree, keys, this._depthMap, dataKey);
     });
-    this._resetColumnManager = memoize((columns, fixed) => {
-      this.columnManager.reset(columns, fixed);
+    this._resetColumnManager = memoize(
+      (columns, fixed) => {
+        this.columnManager.reset(columns, fixed);
 
-      if (this.props.estimatedRowHeight && fixed) {
-        if (!this.columnManager.hasLeftFrozenColumns()) {
-          this._leftRowHeightMap = {};
-        }
-        if (!this.columnManager.hasRightFrozenColumns()) {
-          this._rightRowHeightMap = {};
+        if (this.props.estimatedRowHeight && fixed) {
+          if (!this.columnManager.hasLeftFrozenColumns()) {
+            this._leftRowHeightMap = {};
+          }
+          if (!this.columnManager.hasRightFrozenColumns()) {
+            this._rightRowHeightMap = {};
+          }
         }
-      }
-    }, isObjectEqual);
+      },
+      (newArgs, lastArgs) => isObjectEqual(newArgs, lastArgs, this.props.ignoreFunctionInColumnCompare)
+    );
 
     this._isResetting = false;
     this._resetIndex = null;
@@ -1033,6 +1036,7 @@ BaseTable.defaultProps = {
   overscanRowCount: 1,
   onEndReachedThreshold: 500,
   getScrollbarSize: defaultGetScrollbarSize,
+  ignoreFunctionInColumnCompare: true,
 
   onScroll: noop,
   onRowsRendered: noop,
@@ -1283,6 +1287,10 @@ BaseTable.propTypes = {
    * Each of the handlers is of the shape of `({ rowData, rowIndex, rowKey, event }) => object`
    */
   rowEventHandlers: PropTypes.object,
+  /**
+   * whether to ignore function properties while comparing column definition
+   */
+  ignoreFunctionInColumnCompare: PropTypes.bool,
   /**
    * A object for the custom components, like `ExpandIcon` and `SortIndicator`
    */

src/utils.js

@@ -26,7 +26,7 @@ export function normalizeColumns(elements) {
   return columns;
 }
 
-export function isObjectEqual(objA, objB) {
+export function isObjectEqual(objA, objB, ignoreFunction = true) {
   if (objA === objB) return true;
   if (objA === null && objB === null) return true;
   if (objA === null || objB === null) return false;
@@ -38,13 +38,22 @@ export function isObjectEqual(objA, objB) {
 
   for (let i = 0; i < keysA.length; i++) {
     const key = keysA[i];
+
+    if (key === '_owner' && objA.$$typeof) {
+      // React-specific: avoid traversing React elements' _owner.
+      //  _owner contains circular references
+      // and is not needed when comparing the actual elements (and not their owners)
+      continue;
+    }
+
     const valueA = objA[key];
     const valueB = objB[key];
+    const valueAType = typeof valueA;
 
-    if (typeof valueA !== typeof valueB) return false;
-    if (typeof valueA === 'function') continue;
-    if (typeof valueA === 'object') {
-      if (!isObjectEqual(valueA, valueB)) return false;
+    if (valueAType !== typeof valueB) return false;
+    if (valueAType === 'function' && ignoreFunction) continue;
+    if (valueAType === 'object') {
+      if (!isObjectEqual(valueA, valueB, ignoreFunction)) return false;
       else continue;
     }
     if (valueA !== valueB) return false;