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.

Sign up for GitHub

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

Jump to bottom

Improve DataTable re-rendering performance #2993

Merged
merged 5 commits into from
Feb 11, 2022
Merged

Improve DataTable re-rendering performance #2993

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
1b99eb0
Improve DataTable rerendering performance
Jan 27, 2022
e21f5ae
Remove lodash.memoize
Feb 10, 2022
6d4ca7d
Merge branch 'master' into datatable-perf
Feb 10, 2022
60a51fe
Merge branch 'master' into datatable-perf
interactivellama Feb 11, 2022
7172226
Merge branch 'master' into datatable-perf
interactivellama Feb 11, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 4 additions & 0 deletions .eslintrc.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ module.exports = {
'json',
'prefer-object-spread',
'@salesforce/slds-react',
'react-hooks',
],
env: {
browser: true,
Expand Down Expand Up @@ -195,6 +196,9 @@ module.exports = {
// 'fp/no-unused-expression': 'error',
'fp/no-valueof-field': 'error',

'react-hooks/rules-of-hooks': 'error',
'react-hooks/exhaustive-deps': 'error',

//
// THE FOLLOWING RULES NEED REVIEW IN THE FUTURE (and possibly removed)
//
Expand Down
1 change: 0 additions & 1 deletion components/button/__tests__/button.browser-test.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,6 @@ describe('SLDSButton: ', () => {
style: { background: 'rgb(18, 49, 35)' },
});
[btn] = cmp.getElementsByClassName('slds-button');
console.log('!!!!!', cmp, btn);
});

