Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
38 commits
Select commit Hold shift + click to select a range
845460d
Optimize how many reports render at one time
tgolen Sep 15, 2020
f447382
Add some comments to explain the pure component
tgolen Sep 15, 2020
fbab50d
Optimize the rendering of report actions
tgolen Sep 15, 2020
8fa9a37
Add HOC for batched rendering
tgolen Sep 15, 2020
4457014
Merge branch 'master' into tgolen-first-render
tgolen Sep 15, 2020
8ed7f31
Remove initial rendering logic
tgolen Sep 15, 2020
ceb3c90
Merge remote-tracking branch 'origin' into tgolen-first-render
marcaaron Sep 16, 2020
9c5cea5
Merge branch 'master' into tgolen-first-render
tgolen Sep 16, 2020
8160cec
Improve comment
tgolen Sep 16, 2020
5891973
Remove an extra view
tgolen Sep 16, 2020
901585a
Improve the batch definitions
tgolen Sep 16, 2020
f6672dd
Improve props
tgolen Sep 16, 2020
4f1a229
Rename comments to indicate where props are coming from
tgolen Sep 16, 2020
c0a3ded
Add some comments for how the HOC is used
tgolen Sep 16, 2020
3f77ea9
Refactor URL storage in Ion a little
tgolen Sep 16, 2020
6293c58
Revert "Refactor URL storage in Ion a little"
tgolen Sep 16, 2020
41fa8a1
Fix indexing
tgolen Sep 16, 2020
9d9a2f8
Merge branch 'tgolen-first-render' of https://github.com/Expensify/Re…
marcaaron Sep 16, 2020
e0c2063
Have batch rendering work on normal component updates
tgolen Sep 17, 2020
fe7ddf9
Merge branch 'tgolen-first-render' of https://github.com/Expensify/Re…
marcaaron Sep 17, 2020
a7388c3
Prevent batches from overwriting existing props better
tgolen Sep 17, 2020
8d6f0cf
Moar comments
tgolen Sep 17, 2020
35824f1
Merge branch 'tgolen-first-render' of https://github.com/Expensify/Re…
marcaaron Sep 17, 2020
971442e
Remove extra prop
tgolen Sep 21, 2020
4cf1e06
Remove componentDidUpdate
tgolen Sep 21, 2020
d297927
Merge branch 'master' into tgolen-first-render
tgolen Sep 21, 2020
b189b8c
Put back componentDidUpdate
tgolen Sep 21, 2020
1ac8155
Merge branch 'tgolen-first-render' of https://github.com/Expensify/Re…
marcaaron Oct 15, 2020
ae0406a
fix conflicts
marcaaron Oct 15, 2020
f344c2b
fix conflicts
marcaaron Oct 15, 2020
34a0fa6
clean up and add some comments
marcaaron Oct 15, 2020
7fc19f5
remove some changes to clear up diff a bit
marcaaron Oct 15, 2020
c4e22d4
fix another diff
marcaaron Oct 15, 2020
9806698
more clean up
marcaaron Oct 15, 2020
0559a57
fix style
marcaaron Oct 15, 2020
2f718e3
fix conflicts
marcaaron Oct 15, 2020
1230443
fix bad merge
marcaaron Oct 15, 2020
5faccd6
just use single line instead of method;
marcaaron Oct 15, 2020
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
81 changes: 81 additions & 0 deletions src/components/withBatchedRendering.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,81 @@
/**
* This HOC is used to incrementally display a collection of data in batches.
* Example: in MainView.js only the visible reports should be rendered in the first batch,
* then all other reports are rendered in the second batch
*/
import React, {Component} from 'react';
import _ from 'underscore';

/**
* Returns the display name of a component
*
* @param {object} component
* @returns {string}
*/
function getDisplayName(component) {
Copy link
Contributor

Choose a reason for hiding this comment

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

IIRC this function is the same for all our HOC's, can we DRY this up and make it a separate utility?

Copy link
Contributor

@marcaaron marcaaron Oct 15, 2020

Choose a reason for hiding this comment

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

Yeah I was thinking of doing this but just left it since the proposed changes are kind of complicated and I prefer to have fewer file changes per PR if possible.

Copy link
Contributor

Choose a reason for hiding this comment

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

Alright, it's NAB for me because this function is super simple

return component.displayName || component.name || 'Component';
}

