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

Memoize some functions in OptionsListUtils #10239

Merged
merged 11 commits into from
Aug 8, 2022
Merged

Memoize some functions in OptionsListUtils #10239

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
7581104
Fix some propTypes errors
marcaaron Aug 4, 2022
59aaced
remove unneeded
marcaaron Aug 4, 2022
1445833
Add some comments to explain what is happening
marcaaron Aug 4, 2022
4a601ac
Add memoize-one as lodash doesnt work correctly
marcaaron Aug 4, 2022
d1e4e91
Memoize a few more things
marcaaron Aug 4, 2022
d817478
just leftover code
marcaaron Aug 5, 2022
9f912f2
Fix variable name
marcaaron Aug 5, 2022
ff5bfdb
revert back to OG package-lock.json
marcaaron Aug 5, 2022
4c2c66a
fix package-lock.json
marcaaron Aug 5, 2022
49426e0
fix conflicts
marcaaron Aug 8, 2022
1ef1980
Fix conflicts
marcaaron Aug 8, 2022
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
17 changes: 11 additions & 6 deletions package-lock.json
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

3 changes: 2 additions & 1 deletion package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,7 @@
"htmlparser2": "^7.2.0",
"localforage": "^1.10.0",
"lodash": "4.17.21",
"memoize-one": "^6.0.0",
Copy link
Contributor

Choose a reason for hiding this comment

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

Out of curiosity, why did we choose to add a new lib instead of using _.memoize: https://underscorejs.org/#memoize ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

_.memoize didn't work for me out of the box because it creates a hash from the first argument of the function (vs checking to see if all arguments are unique). memoize-one "just worked" so I rolled with it instead of trying to figure out the hash function stuff.

"metro-config": "^0.71.3",
"moment": "^2.29.4",
"moment-timezone": "^0.5.31",
Expand All @@ -93,7 +94,7 @@
"react-native-image-picker": "^4.7.3",
"react-native-image-size": "git+https://github.com/Expensify/react-native-image-size#6b5ab5110dc3ed554f8eafbc38d7d87c17147972",
"react-native-modal": "^13.0.0",
"react-native-onyx": "1.0.10",
"react-native-onyx": "1.0.12",
"react-native-pdf": "^6.6.2",
"react-native-performance": "^2.0.0",
"react-native-permissions": "^3.0.1",
Expand Down
96 changes: 55 additions & 41 deletions src/libs/OptionsListUtils.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ import _ from 'underscore';
import Onyx from 'react-native-onyx';
import lodashGet from 'lodash/get';
import lodashOrderBy from 'lodash/orderBy';
import memoizeOne from 'memoize-one';
import Str from 'expensify-common/lib/str';
import ONYXKEYS from '../ONYXKEYS';
import CONST from '../CONST';
Expand All @@ -11,6 +12,8 @@ import * as Localize from './Localize';
import Permissions from './Permissions';
import * as CollectionUtils from './CollectionUtils';

const memoizedOrderBy = memoizeOne(lodashOrderBy);