it('renders correct label', () => {
Expand Down
4 changes: 2 additions & 2 deletions components/data-table/cell.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ import classNames from 'classnames';

import CellContext from './private/cell-context';
import TableContext from './private/table-context';
import contextHelper from './private/context-helper';
import useContextHelper from './private/context-helper';

// ## Constants
import { DATA_TABLE_CELL } from '../../utilities/constants';
Expand All @@ -21,7 +21,7 @@ import { DATA_TABLE_CELL } from '../../utilities/constants';
const DataTableCell = (props) => {
const tableContext = useContext(TableContext);
const cellContext = useContext(CellContext);
const { tabIndex, hasFocus, handleFocus, handleKeyDown } = contextHelper(
const { tabIndex, hasFocus, handleFocus, handleKeyDown } = useContextHelper(
tableContext,
cellContext,
props.fixedLayout
Expand Down
215 changes: 130 additions & 85 deletions components/data-table/index.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,8 +16,9 @@ import shortid from 'shortid';

import classNames from 'classnames';
import assign from 'lodash.assign';
import isEqual from 'lodash.isequal';
import memoize from 'memoize-one';
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The problem with using lodash.memoize is it only bases its memoization on the first argument supplied to the function by default, and you need to hand-write the cache key calculation otherwise. memoize-one is free of that shortcoming and is a popular library that's lightweight and performant.

I think there is only one usage of lodash.memoize that I could remove if necessary.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, one memoize library would be great!

import reject from 'lodash.reject';

// This component's `checkProps` which issues warnings to developers about properties when in development mode (similar to React's built in development tools)
import ColumnResizer from 'column-resizer';
import checkProps from './check-props';
Expand Down Expand Up @@ -74,6 +75,90 @@ const defaultProps = {
},
};

const getAssistiveText = memoize(
(
assistiveText,
actionsHeaderText,
columnSortText,
columnSortedAscendingText,
columnSortedDescendingText,
selectAllRowsText,
selectRowText
) => {
const result = {
...defaultProps.assistiveText,
...assistiveText,
};
if (actionsHeaderText) {
result.actionsHeader = actionsHeaderText;
}
if (selectAllRowsText) {
result.selectAllRows = selectAllRowsText;
}
if (columnSortedAscendingText) {
result.columnSortedAscending = columnSortedAscendingText;
}
if (columnSortedDescendingText) {
result.columnSortedDescending = columnSortedDescendingText;
}
if (columnSortText) {
result.columnSort = columnSortText;
}
if (selectRowText) {
result.selectRow = selectRowText;
}
return result;
},
isEqual
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because assistiveText is an object rather than a scalar value, we need to provide a comparison function

);

const getColumnsAndRowActions = memoize(
(children, id, fixedHeader, fixedLayout, search) => {
const columns = [];
let RowActions = null;

React.Children.forEach(children, (child) => {
if (child && child.type.displayName === DataTableColumn.displayName) {
const { children: columnChildren, ...columnProps } = child.props;
const props = { fixedLayout, search, id, ...columnProps };

let Cell;
if (
columnChildren &&
columnChildren.type.displayName === DATA_TABLE_CELL
) {
Cell = columnChildren.type;
assign(props, columnChildren.props);
} else {
Cell = DataTableCell;
}

// eslint-disable-next-line fp/no-mutating-methods
columns.push({
Cell,
props,
});
} else if (
child &&
child.type.displayName === DataTableRowActions.displayName
) {
const { dropdown } = child.props;
const dropdownPropOverrides = {};
if (fixedHeader) {
dropdownPropOverrides.menuPosition = 'overflowBoundaryElement';
}
RowActions = React.cloneElement(child, {
dropdown: dropdown
? React.cloneElement(dropdown, dropdownPropOverrides)
: null,
});
}
});
return { columns, RowActions };
},
isEqual
);

/**
* DataTables support the display of structured data in rows and columns with an HTML table. To sort, filter or paginate the table, simply update the data passed in the items to the table and it will re-render itself appropriately. The table will throw a sort event as needed, and helper components for paging and filtering are coming soon.
*
Expand Down Expand Up @@ -366,9 +451,7 @@ class DataTable extends React.Component {
// Simulating a scroll here will ensure that enough rows are loaded to enable scrolling
this.loadMoreIfNeeded();
}
if (this.props.items !== prevProps.items) {
this.interactiveElements = {};
}

if (
this.state.allowKeyboardNavigation &&
!prevState.allowKeyboardNavigation
Expand Down Expand Up @@ -440,6 +523,23 @@ class DataTable extends React.Component {
return null;
}

getTableContext = memoize((state, isKeyboardNavigation) => ({
activeCell: state.activeCell,
activeElement: state.activeElement,
mode: state.mode,
tableHasFocus: state.tableHasFocus,
changeActiveCell: this.changeActiveCell,
changeActiveElement: this.changeActiveElement,
handleKeyDown: this.handleKeyDown,
registerInteractiveElement: this.registerInteractiveElement,
allowKeyboardNavigation: state.allowKeyboardNavigation,
setAllowKeyboardNavigation: (allowKeyboardNavigation) => {
if (isKeyboardNavigation) {
this.setState({ allowKeyboardNavigation });
}
},
}));

handleToggleAll = (e, { checked }) => {
// REMOVE AT NEXT BREAKING CHANGE
// `onChange` is deprecated and replaced with `onRowChange`
Expand Down Expand Up @@ -587,6 +687,13 @@ class DataTable extends React.Component {
}
};

// eslint-disable-next-line camelcase
UNSAFE_componentWillUpdate(nextProps) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is fixing an existing issue (not related to performance) where navigating cells via keyboard and pressing spacebar wasn't triggering the interactive element.
I spent limited amount of time fixing this because it's out of scope so I didn't look into ways of avoiding an unsafe lifecycle.

if (this.props.items !== nextProps.items) {
this.interactiveElements = {};
}
}

isResizable() {
return this.props.fixedLayout && this.props.resizable;
}
Expand Down Expand Up @@ -989,71 +1096,24 @@ class DataTable extends React.Component {
const allSelected = canSelectRows && numNonHeaderRows === numSelected;
const indeterminateSelected =
canSelectRows && numNonHeaderRows !== numSelected && numSelected !== 0;
const columns = [];
let RowActions = null;

React.Children.forEach(this.props.children, (child) => {
if (child && child.type.displayName === DataTableColumn.displayName) {
const { children, ...columnProps } = child.props;

const props = assign({}, this.props);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This essentially propagates all table props onto columns. In the extracted function I replaced this with individual props that are actually used. Passing more props than necessary can exacerbate the issue with unnecessary re-renders.

// eslint-disable-next-line fp/no-delete
delete props.children;
assign(props, columnProps);

let Cell;
if (children && children.type.displayName === DATA_TABLE_CELL) {
Cell = children.type;
assign(props, children.props);
} else {
Cell = DataTableCell;
}

// eslint-disable-next-line fp/no-mutating-methods
columns.push({
Cell,
props,
dataTableProps: this.props,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

When extracting this code, I removed this property since dataTableProps were not being used.

});
} else if (
child &&
child.type.displayName === DataTableRowActions.displayName
) {
const { dropdown } = child.props;
const dropdownPropOverrides = {};
if (this.getFixedHeader()) {
dropdownPropOverrides.menuPosition = 'overflowBoundaryElement';
}
RowActions = React.cloneElement(child, {
dropdown: dropdown
? React.cloneElement(dropdown, dropdownPropOverrides)
: null,
});
}
});
const { columns, RowActions } = getColumnsAndRowActions(
this.props.children,
this.props.id,
this.getFixedHeader(),
this.props.fixedLayout,
this.props.search
);

const assistiveText = {
...defaultProps.assistiveText,
...this.props.assistiveText,
};
if (this.props.assistiveTextForActionsHeader) {
assistiveText.actionsHeader = this.props.assistiveTextForActionsHeader;
}
if (this.props.assistiveTextForSelectAllRows) {
assistiveText.selectAllRows = this.props.assistiveTextForSelectAllRows;
}
if (this.props.assistiveTextForColumnSortedAscending) {
assistiveText.columnSortedAscending = this.props.assistiveTextForColumnSortedAscending;
}
if (this.props.assistiveTextForColumnSortedDescending) {
assistiveText.columnSortedDescending = this.props.assistiveTextForColumnSortedDescending;
}
if (this.props.assistiveTextForColumnSort) {
assistiveText.columnSort = this.props.assistiveTextForColumnSort;
}
if (this.props.assistiveTextForSelectRow) {
assistiveText.selectRow = this.props.assistiveTextForSelectRow;
}
const assistiveText = getAssistiveText(
this.props.assistiveText,
this.props.assistiveTextForActionsHeader,
this.props.assistiveTextForSelectAllRows,
this.props.assistiveTextForColumnSortedAscending,
this.props.assistiveTextForColumnSortedDescending,
this.props.assistiveTextForColumnSort,
this.props.assistiveTextForSelectRow
);

if (this.props.selectRows && this.props.selectRows !== 'radio') {
ariaProps['aria-multiselectable'] = 'true';
Expand All @@ -1066,26 +1126,11 @@ class DataTable extends React.Component {
select: canSelectRows ? this.headerRefs.select : [],
};

const tableContext = {
activeCell: this.state.activeCell,
activeElement: this.state.activeElement,
mode: this.state.mode,
tableHasFocus: this.state.tableHasFocus,
changeActiveCell: this.changeActiveCell,
changeActiveElement: this.changeActiveElement,
handleKeyDown: this.handleKeyDown,
registerInteractiveElement: this.registerInteractiveElement,
allowKeyboardNavigation: this.state.allowKeyboardNavigation,
setAllowKeyboardNavigation: (allowKeyboardNavigation) => {
if (this.getKeyboardNavigation()) {
this.setState({ allowKeyboardNavigation });
}
},
};

let component = (
<React.Fragment>
<TableContext.Provider value={tableContext}>
<TableContext.Provider
value={this.getTableContext(this.state, this.getKeyboardNavigation())}
>
<table
{...ariaProps}
className={classNames(
Expand Down
40 changes: 26 additions & 14 deletions components/data-table/private/context-helper.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,30 +1,42 @@
/* Copyright (c) 2015-present, salesforce.com, inc. All rights reserved */
/* Licensed under BSD 3-Clause - see LICENSE.txt or git.io/sfdc-license */

import { useCallback } from 'react';

/**
* Calculates data table keyboard navigation state based on currently selected cell
*/
export default (tableContext, cellContext, fixedLayout) => {
export default function useTableContextHelper(
tableContext,
cellContext,
fixedLayout
) {
const isActive =
tableContext.activeCell.rowIndex === cellContext.rowIndex &&
tableContext.activeCell.columnIndex === cellContext.columnIndex;

const hasFocus = fixedLayout && tableContext.tableHasFocus && isActive;

const handleFocus = () => {
const { changeActiveCell, handleKeyDown: handleTableKeyDown } = tableContext;
const handleFocus = useCallback(() => {
if (fixedLayout && tableContext.allowKeyboardNavigation) {
tableContext.changeActiveCell(
cellContext.rowIndex,
cellContext.columnIndex
);
changeActiveCell(cellContext.rowIndex, cellContext.columnIndex);
}
};
}, [
fixedLayout,
tableContext.allowKeyboardNavigation,
changeActiveCell,
cellContext.rowIndex,
cellContext.columnIndex,
]);

const handleKeyDown = (event) => {
if (fixedLayout && tableContext.allowKeyboardNavigation) {
tableContext.handleKeyDown(event);
}
};
const handleKeyDown = useCallback(
(event) => {
if (fixedLayout && tableContext.allowKeyboardNavigation) {
handleTableKeyDown(event);
}
},
[fixedLayout, tableContext.allowKeyboardNavigation, handleTableKeyDown]
);

const tabIndex =
fixedLayout &&
Expand All @@ -35,4 +47,4 @@ export default (tableContext, cellContext, fixedLayout) => {
: undefined;

return { tabIndex, hasFocus, handleFocus, handleKeyDown };
};
}