Skip to content

Improved initial value in useRefs - changed useRefs to more suitable hooks #29643

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

Merged
mountiny merged 7 commits into from
Oct 26, 2023
Merged

Improved initial value in useRefs - changed useRefs to more suitable hooks #29643

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
5b12a46
Improved initial state in useRefs - changed useRefs to more suitable …
sebryu Oct 15, 2023
b0be5dd
Refactored storing useMemo first computation to new hook - useInitial…
sebryu Oct 20, 2023
10952f2
Eslint fix
sebryu Oct 20, 2023
c1a1acf
Eslint fix
sebryu Oct 23, 2023
8feada1
Removed dependency array from useInitialValue call
sebryu Oct 24, 2023
7e06bd7
Merge branch 'main' into Sebryu-performance-initial-ref-optimization
sebryu Oct 25, 2023
46170f8
Merge branch 'main' into Sebryu-performance-initial-ref-optimization
sebryu Oct 26, 2023
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
4 changes: 2 additions & 2 deletions src/components/Attachments/AttachmentCarousel/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,7 @@ function AttachmentCarousel({report, reportActions, source, onNavigate, setDownl
* @param {Object} item
* @param {number} index
*/
const updatePage = useRef(
const updatePage = useCallback(
({viewableItems}) => {
Keyboard.dismiss();

Expand Down Expand Up @@ -207,7 +207,7 @@ function AttachmentCarousel({report, reportActions, source, onNavigate, setDownl
getItemLayout={getItemLayout}
keyExtractor={(item) => item.source}
viewabilityConfig={viewabilityConfig}
onViewableItemsChanged={updatePage.current}
onViewableItemsChanged={updatePage}
Copy link
Copy Markdown
Contributor

@situchan situchan Oct 27, 2023

Choose a reason for hiding this comment

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

There's bad regression on this line:

Screen.Recording.2023-10-27.at.1.46.22.PM.mov

/>
)}

Expand Down
9 changes: 9 additions & 0 deletions src/hooks/useInitialValue.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
import {useState} from 'react';

// In some places we set initial value on first render, but we don't want to re-run the function
// This hook will memoize the initial value and return that without setter, so it's never changed
// https://github.com/Expensify/App/pull/29643#issuecomment-1765894078
export default function useInitialValue<T>(initialStateFunc: () => T) {
const [initialValue] = useState(initialStateFunc);
Copy link
Copy Markdown
Contributor

@allroundexperts allroundexperts Oct 26, 2023

Choose a reason for hiding this comment

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

Shouldn't we call the initialStateFunc?

Copy link
Copy Markdown
Contributor

@allroundexperts allroundexperts Oct 26, 2023

Choose a reason for hiding this comment

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

I don't see it being called anywhere.

Copy link
Copy Markdown
Contributor Author

@sebryu sebryu Oct 26, 2023

Choose a reason for hiding this comment

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

@allroundexperts I'm passing function to useState and useState will run it only once for computing initial value. It's used that way to avoid unnecessary recalculation of initial state function, as described in react docs (calling useState(initialStateFunc()) will run initialStateFunc on each render).

Copy link
Copy Markdown
Contributor

@allroundexperts allroundexperts Oct 26, 2023

Choose a reason for hiding this comment

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

Nice. I didn't know that. Thanks for the link!

return initialValue;
}
2 changes: 1 addition & 1 deletion src/pages/home/report/ContextMenu/PopoverReportActionContextMenu.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -238,7 +238,7 @@ function PopoverReportActionContextMenu(_props, ref) {
Report.deleteReportComment(reportIDRef.current, reportActionRef.current);
}
setIsDeleteCommentConfirmModalVisible(false);
}, [reportActionRef]);
Copy link
Copy Markdown
Contributor

@Li357 Li357 Oct 24, 2023

Choose a reason for hiding this comment

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

Why are we making this change?

Copy link
Copy Markdown
Contributor Author

@sebryu sebryu Oct 24, 2023

Choose a reason for hiding this comment

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

reportActionRef is a variable that would never change (reportActionRef.current could change but that wouldn't cause rerender) - so I've changed it to line which is easier to read and has clearer intent. So it won't make developers think that callback is dependent on that variable.
From react useRef docs

On the next renders, useRef will return the same object.

}, []);

const hideDeleteModal = () => {
callbackWhenDeleteModalHide.current = () => (onCancelDeleteModal.current = runAndResetCallback(onCancelDeleteModal.current));
Expand Down
9 changes: 5 additions & 4 deletions src/pages/home/report/ReportActionsView.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ import reportPropTypes from '../../reportPropTypes';
import PopoverReactionList from './ReactionList/PopoverReactionList';
import getIsReportFullyVisible from '../../../libs/getIsReportFullyVisible';
import {ReactionListContext} from '../ReportScreenContext';
import useInitialValue from '../../../hooks/useInitialValue';

const propTypes = {
/** The report currently being looked at */
Expand Down Expand Up @@ -71,9 +72,9 @@ function ReportActionsView(props) {
const didLayout = useRef(false);
const didSubscribeToReportTypingEvents = useRef(false);
const isFirstRender = useRef(true);
const hasCachedActions = useRef(_.size(props.reportActions) > 0);
const hasCachedActions = useInitialValue(() => _.size(props.reportActions) > 0);
const mostRecentIOUReportActionID = useInitialValue(() => ReportActionsUtils.getMostRecentIOURequestActionID(props.reportActions));

const mostRecentIOUReportActionID = useRef(ReportActionsUtils.getMostRecentIOURequestActionID(props.reportActions));
const prevNetworkRef = useRef(props.network);
const prevIsSmallScreenWidthRef = useRef(props.isSmallScreenWidth);

Expand Down Expand Up @@ -204,7 +205,7 @@ function ReportActionsView(props) {
}

didLayout.current = true;
Timing.end(CONST.TIMING.SWITCH_REPORT, hasCachedActions.current ? CONST.TIMING.WARM : CONST.TIMING.COLD);
Timing.end(CONST.TIMING.SWITCH_REPORT, hasCachedActions ? CONST.TIMING.WARM : CONST.TIMING.COLD);

// Capture the init measurement only once not per each chat switch as the value gets overwritten
if (!ReportActionsView.initMeasured) {
Expand All @@ -226,7 +227,7 @@ function ReportActionsView(props) {
report={props.report}
onLayout={recordTimeToMeasureItemLayout}
sortedReportActions={props.reportActions}
mostRecentIOUReportActionID={mostRecentIOUReportActionID.current}
mostRecentIOUReportActionID={mostRecentIOUReportActionID}
loadOlderChats={loadOlderChats}
loadNewerChats={loadNewerChats}
isLoadingInitialReportActions={props.isLoadingInitialReportActions}
Expand Down
53 changes: 28 additions & 25 deletions src/pages/iou/steps/MoneyRequestConfirmPage.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ import useNetwork from '../../../hooks/useNetwork';
import useWindowDimensions from '../../../hooks/useWindowDimensions';
import {iouPropTypes, iouDefaultProps} from '../propTypes';
import * as Expensicons from '../../../components/Icon/Expensicons';
import useInitialValue from '../../../hooks/useInitialValue';

const propTypes = {
/** React Navigation route */
Expand Down Expand Up @@ -63,10 +64,10 @@ function MoneyRequestConfirmPage(props) {
const {isOffline} = useNetwork();
const {windowWidth} = useWindowDimensions();
const prevMoneyRequestId = useRef(props.iou.id);
const iouType = useRef(lodashGet(props.route, 'params.iouType', ''));
const isDistanceRequest = MoneyRequestUtils.isDistanceRequest(iouType.current, props.selectedTab);
const iouType = useInitialValue(() => lodashGet(props.route, 'params.iouType', ''));
Copy link
Copy Markdown
Contributor

@Li357 Li357 Oct 24, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

What's the actual performance benefit here? Object property lookup is not expensive. Or is this just for consistency?

Copy link
Copy Markdown
Contributor Author

@sebryu sebryu Oct 24, 2023

Choose a reason for hiding this comment

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

As you wrote - there is negligible performance benefit, so it's more about:

  1. Consistency.
  2. Clearer intent - now is easier to read this as 'I will take initial value on first render, and I won't change it'

const reportID = useInitialValue(() => lodashGet(props.route, 'params.reportID', ''));
const isDistanceRequest = MoneyRequestUtils.isDistanceRequest(iouType, props.selectedTab);
const isScanRequest = MoneyRequestUtils.isScanRequest(props.selectedTab);
const reportID = useRef(lodashGet(props.route, 'params.reportID', ''));
const [receiptFile, setReceiptFile] = useState();
const participants = useMemo(
() =>
Expand All @@ -77,7 +78,7 @@ function MoneyRequestConfirmPage(props) {
[props.iou.participants, props.personalDetails],
);
const isPolicyExpenseChat = useMemo(() => ReportUtils.isPolicyExpenseChat(ReportUtils.getRootParentReport(props.report)), [props.report]);
const isManualRequestDM = props.selectedTab === CONST.TAB.MANUAL && iouType.current === CONST.IOU.TYPE.REQUEST;
const isManualRequestDM = props.selectedTab === CONST.TAB.MANUAL && iouType === CONST.IOU.TYPE.REQUEST;

useEffect(() => {
IOU.resetMoneyRequestCategory();
Expand All @@ -101,47 +102,47 @@ function MoneyRequestConfirmPage(props) {
}
FileUtils.readFileAsync(props.iou.receiptPath, props.iou.receiptFilename).then((file) => {
if (!file) {
Navigation.goBack(ROUTES.MONEY_REQUEST.getRoute(iouType.current, reportID.current));
Navigation.goBack(ROUTES.MONEY_REQUEST.getRoute(iouType, reportID));
} else {
const receipt = file;
receipt.state = file && isManualRequestDM ? CONST.IOU.RECEIPT_STATE.OPEN : CONST.IOU.RECEIPT_STATE.SCANREADY;
setReceiptFile(receipt);
}
});
}, [props.iou.receiptPath, props.iou.receiptFilename, isManualRequestDM]);
}, [props.iou.receiptPath, props.iou.receiptFilename, isManualRequestDM, iouType, reportID]);

useEffect(() => {
// ID in Onyx could change by initiating a new request in a separate browser tab or completing a request
if (!isDistanceRequest && prevMoneyRequestId.current !== props.iou.id) {
// The ID is cleared on completing a request. In that case, we will do nothing.
if (props.iou.id) {
Navigation.goBack(ROUTES.MONEY_REQUEST.getRoute(iouType.current, reportID.current), true);
Navigation.goBack(ROUTES.MONEY_REQUEST.getRoute(iouType, reportID), true);
}
return;
}

// Reset the money request Onyx if the ID in Onyx does not match the ID from params
const moneyRequestId = `${iouType.current}${reportID.current}`;
const moneyRequestId = `${iouType}${reportID}`;
const shouldReset = !isDistanceRequest && props.iou.id !== moneyRequestId;
if (shouldReset) {
IOU.resetMoneyRequestInfo(moneyRequestId);
}

if (_.isEmpty(props.iou.participants) || (props.iou.amount === 0 && !props.iou.receiptPath && !isDistanceRequest) || shouldReset || ReportUtils.isArchivedRoom(props.report)) {
Navigation.goBack(ROUTES.MONEY_REQUEST.getRoute(iouType.current, reportID.current), true);
Navigation.goBack(ROUTES.MONEY_REQUEST.getRoute(iouType, reportID), true);
}

return () => {
prevMoneyRequestId.current = props.iou.id;
};
}, [props.iou.participants, props.iou.amount, props.iou.id, props.iou.receiptPath, isDistanceRequest, props.report]);
}, [props.iou.participants, props.iou.amount, props.iou.id, props.iou.receiptPath, isDistanceRequest, props.report, iouType, reportID]);

const navigateBack = () => {
let fallback;
if (reportID.current) {
fallback = ROUTES.MONEY_REQUEST.getRoute(iouType.current, reportID.current);
if (reportID) {
fallback = ROUTES.MONEY_REQUEST.getRoute(iouType, reportID);
} else {
fallback = ROUTES.MONEY_REQUEST_PARTICIPANTS.getRoute(iouType.current);
fallback = ROUTES.MONEY_REQUEST_PARTICIPANTS.getRoute(iouType);
}
Navigation.goBack(fallback);
};
Expand Down Expand Up @@ -211,8 +212,8 @@ function MoneyRequestConfirmPage(props) {
const trimmedComment = props.iou.comment.trim();

// If we have a receipt let's start the split bill by creating only the action, the transaction, and the group DM if needed
if (iouType.current === CONST.IOU.TYPE.SPLIT && props.iou.receiptPath) {
const existingSplitChatReportID = CONST.REGEX.NUMBER.test(reportID.current) ? reportID.current : '';
if (iouType === CONST.IOU.TYPE.SPLIT && props.iou.receiptPath) {
const existingSplitChatReportID = CONST.REGEX.NUMBER.test(reportID) ? reportID : '';
FileUtils.readFileAsync(props.iou.receiptPath, props.iou.receiptFilename).then((receipt) => {
IOU.startSplitBill(
selectedParticipants,
Expand All @@ -228,7 +229,7 @@ function MoneyRequestConfirmPage(props) {

// IOUs created from a group report will have a reportID param in the route.
// Since the user is already viewing the report, we don't need to navigate them to the report
if (iouType.current === CONST.IOU.TYPE.SPLIT && CONST.REGEX.NUMBER.test(reportID.current)) {
if (iouType === CONST.IOU.TYPE.SPLIT && CONST.REGEX.NUMBER.test(reportID)) {
IOU.splitBill(
selectedParticipants,
props.currentUserPersonalDetails.login,
Expand All @@ -237,13 +238,13 @@ function MoneyRequestConfirmPage(props) {
trimmedComment,
props.iou.currency,
props.iou.category,
reportID.current,
reportID,
);
return;
}

// If the request is created from the global create menu, we also navigate the user to the group report
if (iouType.current === CONST.IOU.TYPE.SPLIT) {
if (iouType === CONST.IOU.TYPE.SPLIT) {
IOU.splitBillAndOpenReport(
selectedParticipants,
props.currentUserPersonalDetails.login,
Expand Down Expand Up @@ -281,6 +282,8 @@ function MoneyRequestConfirmPage(props) {
requestMoney,
createDistanceRequest,
receiptFile,
iouType,
reportID,
],
);

Expand Down Expand Up @@ -312,11 +315,11 @@ function MoneyRequestConfirmPage(props) {
return props.translate('common.distance');
}

if (iouType.current === CONST.IOU.TYPE.SPLIT) {
if (iouType === CONST.IOU.TYPE.SPLIT) {
return props.translate('iou.split');
}

if (iouType.current === CONST.IOU.TYPE.SEND) {
if (iouType === CONST.IOU.TYPE.SEND) {
return props.translate('common.send');
}

Expand All @@ -339,13 +342,13 @@ function MoneyRequestConfirmPage(props) {
{
icon: Expensicons.Receipt,
text: props.translate('receipt.addReceipt'),
onSelected: () => Navigation.navigate(ROUTES.MONEY_REQUEST_RECEIPT.getRoute(iouType.current, reportID.current)),
onSelected: () => Navigation.navigate(ROUTES.MONEY_REQUEST_RECEIPT.getRoute(iouType, reportID)),
},
]}
/>
<MoneyRequestConfirmationList
transactionID={props.iou.transactionID}
hasMultipleParticipants={iouType.current === CONST.IOU.TYPE.SPLIT}
hasMultipleParticipants={iouType === CONST.IOU.TYPE.SPLIT}
selectedParticipants={participants}
iouAmount={props.iou.amount}
iouComment={props.iou.comment}
Expand All @@ -367,15 +370,15 @@ function MoneyRequestConfirmPage(props) {
}}
receiptPath={props.iou.receiptPath}
receiptFilename={props.iou.receiptFilename}
iouType={iouType.current}
reportID={reportID.current}
iouType={iouType}
reportID={reportID}
isPolicyExpenseChat={isPolicyExpenseChat}
// The participants can only be modified when the action is initiated from directly within a group chat and not the floating-action-button.
// This is because when there is a group of people, say they are on a trip, and you have some shared expenses with some of the people,
// but not all of them (maybe someone skipped out on dinner). Then it's nice to be able to select/deselect people from the group chat bill
// split rather than forcing the user to create a new group, just for that expense. The reportID is empty, when the action was initiated from
// the floating-action-button (since it is something that exists outside the context of a report).
canModifyParticipants={!_.isEmpty(reportID.current)}
canModifyParticipants={!_.isEmpty(reportID)}
policyID={props.report.policyID}
bankAccountRoute={ReportUtils.getBankAccountRoute(props.report)}
iouMerchant={props.iou.merchant}
Expand Down
28 changes: 15 additions & 13 deletions src/pages/iou/steps/MoneyRequstParticipantsPage/MoneyRequestParticipantsPage.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import React, {useEffect, useRef, useState} from 'react';
import React, {useEffect, useRef, useState, useCallback} from 'react';
import {View} from 'react-native';
import PropTypes from 'prop-types';
import {withOnyx} from 'react-native-onyx';
Expand All @@ -17,6 +17,7 @@ import * as IOU from '../../../../libs/actions/IOU';
import * as MoneyRequestUtils from '../../../../libs/MoneyRequestUtils';
import {iouPropTypes, iouDefaultProps} from '../../propTypes';
import useLocalize from '../../../../hooks/useLocalize';
import useInitialValue from '../../../../hooks/useInitialValue';

const propTypes = {
/** React Navigation route */
Expand Down Expand Up @@ -47,10 +48,10 @@ function MoneyRequestParticipantsPage({iou, selectedTab, route}) {
const {translate} = useLocalize();
const prevMoneyRequestId = useRef(iou.id);
const optionsSelectorRef = useRef();
const iouType = useRef(lodashGet(route, 'params.iouType', ''));
const reportID = useRef(lodashGet(route, 'params.reportID', ''));
const isDistanceRequest = MoneyRequestUtils.isDistanceRequest(iouType.current, selectedTab);
const isSendRequest = iouType.current === CONST.IOU.TYPE.SEND;
const iouType = useInitialValue(() => lodashGet(route, 'params.iouType', ''));
const reportID = useInitialValue(() => lodashGet(route, 'params.reportID', ''));
const isDistanceRequest = MoneyRequestUtils.isDistanceRequest(iouType, selectedTab);
const isSendRequest = iouType === CONST.IOU.TYPE.SEND;
const isScanRequest = MoneyRequestUtils.isScanRequest(selectedTab);
const isSplitRequest = iou.id === CONST.IOU.TYPE.SPLIT;
const [headerTitle, setHeaderTitle] = useState();
Expand All @@ -71,12 +72,13 @@ function MoneyRequestParticipantsPage({iou, selectedTab, route}) {

const navigateToConfirmationStep = (moneyRequestType) => {
IOU.setMoneyRequestId(moneyRequestType);
Navigation.navigate(ROUTES.MONEY_REQUEST_CONFIRMATION.getRoute(moneyRequestType, reportID.current));
Navigation.navigate(ROUTES.MONEY_REQUEST_CONFIRMATION.getRoute(moneyRequestType, reportID));
};

const navigateBack = (forceFallback = false) => {
Navigation.goBack(ROUTES.MONEY_REQUEST.getRoute(iouType.current, reportID.current), forceFallback);
};
const navigateBack = useCallback((forceFallback = false) => {
Navigation.goBack(ROUTES.MONEY_REQUEST.getRoute(iouType, reportID), forceFallback);
// eslint-disable-next-line react-hooks/exhaustive-deps -- no deps as we use only initial values
}, []);

useEffect(() => {
// ID in Onyx could change by initiating a new request in a separate browser tab or completing a request
Expand All @@ -89,7 +91,7 @@ function MoneyRequestParticipantsPage({iou, selectedTab, route}) {
}

// Reset the money request Onyx if the ID in Onyx does not match the ID from params
const moneyRequestId = `${iouType.current}${reportID.current}`;
const moneyRequestId = `${iouType}${reportID}`;
const shouldReset = iou.id !== moneyRequestId;
if (shouldReset) {
IOU.resetMoneyRequestInfo(moneyRequestId);
Expand All @@ -101,7 +103,7 @@ function MoneyRequestParticipantsPage({iou, selectedTab, route}) {
return () => {
prevMoneyRequestId.current = iou.id;
};
}, [iou.amount, iou.id, iou.receiptPath, isDistanceRequest, isSplitRequest]);
}, [iou.amount, iou.id, iou.receiptPath, isDistanceRequest, isSplitRequest, iouType, reportID, navigateBack]);

return (
<ScreenWrapper
Expand All @@ -120,10 +122,10 @@ function MoneyRequestParticipantsPage({iou, selectedTab, route}) {
ref={(el) => (optionsSelectorRef.current = el)}
participants={iou.participants}
onAddParticipants={IOU.setMoneyRequestParticipants}
navigateToRequest={() => navigateToConfirmationStep(iouType.current)}
navigateToRequest={() => navigateToConfirmationStep(iouType)}
navigateToSplit={() => navigateToConfirmationStep(CONST.IOU.TYPE.SPLIT)}
safeAreaPaddingBottomStyle={safeAreaPaddingBottomStyle}
iouType={iouType.current}
iouType={iouType}
isDistanceRequest={isDistanceRequest}
isScanRequest={isScanRequest}
/>
Expand Down
Loading