/**
* OptionsListUtils is used to build a list options passed to the OptionsList component. Several different UI views can
* be configured to display different results based on the options passed to the private getOptions() method. Public
Expand Down Expand Up @@ -41,18 +44,6 @@ Onyx.connect({
callback: val => preferredLocale = val || CONST.DEFAULT_LOCALE,
});

const reportsWithDraft = {};
Onyx.connect({
key: ONYXKEYS.COLLECTION.REPORTS_WITH_DRAFT,
callback: (hasDraft, key) => {
if (!key) {
return;
}

reportsWithDraft[key] = hasDraft;
},
});

const policies = {};
Onyx.connect({
key: ONYXKEYS.COLLECTION.POLICY,
Expand Down Expand Up @@ -126,6 +117,8 @@ function getPersonalDetailsForLogins(logins, personalDetails) {
return personalDetailsForLogins;
}

const memoizedGetPersonalDetailsForLogins = memoizeOne(getPersonalDetailsForLogins);

/**
* Constructs a Set with all possible names (displayName, firstName, lastName, email) for all participants in a report,
* to be used in isSearchStringMatch.
Expand Down Expand Up @@ -189,15 +182,17 @@ function getSearchText(report, reportName, personalDetailList, isChatRoomOrPolic
return _.unique(searchTerms).join(' ');
}

const memoizedGetSearchText = memoizeOne(getSearchText);

/**
* Determines whether a report has a draft comment.
*
* @param {Object} report
* @param {Object} reportsWithDraft
* @return {Boolean}
*/
function hasReportDraftComment(report) {
function hasReportDraftComment(report, reportsWithDraft = {}) {
return report
&& reportsWithDraft
&& lodashGet(reportsWithDraft, `${ONYXKEYS.COLLECTION.REPORTS_WITH_DRAFT}${report.reportID}`, false);
}

Expand All @@ -207,22 +202,24 @@ function hasReportDraftComment(report) {
* @param {Array<String>} logins
* @param {Object} personalDetails
* @param {Object} report
* @param {Object} reportsWithDraft
* @param {Object} options
* @param {Boolean} [options.showChatPreviewLine]
* @param {Boolean} [options.forcePolicyNamePreview]
* @returns {Object}
*/
function createOption(logins, personalDetails, report, {
showChatPreviewLine = false, forcePolicyNamePreview = false,
function createOption(logins, personalDetails, report, reportsWithDraft, {
showChatPreviewLine = false,
forcePolicyNamePreview = false,
}) {
const isChatRoom = ReportUtils.isChatRoom(report);
const isPolicyExpenseChat = ReportUtils.isPolicyExpenseChat(report);
const personalDetailMap = getPersonalDetailsForLogins(logins, personalDetails);
const personalDetailMap = memoizedGetPersonalDetailsForLogins(logins, personalDetails);
const personalDetailList = _.values(personalDetailMap);
const isArchivedRoom = ReportUtils.isArchivedRoom(report);
const hasMultipleParticipants = personalDetailList.length > 1 || isChatRoom || isPolicyExpenseChat;
const personalDetail = personalDetailList[0];
const hasDraftComment = hasReportDraftComment(report);
const hasDraftComment = hasReportDraftComment(report, reportsWithDraft);
const hasOutstandingIOU = lodashGet(report, 'hasOutstandingIOU', false);
const iouReport = hasOutstandingIOU
? lodashGet(iouReports, `${ONYXKEYS.COLLECTION.REPORT_IOUS}${report.iouReportID}`, {})
Expand Down Expand Up @@ -276,7 +273,7 @@ function createOption(logins, personalDetails, report, {
isUnread: report ? report.unreadActionCount > 0 : null,
hasDraftComment,
keyForList: report ? String(report.reportID) : personalDetail.login,
searchText: getSearchText(report, reportName, personalDetailList, isChatRoom || isPolicyExpenseChat),
searchText: memoizedGetSearchText(report, reportName, personalDetailList, isChatRoom || isPolicyExpenseChat),
isPinned: lodashGet(report, 'isPinned', false),
hasOutstandingIOU,
iouReportID: lodashGet(report, 'iouReportID'),
Expand All @@ -289,6 +286,8 @@ function createOption(logins, personalDetails, report, {
};
}

const memoizedCreateOption = memoizeOne(createOption);

/**
* Searches for a match when provided with a value
*
Expand Down Expand Up @@ -342,6 +341,10 @@ function isCurrentUser(userDetails) {
return result;
}

// We are storing a map of logins in the format {[login]: [login]} so that the memoized functions looking for an array with a login in it
// treat this like the same argument (because it will use the same reference). Memoization for personalDetails won't work properly without this.
const loginArrayMap = {};

/**
* Build the options
*
Expand All @@ -353,6 +356,7 @@ function isCurrentUser(userDetails) {
* @private
*/
function getOptions(reports, personalDetails, activeReportID, {
reportsWithDraft = {},
betas = [],
selectedOptions = [],
maxRecentReportsToShow = 0,
Expand Down Expand Up @@ -390,8 +394,9 @@ function getOptions(reports, personalDetails, activeReportID, {
if (sortByAlphaAsc) {
sortProperty = ['reportName'];
}

const sortDirection = [sortByAlphaAsc ? 'asc' : 'desc'];
let orderedReports = lodashOrderBy(reports, sortProperty, sortDirection);
let orderedReports = memoizedOrderBy(reports, sortProperty, sortDirection);

// Move the archived Rooms to the last
orderedReports = _.sortBy(orderedReports, report => ReportUtils.isArchivedRoom(report));
Expand All @@ -409,7 +414,7 @@ function getOptions(reports, personalDetails, activeReportID, {
return;
}

const hasDraftComment = hasReportDraftComment(report);
const hasDraftComment = hasReportDraftComment(report, reportsWithDraft);
const iouReportOwner = lodashGet(report, 'hasOutstandingIOU', false)
? lodashGet(iouReports, [`${ONYXKEYS.COLLECTION.REPORT_IOUS}${report.iouReportID}`, 'ownerEmail'], '')
: '';
Expand Down Expand Up @@ -456,22 +461,33 @@ function getOptions(reports, personalDetails, activeReportID, {
reportMapForLogins[logins[0]] = report;
}
const isSearchingSomeonesPolicyExpenseChat = !report.isOwnPolicyExpenseChat && searchValue !== '';
allReportOptions.push(createOption(logins, personalDetails, report, {
allReportOptions.push(memoizedCreateOption(logins, personalDetails, report, reportsWithDraft, {
showChatPreviewLine,
forcePolicyNamePreview: isPolicyExpenseChat ? isSearchingSomeonesPolicyExpenseChat : forcePolicyNamePreview,
}));
});

let allPersonalDetailsOptions = _.map(personalDetails, personalDetail => (
createOption([personalDetail.login], personalDetails, reportMapForLogins[personalDetail.login], {
showChatPreviewLine,
forcePolicyNamePreview,
})
));
let allPersonalDetailsOptions = _.map(personalDetails, (personalDetail) => {
// We want to use the same argument reference when creating the personalDetails option as memoization won't work properly
// if we passed [personalDetail.login] directly (that would be a new argument since we'd initialize a new array)
if (!loginArrayMap[personalDetail.login]) {
loginArrayMap[personalDetail.login] = [personalDetail.login];
}
return memoizedCreateOption(
loginArrayMap[personalDetail.login],
personalDetails,
reportMapForLogins[personalDetail.login],
reportsWithDraft,
{
showChatPreviewLine,
forcePolicyNamePreview,
},
);
});

if (sortPersonalDetailsByAlphaAsc) {
// PersonalDetails should be ordered Alphabetically by default - https://github.com/Expensify/App/issues/8220#issuecomment-1104009435
allPersonalDetailsOptions = lodashOrderBy(allPersonalDetailsOptions, [personalDetail => personalDetail.text.toLowerCase()], 'asc');
allPersonalDetailsOptions = memoizedOrderBy(allPersonalDetailsOptions, [personalDetail => personalDetail.text.toLowerCase()], 'asc');
}

// Always exclude already selected options and the currently logged in user
Expand Down Expand Up @@ -529,20 +545,20 @@ function getOptions(reports, personalDetails, activeReportID, {
// If we are prioritizing reports with draft comments, add them before the normal recent report options
// and sort them by report name.
if (prioritizeReportsWithDraftComments) {
const sortedDraftReports = lodashOrderBy(draftReportOptions, ['text'], ['asc']);
const sortedDraftReports = memoizedOrderBy(draftReportOptions, ['text'], ['asc']);
recentReportOptions = sortedDraftReports.concat(recentReportOptions);
}

// If we are prioritizing IOUs the user owes, add them before the normal recent report options and reports
// with draft comments.
if (prioritizeIOUDebts) {
const sortedIOUReports = lodashOrderBy(iouDebtReportOptions, ['iouReportAmount'], ['desc']);
const sortedIOUReports = memoizedOrderBy(iouDebtReportOptions, ['iouReportAmount'], ['desc']);
recentReportOptions = sortedIOUReports.concat(recentReportOptions);
}

// If we are prioritizing our pinned reports then shift them to the front and sort them by report name
if (prioritizePinnedReports) {
const sortedPinnedReports = lodashOrderBy(pinnedReportOptions, ['text'], ['asc']);
const sortedPinnedReports = memoizedOrderBy(pinnedReportOptions, ['text'], ['asc']);
recentReportOptions = sortedPinnedReports.concat(recentReportOptions);
}

Expand Down Expand Up @@ -584,7 +600,7 @@ function getOptions(reports, personalDetails, activeReportID, {
const login = (Str.isValidPhone(searchValue) && !searchValue.includes('+'))
? `+${countryCodeByIP}${searchValue}`
: searchValue;
userToInvite = createOption([login], personalDetails, null, {
userToInvite = memoizedCreateOption([login], personalDetails, null, reportsWithDraft, {
showChatPreviewLine,
});
userToInvite.icons = [ReportUtils.getDefaultAvatar(login)];
Expand All @@ -595,7 +611,7 @@ function getOptions(reports, personalDetails, activeReportID, {
// When sortByReportTypeInSearch is true, recentReports will be returned with all the reports including personalDetailsOptions in the correct Order.
recentReportOptions.push(...personalDetailsOptions);
personalDetailsOptions = [];
recentReportOptions = lodashOrderBy(recentReportOptions, [(option) => {
recentReportOptions = memoizedOrderBy(recentReportOptions, [(option) => {
if (option.isChatRoom || option.isArchivedRoom) {
return 3;
}
Expand Down Expand Up @@ -746,15 +762,10 @@ function getMemberInviteOptions(
* @param {Number} activeReportID
* @param {String} priorityMode
* @param {Array<String>} betas
* @param {Object} reportsWithDraft
* @returns {Object}
*/
function getSidebarOptions(
reports,
personalDetails,
activeReportID,
priorityMode,
betas,
) {
function calculateSidebarOptions(reports, personalDetails, activeReportID, priorityMode, betas, reportsWithDraft) {
let sideBarOptions = {
prioritizeIOUDebts: true,
prioritizeReportsWithDraftComments: true,
Expand All @@ -774,9 +785,12 @@ function getSidebarOptions(
showChatPreviewLine: true,
prioritizePinnedReports: true,
...sideBarOptions,
reportsWithDraft,
});
}

const getSidebarOptions = memoizeOne(calculateSidebarOptions);

/**
* Helper method that returns the text to be used for the header's message and title (if any)
*
Expand Down
8 changes: 7 additions & 1 deletion src/libs/actions/App.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,13 @@ Onyx.connect({
let myPersonalDetails;
Onyx.connect({
key: ONYXKEYS.PERSONAL_DETAILS,
callback: val => myPersonalDetails = val[currentUserEmail],
callback: (val) => {
if (!val || !currentUserEmail) {
return;
}

myPersonalDetails = val[currentUserEmail];
},
});

const allPolicies = {};
Expand Down
3 changes: 2 additions & 1 deletion src/pages/home/ReportScreen.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,7 @@ const propTypes = {

/** The type of the policy */
type: PropTypes.string,
})).isRequired,
})),
luacmartins marked this conversation as resolved.
Show resolved Hide resolved

/** Information about the network */
network: networkPropTypes.isRequired,
Expand All @@ -99,6 +99,7 @@ const defaultProps = {
isComposerFullSize: false,
betas: [],
isLoadingInitialReportActions: false,
policies: {},
};

/**
Expand Down
Loading