Skip to content

useOnNetworkReconnect() #17400

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
marcaaron merged 4 commits into from
May 6, 2023
Merged

useOnNetworkReconnect() #17400

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
a7e4315
useOnNetworkReconnect
marcaaron Apr 13, 2023
6089d3b
Clean up with useOnNetworkReconnect()
marcaaron Apr 13, 2023
7338e7a
Remove callback from dependencies
marcaaron Apr 14, 2023
05d79db
Fix conflicts
marcaaron Apr 25, 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
27 changes: 4 additions & 23 deletions src/components/Avatar.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, {useState} from 'react';
import {View} from 'react-native';
import PropTypes from 'prop-types';
import _ from 'underscore';
Expand All @@ -9,10 +9,9 @@ import CONST from '../CONST';
import * as StyleUtils from '../styles/StyleUtils';
import * as Expensicons from './Icon/Expensicons';
import Image from './Image';
import {withNetwork} from './OnyxProvider';
import networkPropTypes from './networkPropTypes';
import styles from '../styles/styles';
import * as ReportUtils from '../libs/ReportUtils';
import useOnNetworkReconnect from './hooks/useOnNetworkReconnect';

const propTypes = {
/** Source for the avatar. Can be a URL or an icon. */
Expand Down Expand Up @@ -44,9 +43,6 @@ const propTypes = {

/** Owner of the avatar, typically a login email or workspace name */
name: PropTypes.string,

/** Props to detect online status */
network: networkPropTypes.isRequired,
};

const defaultProps = {
Expand All @@ -62,23 +58,8 @@ const defaultProps = {

function Avatar(props) {
const [imageError, setImageError] = useState(false);
const prevNetworkStatusRef = useRef(props.network.isOffline);

useEffect(() => {
const isReconnecting = prevNetworkStatusRef.current && !props.network.isOffline;
if (!imageError || !isReconnecting) {
return;
}
setImageError(false);

// We have not added the imageError as the dependency because effect is concerned with `imageError` only when the network state changes from offline -> online
// eslint-disable-next-line react-hooks/exhaustive-deps
}, [props.network.isOffline]);

useEffect(() => {
// Used to store previous network state to compare on next render
prevNetworkStatusRef.current = props.network.isOffline;
});
useOnNetworkReconnect(() => setImageError(false));

if (!props.source) {
return null;
Expand Down Expand Up @@ -128,4 +109,4 @@ function Avatar(props) {
}
Avatar.defaultProps = defaultProps;
Avatar.propTypes = propTypes;
export default withNetwork()(Avatar);
export default Avatar;
3 changes: 2 additions & 1 deletion src/components/OnyxProvider.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ import ComposeProviders from './ComposeProviders';

// Set up any providers for individual keys. This should only be used in cases where many components will subscribe to
// the same key (e.g. FlatList renderItem components)
const [withNetwork, NetworkProvider] = createOnyxContext(ONYXKEYS.NETWORK, {});
const [withNetwork, NetworkProvider, NetworkContext] = createOnyxContext(ONYXKEYS.NETWORK, {});
const [withPersonalDetails, PersonalDetailsProvider] = createOnyxContext(ONYXKEYS.PERSONAL_DETAILS);
const [withCurrentDate, CurrentDateProvider] = createOnyxContext(ONYXKEYS.CURRENT_DATE);
const [withReportActionsDrafts, ReportActionsDraftsProvider] = createOnyxContext(ONYXKEYS.COLLECTION.REPORT_ACTIONS_DRAFTS);
Expand Down Expand Up @@ -45,4 +45,5 @@ export {
withCurrentDate,
withBlockedFromConcierge,
withBetas,
NetworkContext,
};
2 changes: 1 addition & 1 deletion src/components/createOnyxContext.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,5 +51,5 @@ export default (onyxKeyName, defaultValue) => {
return Consumer;
};

return [withOnyxKey, ProviderWithOnyx];
return [withOnyxKey, ProviderWithOnyx, Context];
};
27 changes: 27 additions & 0 deletions src/components/hooks/useOnNetworkReconnect.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
import {useRef, useContext, useEffect} from 'react';
import {NetworkContext} from '../OnyxProvider';

/**
Comment thread
stitesExpensify marked this conversation as resolved.
* @param {Function} onNetworkReconnect
*/
export default function useOnNetworkReconnect(onNetworkReconnect) {
const callback = useRef(onNetworkReconnect);
callback.current = onNetworkReconnect;

@fedirjh fedirjh Apr 25, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Do you think this is necessary? I think providing an initialValue should be enough for ref initialization.

initialValue: The value you want the ref object’s current property to be initially. It can be a value of any type. This argument is ignored after the initial render.

@marcaaron marcaaron Apr 25, 2023

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yes because the function itself could change e.g. if it was passed as a prop. Not saying it would be the best idea, but I think we rather support that than confuse someone down the line.

@fedirjh fedirjh Apr 26, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

NAB: Ah, I see now. However, it could be optimized further with the use of useEffect

useEffect(() => {
    callback.current = onNetworkReconnect;
}, [onNetworkReconnect]);

@kidroca kidroca Apr 26, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Introducing useEffect would change the intended logic - it would assign the new value, only after the hook runs, this could lead to situations where other hooks might not have a reference to the new value when they run

The expression:

callback.current = onNetworkReconnect;

sets an object key to point to a memory address, there's no heavy computation to mitigate by introducing an effect hook (it's not like = is copying or cloning the assigned value)

@fedirjh fedirjh Apr 26, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for explaining that, @kidroca. I see your point now and agree that using the direct assignment is the better approach as it avoids any potential timing issues. Thanks again for your input!


const {isOffline} = useContext(NetworkContext);
const prevOfflineStatusRef = useRef(isOffline);
useEffect(() => {
// If we were offline before and now we are not offline then we just reconnected
const didReconnect = prevOfflineStatusRef.current && !isOffline;
if (!didReconnect) {
return;
}

callback.current();
}, [isOffline]);

useEffect(() => {
// Used to store previous prop values to compare on next render
prevOfflineStatusRef.current = isOffline;
}, [isOffline]);
}
19 changes: 3 additions & 16 deletions src/pages/iou/MoneyRequestModal.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,11 +23,10 @@ import ScreenWrapper from '../../components/ScreenWrapper';
import CONST from '../../CONST';
import * as PersonalDetails from '../../libs/actions/PersonalDetails';
import withCurrentUserPersonalDetails from '../../components/withCurrentUserPersonalDetails';
import networkPropTypes from '../../components/networkPropTypes';
import {withNetwork} from '../../components/OnyxProvider';
import reportPropTypes from '../reportPropTypes';
import * as ReportUtils from '../../libs/ReportUtils';
import * as ReportScrollManager from '../../libs/ReportScrollManager';
import useOnNetworkReconnect from '../../components/hooks/useOnNetworkReconnect';
import * as DeviceCapabilities from '../../libs/DeviceCapabilities';

/**
Expand All @@ -44,9 +43,6 @@ const propTypes = {
// eslint-disable-next-line react/no-unused-prop-types
report: reportPropTypes,

/** Information about the network */
network: networkPropTypes.isRequired,

// Holds data related to request view state, rather than the underlying request data.
iou: PropTypes.shape({
/** Whether or not transaction creation has started */
Expand Down Expand Up @@ -112,7 +108,6 @@ const MoneyRequestModal = (props) => {
: [Steps.MoneyRequestAmount, Steps.MoneyRequestParticipants, Steps.MoneyRequestConfirm]),
[reportParticipants.length]);
const prevCreatingIOUTransactionStatusRef = useRef(lodashGet(props.iou, 'creatingIOUTransaction'));
const prevNetworkStatusRef = useRef(props.network.isOffline);

const [previousStepIndex, setPreviousStepIndex] = useState(-1);
const [currentStepIndex, setCurrentStepIndex] = useState(0);
Expand Down Expand Up @@ -144,18 +139,11 @@ const MoneyRequestModal = (props) => {
}
}, [props.iou]);

useEffect(() => {
if (props.network.isOffline || !prevNetworkStatusRef.current) {
return;
}

// User came back online, so let's refetch the currency details based on location
PersonalDetails.openMoneyRequestModalPage();
}, [props.network.isOffline]);
// User came back online, so let's refetch the currency details based on location
useOnNetworkReconnect(PersonalDetails.openMoneyRequestModalPage);

useEffect(() => {
// Used to store previous prop values to compare on next render
prevNetworkStatusRef.current = props.network.isOffline;
prevCreatingIOUTransactionStatusRef.current = lodashGet(props.iou, 'creatingIOUTransaction');
});

Expand Down Expand Up @@ -461,7 +449,6 @@ MoneyRequestModal.defaultProps = defaultProps;

export default compose(
withLocalize,
withNetwork(),
withCurrentUserPersonalDetails,
withOnyx({
report: {
Expand Down