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

Improve reconnection callback #8858

Merged
merged 8 commits into from
May 3, 2022
Merged

Improve reconnection callback #8858

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
995c298
create getAppData
luacmartins May 2, 2022
980420e
replace API calls in AuthScreens with getAppData
luacmartins May 2, 2022
054bb14
update onReconnect registered functions'
luacmartins May 2, 2022
5957933
return promise
luacmartins May 2, 2022
ed35c1e
resolve dependency cycle
luacmartins May 2, 2022
d3e1840
rename Update to AppUpdate
luacmartins May 3, 2022
d06614b
add shouldSyncPolicyList param, create isListeningForReportChanges, u…
luacmartins May 3, 2022
d5ee72b
revert isListeningForReportChanges
luacmartins May 3, 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
18 changes: 2 additions & 16 deletions src/libs/Navigation/AppNavigator/AuthScreens.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,16 +19,13 @@ import ONYXKEYS from '../../../ONYXKEYS';
import Timing from '../../actions/Timing';
import NetworkConnection from '../../NetworkConnection';
import CONFIG from '../../../CONFIG';
import * as GeoLocation from '../../actions/GeoLocation';
import KeyboardShortcut from '../../KeyboardShortcut';
import Navigation from '../Navigation';
import * as User from '../../actions/User';
import * as Modal from '../../actions/Modal';
import NameValuePair from '../../actions/NameValuePair';
import * as Policy from '../../actions/Policy';
import modalCardStyleInterpolator from './modalCardStyleInterpolator';
import createCustomModalStackNavigator from './createCustomModalStackNavigator';
import * as BankAccounts from '../../actions/BankAccounts';

// Main drawer navigator
import MainDrawerNavigator from './MainDrawerNavigator';
Expand Down Expand Up @@ -107,20 +104,9 @@ class AuthScreens extends React.Component {
Policy.subscribeToPolicyEvents();
});

// Fetch some data we need on initialization
NameValuePair.get(CONST.NVP.PRIORITY_MODE, ONYXKEYS.NVP_PRIORITY_MODE, 'default');
NameValuePair.get(CONST.NVP.IS_FIRST_TIME_NEW_EXPENSIFY_USER, ONYXKEYS.NVP_IS_FIRST_TIME_NEW_EXPENSIFY_USER, true);
App.getLocale();
PersonalDetails.fetchPersonalDetails();
User.getUserDetails();
User.getBetas();
User.getDomainInfo();
PersonalDetails.fetchLocalCurrency();
Report.fetchAllReports(true, true);
GeoLocation.fetchCountryCodeByRequestIP();
// Listen for report changes and fetch some data we need on initialization
UnreadIndicatorUpdater.listenForReportChanges();
luacmartins marked this conversation as resolved.
Show resolved Hide resolved
BankAccounts.fetchFreePlanVerifiedBankAccount();
BankAccounts.fetchUserWallet();
luacmartins marked this conversation as resolved.
Show resolved Hide resolved
App.getAppData(false);

// Load policies, maybe creating a new policy first.
Linking.getInitialURL()
Expand Down
4 changes: 2 additions & 2 deletions src/libs/Notification/LocalNotification/BrowserNotifications.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,8 @@
import _ from 'underscore';
import Str from 'expensify-common/lib/str';
import focusApp from './focusApp';
import * as AppUpdate from '../../actions/AppUpdate';
import EXPENSIFY_ICON_URL from '../../../../assets/images/expensify-logo-round-clearspace.png';
import * as App from '../../actions/App';

const DEFAULT_DELAY = 4000;

Expand Down Expand Up @@ -128,7 +128,7 @@ export default {
body: 'A new version of this app is available!',
delay: 0,
onClick: () => {
App.triggerUpdateAvailable();
AppUpdate.triggerUpdateAvailable();
marcaaron marked this conversation as resolved.
Show resolved Hide resolved
},
});
},
Expand Down
42 changes: 39 additions & 3 deletions src/libs/actions/App.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,14 @@ import CONST from '../../CONST';
import Log from '../Log';
import Performance from '../Performance';
import Timing from './Timing';
import NameValuePair from './NameValuePair';
import * as PersonalDetails from './PersonalDetails';
import * as User from './User';
import * as Report from './Report';
import * as GeoLocation from './GeoLocation';
import * as BankAccounts from './BankAccounts';
import * as Policy from './Policy';
import NetworkConnection from '../NetworkConnection';

