Refactor the rest of the code to remove Ion.get()

README.md

@@ -2,22 +2,55 @@
 
 # Philosophy
 This application is built with the following principles.
-1. **Offline first** - All data that is brought into the app should be stored immediately in Ion which puts the data into persistent storage (eg. localStorage on browser platforms).
-1. **UI Binds to Ion** - UI components bind to Ion so that any change to the Ion data is automatically reflected in the component by calling setState() with the changed data.
-1. **Actions manage Ion Data** - When the UI needs to request or write data from the server, this is done through Actions exclusively.
-    1. Actions should never return data, see the first point. Example:  if the action is `fetchReports()`, it does not return the reports, `fetchReports()` returns nothing. The action makes an XHR, then puts the data into Ion (using `Ion.set()` or `Ion.merge()`). Any UI that is subscribed to that piece of data in Ion is automatically updated.
+1. **Data Flow** - Ideally, this is how data flows through the app:
+    1. Server pushes data to the disk of any client (Server -> Pusher event -> Action listening to pusher event -> Ion). Currently the code only does this with report comments. Until we make more server changes, this steps is actually done by the client requesting data from the server via XHR and then storing the response in Ion.
+    1. Disk pushes data to the UI (Ion -> withIon()/connect() -> React component).
+    1. UI pushes data to people's brains (React component -> device screen).
+    1. Brain pushes data into UI inputs (Device input -> React component).
+    1. UI inputs push data to the server (React component -> Action -> XHR to server).
+    1. Go to 1
+1. **Offline first** 
+    - All data that is brought into the app and is necessary to display the app when offline should be stored on disk in persistent storage (eg. localStorage on browser platforms). [AsyncStorage](https://react-native-community.github.io/async-storage/) is a cross-platform abstraction layer that is used to access persistent storage.
+    - All data that is displayed, comes from persistent storage.
+1. **UI Binds to data on disk** 
+    - Ion is a Pub/Sub library to connect the application to the data stored on disk.
+    - UI components subscribe to Ion (using `withIon()`) and any change to the Ion data is published to the component by calling `setState()` with the changed data.
+    - Libraries subscribe to Ion (with `Ion.connect()`) and any change to the Ion data is published to the callback callback with the changed data.
+    - The UI should never call any Ion methods except for `Ion.connect()`. That is the job of Actions (see next section).
+    - The UI always triggers an Action when something needs to happen (eg. a person inputs data, the UI triggers an Action with this data).
+    - The UI should be as flexible as possible when it comes to:
+        - Incomplete or missing data. Always assume data is incomplete or not there. For example, when a comment is pushed to the client from a pusher event, it's possible that Ion does not have data for that report yet. That's OK. A partial report object is added to Ion for the report key `report_1234 = {reportID: 1234, isUnread: true}`. Then there is code that monitors Ion for reports with incomplete data, and calls `fetchChatReportsByIDs(1234)` to get the full data for that report. The UI should be able to gracefully handle the report object not being complete. In this example, the sidebar wouldn't display any report that doesn't have a report name.
+        - The order that actions are done in. All actions should be done in parallel instead of sequence. 
+            - Parallel actions are asynchronous methods that don't return promises. Any number of these actions can be called at one time and it doesn't matter what order they happen in or when they complete.
+            - In-Sequence actions are asynchronous methods that return promises. This is necessary when one asynchronous method depends on the results from a previous asynchronous method. Example: Making an XHR to `command=CreateChatReport` which returns a reportID which is used to call `command=Get&rvl=reportStuff`.
+1. **Actions manage Ion Data** 
+    - When data needs to be written to or read from the server, this is done through Actions only.
+    - Public action methods should never return anything (not data or a promise). This is done to ensure that action methods can be called in parallel with no dependency on other methods (see discussion above).
+    - Actions should favor using `Ion.merge()` over `Ion.set()` so that other values in an object aren't completely overwritten.
+    - In general, the operations that happen inside an action should be done in parallel and not in sequence ((eg. don't use the promise of one Ion method to trigger a second Ion method). Ion is built so that every operation is done in parallel and it doesn't matter what order they finish in. XHRs on the other hand need to be handled in sequence with promise chains in order to access and act upon the response.
+    - If an Action needs to access data stored on disk, use a local variable and `Ion.connect()`
+    - Data should be optimistically stored on disk whenever possible without waiting for a server response. Example of creating a new optimistic comment:
+        1. user adds a comment 
+        2. comment is shown in the UI (by mocking the expected response from the server) 
+        3. comment is created in the server 
+        4. server responds 
+        5. UI updates with data from the server
+
 1. **Cross Platform 99.9999%**
     1. A feature isn't done until it works on all platforms.  Accordingly, don't even bother writing a platform-specific code block because you're just going to need to undo it.
     1. If the reason you can't write cross platform code is because there is a bug in ReactNative that is preventing it from working, the correct action is to fix RN and submit a PR upstream -- not to hack around RN bugs with platform-specific code paths.
     1. If there is a feature that simply doesn't exist on all platforms and thus doesn't exist in RN, rather than doing if (platform=iOS) { }, instead write a "shim" library that is implemented with NOOPs on the other platforms.  For example, rather than injecting platform-specific multi-tab code (which can only work on browsers, because it's the only platform with multiple tabs), write a TabManager class that just is NOOP for non-browser platforms.  This encapsulates the platform-specific code into a platform library, rather than sprinkling through the business logic.
-    1. Put all platform specific code in a dedicated branch, like /platform, and reject any PR that attempts to put platform-specific code anywhere else.  This maintains a strict separation between business logic and platform code.
+    1. Put all platform specific code in dedicated files and folders, like /platform, and reject any PR that attempts to put platform-specific code anywhere else.  This maintains a strict separation between business logic and platform code.
 
 # Local development
 ## Getting started
 1. Install `node` & `npm`: `brew install node`
 2. Install `watchman`: `brew install watchman`
 3. Install dependencies: `npm install`
 4. Run `cp .env.example .env` and edit `.env` to have your local config options(for example, we are curretly hardcoding the pinned chat reports IDs with the `REPORT_IDS` config option).
+
+You can use any IDE or code editing tool for developing on any platform. Use your favorite!
+
 ## Running the web app 🕸
 * To run a **Development Server**: `npm run web`
 * To build a **production build**: `npm run build`
@@ -54,6 +87,40 @@ This application is built with the following principles.
 2. This will allow you to attach a debugger in your IDE, React Developer Tools, or your browser. 
 3. For more information on how to attach a debugger, see [React Native Debugging Documentation](https://reactnative.dev/docs/debugging#chrome-developer-tools)
 
+## Things to know or brush up on before jumping into the code
+1. The major difference between React-Native and React are the [components](https://reactnative.dev/docs/components-and-apis) that are used in the `render()` method. Everything else is exactly the same. If you learn React, you've already learned 98% of React-Native.
+1. The application uses [React-Router](https://reactrouter.com/native/guides/quick-start) for navigating between parts of the app.
+1. [Higher Order Components](https://reactjs.org/docs/higher-order-components.html) are used to connect React components to persistent storage via Ion.
+
+## Structure of the app
+These are the main pieces of the application.
+
+### Ion
+This is a persistent storage solution wrapped in a Pub/Sub library. In general that means:
+
+- Ion stores and retrieves data from persistent storage
+- Data is stored as key/value pairs, where the value can be anything from a single piece of data to a complex object
+- Collections of data are usually not stored as a single key (eg. an array with multiple objects), but as individual keys+ID (eg. `report_1234`, `report_4567`, etc.). Store collections as individual keys when a component will bind directly to one of those keys. For example: reports are stored as individual keys because `SidebarLink.js` binds to the individual report keys for each link. However, report actions are stored as an array of objects because nothing binds directly to a single report action.
+- Ion allows other code to subscribe to changes in data, and then publishes change events whenever data is changed
+- Anything needing to read Ion data needs to:
+    1. Know what key the data is stored in (for web, you can find this by looking in the JS console > Application > local storage)
+    2. Subscribe to changes of the data for a particular key or set of keys. React components use `withIon()` and non-React libs use `Ion.connect()`.
+    3. Get initialized with the current value of that key from persistent storage (Ion does this by calling `setState()` or triggering the `callback` with the values currently on disk as part of the connection process)
+- Subscribing to Ion keys is done using a regex pattern. For example, since all reports are stored as individual keys like `report_1234`, then if code needs to know about all the reports (eg. display a list of them in the nav menu), then it would subscribe to the key pattern `report_[0-9]+$`.
+
+### Actions
+Actions are responsible for managing what is on disk. This is usually:
+
+- Subscribing to Pusher events to receive data from the server that will get put immediately into Ion
+- Making XHRs to request necessary data from the server and then immediately putting that data into Ion
+- Handling any business logic with input coming from the UI layer
+
+### The UI layer
+This layer is solely responsible for:
+
+- Reflecting exactly the data that is in persistent storage by using `withIon()` to bind to Ion data.
+- Taking user input and passing it to an action
+
 # Deploying 
 ##  Continuous deployment / GitHub workflows
 Every PR merged into `master` will kick off the **Create a new version** GitHub workflow defined in `.github/workflows/version.yml`.

src/Expensify.js

@@ -50,17 +50,21 @@ class Expensify extends Component {
     }
 
     componentDidMount() {
-        Ion.connect({key: IONKEYS.SESSION, path: 'authToken', callback: this.removeLoadingState});
+        Ion.connect({
+            key: IONKEYS.SESSION,
+            callback: this.removeLoadingState,
+        });
     }
 
     /**
      * When the authToken is updated, the app should remove the loading state and handle the authToken
      *
-     * @param {string} authToken
+     * @param {object} session
+     * @param {string} session.authToken
      */
-    removeLoadingState(authToken) {
+    removeLoadingState(session) {
         this.setState({
-            authToken,
+            authToken: session ? session.authToken : null,
             isLoading: false,
         });
     }

src/components/withIon.js

@@ -67,25 +67,17 @@ export default function (mapIonToState) {
              * Takes a single mapping and binds the state of the component to the store
              *
              * @param {object} mapping
-             * @param {string} [mapping.path] a specific path of the store object to map to the state
-             * @param {mixed} [mapping.defaultValue] Used in conjunction with mapping.path to return if the there is
-             *  nothing at mapping.path
-             * @param {boolean} [mapping.addAsCollection] rather than setting a single state value, this will add things
-             *  to an array
-             * @param {string} [mapping.collectionID] the name of the ID property to use for the collection
+             * @param {string} statePropertyName the name of the state property that Ion will add the data to
+             * @param {string} [mapping.indexBy] the name of the ID property to use for the collection
              * @param {string} [mapping.pathForProps] the statePropertyName can contain the string %DATAFROMPROPS% wich
              *  will be replaced with data from the props matching this path. That way, the component can connect to an
              *  Ion key that uses data from this.props.
              *
              *  For example, if a component wants to connect to the Ion key "report_22" and
              *  "22" comes from this.props.match.params.reportID. The statePropertyName would be set to
              *  "report_%DATAFROMPROPS%" and pathForProps would be set to "match.params.reportID"
-             * @param {function} [mapping.loader] a method that will be called after connection to Ion in order to load
-             *  it with data. Typically this will be a method that makes an XHR to load data from the API.
-             * @param {mixed[]} [mapping.loaderParams] An array of params to be passed to the loader method
              * @param {boolean} [mapping.initWithStoredValues] If set to false, then no data will be prefilled into the
              *  component
-             * @param {string} statePropertyName the name of the state property that Ion will add the data to
              */
             connectMappingToIon(mapping, statePropertyName) {
                 const ionConnectionConfig = {
@@ -112,27 +104,6 @@ export default function (mapIonToState) {
                     connectionID = Ion.connect(ionConnectionConfig);
                     this.actionConnectionIDs[connectionID] = connectionID;
                 }
-
-                // Pre-fill the state with any data already in the store
-                if (mapping.initWithStoredValues !== false) {
-                    Ion.getInitialStateFromConnectionID(connectionID)
-                        .then(data => this.setState({[statePropertyName]: data}));
-                }
-
-                // Load the data from an API request if necessary
-                if (mapping.loader) {
-                    const paramsForLoaderFunction = _.map(mapping.loaderParams, (loaderParam) => {
-                        // Some params might com from the props data
-                        if (loaderParam === '%DATAFROMPROPS%') {
-                            return lodashGet(this.props, mapping.pathForProps);
-                        }
-                        return loaderParam;
-                    });
-
-                    // Call the loader function and pass it any params. The loader function will take care of putting
-                    // data into Ion
-                    mapping.loader(...paramsForLoaderFunction || []);
-                }
             }
 
             render() {

src/lib/API.js

@@ -8,6 +8,7 @@ import ROUTES from '../ROUTES';
 import Str from './Str';
 import Guid from './Guid';
 import redirectToSignIn from './actions/SignInRedirect';
+import {redirect} from './actions/App';
 
 // Holds all of the callbacks that need to be triggered when the network reconnects
 const reconnectionCallbacks = [];
@@ -21,35 +22,44 @@ let networkRequestQueue = [];
 let reauthenticating = false;
 
 let authToken;
-Ion.connect({key: IONKEYS.SESSION, path: 'authToken', callback: val => authToken = val});
+Ion.connect({
+    key: IONKEYS.SESSION,
+    callback: val => authToken = val ? val.authToken : null,
+});
 
-// We susbcribe to changes to the online/offline status of the network to determine when we should fire off API calls
-// vs queueing them for later. When going reconnecting, ie, going from offline to online, we fire off all the API calls
-// that we have in the queue
+// We subscribe to changes to the online/offline status of the network to determine when we should fire off API calls
+// vs queueing them for later. When reconnecting, ie, going from offline to online, all the reconnection callbacks
+// are triggered (this is usually Actions that need to re-download data from the server)
 let isOffline;
 Ion.connect({
     key: IONKEYS.NETWORK,
-    path: 'isOffline',
-    callback: (isCurrentlyOffline) => {
-        if (isOffline && !isCurrentlyOffline) {
+    callback: (val) => {
+        if (isOffline && !val.isOffline) {
             _.each(reconnectionCallbacks, callback => callback());
         }
-        isOffline = isCurrentlyOffline;
+        isOffline = val && val.isOffline;
     }
 });
 
 // When the user authenticates for the first time we create a login and store credentials in Ion.
 // When the user's authToken expires we use this login to re-authenticate and get a new authToken
 // and use that new authToken in subsequent API calls
 let credentials;
-Ion.connect({key: IONKEYS.CREDENTIALS, callback: ionCredentials => credentials = ionCredentials});
+Ion.connect({
+    key: IONKEYS.CREDENTIALS,
+    callback: ionCredentials => credentials = ionCredentials,
+});
 
 /**
  * @param {string} login
  * @param {string} password
  * @returns {Promise}
  */
 function createLogin(login, password) {
+    if (!authToken) {
+        throw new Error('createLogin() can\'t be called when there is no authToken');
+    }
+
     // Using xhr instead of request becasue request has logic to re-try API commands when we get a 407 authToken expired
     // in the response, and we call CreateLogin after getting a successful resposne to Authenticate so it's unlikely
     // that we'll get a 407.
@@ -60,7 +70,7 @@ function createLogin(login, password) {
         partnerUserID: login,
         partnerUserSecret: password,
     })
-        .then(() => Ion.set(IONKEYS.CREDENTIALS, {login, password}))
+        .then(() => Ion.merge(IONKEYS.CREDENTIALS, {login, password}))
         .catch(err => Ion.merge(IONKEYS.SESSION, {error: err}));
 }
 
@@ -91,7 +101,6 @@ function queueRequest(command, data) {
  *
  * @param {object} data
  * @param {string} exitTo
- * @returns {Promise}
  */
 function setSuccessfulSignInData(data, exitTo) {
     let redirectTo;
@@ -103,12 +112,8 @@ function setSuccessfulSignInData(data, exitTo) {
     } else {
         redirectTo = ROUTES.HOME;
     }
-    return Ion.multiSet({
-        // The response from Authenticate includes requestID, jsonCode, etc
-        // but we only care about setting these three values in Ion
-        [IONKEYS.SESSION]: _.pick(data, 'authToken', 'accountID', 'email'),
-        [IONKEYS.APP_REDIRECT_TO]: redirectTo,
-    });
+    redirect(redirectTo);
+    Ion.merge(IONKEYS.SESSION, _.pick(data, 'authToken', 'accountID', 'email'));
 }
 
 /**
@@ -148,7 +153,8 @@ function request(command, parameters, type = 'post') {
 
                 // We need to return the promise from setSuccessfulSignInData to ensure the authToken is updated before
                 // we try to create a login below
-                return setSuccessfulSignInData(response, parameters.exitTo);
+                setSuccessfulSignInData(response, parameters.exitTo);
+                authToken = response.authToken;
             })
             .then(() => {
                 // If Expensify login, it's the users first time signing in and we need to
@@ -355,7 +361,7 @@ function authenticate(parameters) {
         .catch((err) => {
             console.error(err);
             console.debug('[SIGNIN] Request error');
-            return Ion.merge(IONKEYS.SESSION, {error: err.message});
+            Ion.merge(IONKEYS.SESSION, {error: err.message});
         });
 }