export default function (propNameToBatch, batches) {
return (WrappedComponent) => {
class withBatchedRendering extends Component {
constructor(props) {
super(props);
this.timers = [];
this.state = {};
}

componentDidMount() {
_.each(batches, (batch) => {
this.timers.push(
setTimeout(() => {
this.setItemsToRender(batch.items(this.props));
}, batch.delay || 0)
);
});
}

componentDidUpdate(prevProps) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Also, if I'm being honest I'm confused about why we need this function and why this.props[propNameToBatch] is a list.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok hmm I'm trying to think of a better way to put this...

Right now I have

We need this to allow the flow of props down from a parent component to work normally after all the batches have finished rendering

But maybe the part you're missing is that...

  1. We are effectively replacing the property coming in from props with the one set in state in this HOC
  2. We explicitly remove the property from this.props in the render() so the props passed by a parent are filtered through this component
  3. If we don't listen for when that property changes and set it to state we'll just remove it and the wrapped component's props will never update

Copy link
Contributor

Choose a reason for hiding this comment

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

Okay, I think I get it now

// We need this to allow the flow of props down from a parent component
// to work normally after all the batches have finished rendering
if (_.size(prevProps[propNameToBatch]) !== _.size(this.props[propNameToBatch])) {
this.setItemsToRender(this.props[propNameToBatch]);
}
}

componentWillUnmount() {
// We need to clean up any timers when the component unmounts or else
// we'll call set state on an unmounting component.
_.each(this.timers, timerID => clearTimeout(timerID));
}

/**
* Sets items to the state key that matches the defined propNameToBatch
*
* @param {Object|Array} items - typically a collection of some kind
*/
setItemsToRender(items) {
this.setState({
[propNameToBatch]: items,
});
}

render() {
// We must remove the original prop that we are splitting into chunks
// since we only want our processed versions to be passed as a prop.
const propsToPass = _.omit(this.props, propNameToBatch);
return (
<WrappedComponent
// eslint-disable-next-line react/jsx-props-no-spreading
{...propsToPass}
// eslint-disable-next-line react/jsx-props-no-spreading
{...this.state}
/>
);
}
}

withBatchedRendering.displayName = `withBatchedRendering(${getDisplayName(WrappedComponent)})`;
return withBatchedRendering;
};
}
36 changes: 36 additions & 0 deletions src/pages/home/MainView.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import _ from 'underscore';
import ReportView from './report/ReportView';
import withIon from '../../components/withIon';
import IONKEYS from '../../IONKEYS';
import withBatchedRendering from '../../components/withBatchedRendering';
import styles from '../../styles/StyleSheet';
import {withRouter} from '../../libs/Router';
import compose from '../../libs/compose';
Expand All @@ -27,6 +28,19 @@ const defaultProps = {
};

class MainView extends Component {
/**
* Looks to see if the reportID matches the report ID in the URL because that
* report needs to be the one that is visible while all the other reports are hidden
*
* @param {number} reportID
*
* @returns {boolean}
*/
isReportIDMatchingURL(reportID) {
const reportIDInURL = parseInt(this.props.match.params.reportID, 10);
return reportID === reportIDInURL;
}

render() {
const reportIDInUrl = parseInt(this.props.match.params.reportID, 10);

Expand Down Expand Up @@ -82,4 +96,26 @@ export default compose(
key: IONKEYS.COLLECTION.REPORT,
},
}),

// The rendering of report views is done in batches.
// The first batch are all the reports that are visible in the LHN.
// The second batch are all the reports.
withBatchedRendering('reports', [
{
items: (props) => {
const reportIDInURL = parseInt(props.match.params.reportID, 10);
const isReportVisible = report => (
report.isUnread
|| report.pinnedReport
|| report.reportID === reportIDInURL
);
return _.pick(props.reports, isReportVisible);
},
delay: 0,
},
{
items: props => props.reports,
delay: 5000,
},
]),
)(MainView);
40 changes: 32 additions & 8 deletions src/pages/home/report/ReportActionsView.js
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,8 @@ import IONKEYS from '../../../IONKEYS';
import ReportActionItem from './ReportActionItem';
import styles from '../../../styles/StyleSheet';
import ReportActionPropTypes from './ReportActionPropTypes';
import compose from '../../../libs/compose';
import withBatchedRendering from '../../../components/withBatchedRendering';
import InvertedFlatList from '../../../components/InvertedFlatList';
import {lastItem} from '../../../libs/CollectionUtils';

Expand Down Expand Up @@ -216,11 +218,33 @@ class ReportActionsView extends React.Component {
ReportActionsView.propTypes = propTypes;
ReportActionsView.defaultProps = defaultProps;

export default withIon({
reportActions: {
key: ({reportID}) => `${IONKEYS.COLLECTION.REPORT_ACTIONS}${reportID}`,
},
session: {
key: IONKEYS.SESSION,
},
})(ReportActionsView);
export default compose(
withIon({
reportActions: {
key: ({reportID}) => `${IONKEYS.COLLECTION.REPORT_ACTIONS}${reportID}`,
},
session: {
key: IONKEYS.SESSION,
},
}),

// The rendering of report actions happens in batches.
// The first batch of actions is limited to the 100 most recent actions.
// The second batch is all of the rest of the actions.
withBatchedRendering('reportActions', [
{
items: (props) => {
const sortedReportActions = _.sortBy(props.reportActions, 'sequenceNumber');
return _.chain(sortedReportActions).last(100).indexBy('sequenceNumber').value();
},
delay: 0,
},
{
items: (props) => {
const sortedReportActions = _.sortBy(props.reportActions, 'sequenceNumber');
return _.indexBy(sortedReportActions, 'sequenceNumber');
},
delay: 7000,
},
]),
)(ReportActionsView);
2 changes: 1 addition & 1 deletion src/pages/home/sidebar/ChatSwitcherView.js
Original file line number Diff line number Diff line change
Expand Up @@ -296,7 +296,7 @@ class ChatSwitcherView extends React.Component {
}
}}
onChangeText={this.updateSearch}
onClearButtonClick={this.reset}
onClearButtonClick={() => this.reset()}
Copy link
Contributor

Choose a reason for hiding this comment

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

Why this change, is it by accident?

Copy link
Contributor

Choose a reason for hiding this comment

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

This was to fix a random error I found. not a bug or related to these changes. one of the annoying prop-types kind of errors.

onFocus={this.triggerOnFocusCallback}
onKeyPress={this.handleKeyPress}
/>
Expand Down