let currentUserAccountID;
Onyx.connect({
Expand Down Expand Up @@ -79,14 +87,42 @@ AppState.addEventListener('change', (nextAppState) => {
appState = nextAppState;
});

function triggerUpdateAvailable() {
Onyx.set(ONYXKEYS.UPDATE_AVAILABLE, true);
/**
* Fetches data needed for app initialization
*
* @param {Boolean} shouldSyncPolicyList Should be false if the initial policy needs to be created. Otherwise, should be true.
* @returns {Promise}
*/
function getAppData(shouldSyncPolicyList = true) {
NameValuePair.get(CONST.NVP.PRIORITY_MODE, ONYXKEYS.NVP_PRIORITY_MODE, 'default');
NameValuePair.get(CONST.NVP.IS_FIRST_TIME_NEW_EXPENSIFY_USER, ONYXKEYS.NVP_IS_FIRST_TIME_NEW_EXPENSIFY_USER, true);
getLocale();
User.getUserDetails();
User.getBetas();
User.getDomainInfo();
PersonalDetails.fetchLocalCurrency();
GeoLocation.fetchCountryCodeByRequestIP();
BankAccounts.fetchFreePlanVerifiedBankAccount();
BankAccounts.fetchUserWallet();

if (shouldSyncPolicyList) {
Policy.getPolicyList();
}

// We should update the syncing indicator when personal details and reports are both done fetching.
return Promise.all([
PersonalDetails.fetchPersonalDetails(),
Report.fetchAllReports(true, true),
]);
Copy link
Contributor

Choose a reason for hiding this comment

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

thought (non-blocking): I was kind of expecting to see all of these inside the Promise.all. But now wondering whether it matters. Probably the report and personal details are the main things you'd need to "sync".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Those two are the only calls returning promises. Not sure if adding the others will make a difference here.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah I guess my question could be rephrased as... should these be returning promises and should we wait for them? But not sure it is essential.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yea, I considered that but defaulted to doing nothing since that's the current behavior.

Copy link
Contributor

Choose a reason for hiding this comment

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

In the doc, we said all them would return promises. But not totally necessary imo.

}

// When the app reconnects from being offline, fetch all initialization data
NetworkConnection.onReconnect(getAppData);

export {
setCurrentURL,
setLocale,
setSidebarLoaded,
getLocale,
triggerUpdateAvailable,
getAppData,
};
11 changes: 11 additions & 0 deletions src/libs/actions/AppUpdate.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
import Onyx from 'react-native-onyx';
import ONYXKEYS from '../../ONYXKEYS';

function triggerUpdateAvailable() {
Onyx.set(ONYXKEYS.UPDATE_AVAILABLE, true);
}

export {
// eslint-disable-next-line import/prefer-default-export
triggerUpdateAvailable,
};
4 changes: 0 additions & 4 deletions src/libs/actions/PersonalDetails.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@ import Onyx from 'react-native-onyx';
import Str from 'expensify-common/lib/str';
import ONYXKEYS from '../../ONYXKEYS';
import CONST from '../../CONST';
import NetworkConnection from '../NetworkConnection';
import * as API from '../API';
import NameValuePair from './NameValuePair';
import * as LoginUtils from '../LoginUtils';
Expand Down Expand Up @@ -371,9 +370,6 @@ function deleteAvatar(defaultAvatarURL) {
mergeLocalPersonalDetails({avatar: defaultAvatarURL});
}

// When the app reconnects from being offline, fetch all of the personal details
NetworkConnection.onReconnect(fetchPersonalDetails);

export {
fetchPersonalDetails,
formatPersonalDetails,
Expand Down
3 changes: 0 additions & 3 deletions src/libs/actions/Report.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1380,9 +1380,6 @@ Onyx.connect({
callback: handleReportChanged,
});

// When the app reconnects from being offline, fetch all of the reports and their actions
NetworkConnection.onReconnect(fetchAllReports);

/**
* Saves a new message for a comment. Marks the comment as edited, which will be reflected in the UI.
*
Expand Down