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

tgolen · 2020-09-08T22:18:12Z

This final refactoring does quite a few things:

It removes all traces of Ion.get() and Ion.multiGet()
It simplifies the Ion connection config to remove loader, loaderParams, path, defaultValue, and addAsCollection
It adds a lot to the readme to help clarify the code philosophy
It initializes all Ion connections with the data already in Ion as part of the connect() process
It adds local timezone support!
Simplifies a lot of the promise chains

Tests

Need to regression test everything. Make sure to sign out and sign in!

Fixes #402

tgolen · 2020-09-10T19:41:13Z

OK, comments were addressed, and branch was tested. I'd appreciate someone double-checking that everything still works (like signing out, signing in, making new comments, having brand new users send comments, unread indicators, etc.).

marcaaron

Ok here are the tests I performed. Overall working good. But I couldn't repro either of these on the master branch.

Unable to sign in without first refreshing sign in page

Clear local storage
Sign in with Expensify login
Unable to sign in
Refresh page
Now able to sign in

Once we are logged in leaving comments works well

Sign out
Sign in
Everything seems ok

But one weird thing that I noticed is that signing out does not unsubscribe you from Pusher. I think maybe it should? But was unsure if there would be any bad effects from that. I tested on master and nothing bad happens (even though yes we are still subscribed there as well) then tested on this branch and ran into problems:

Sign out as person A
Leave a comment from another user on a report as person B
Sign in as person A
Observed the following errors (see below) and unresponsive app
Refresh page and everything works fine again

The Pusher issue seems pretty edge case + we just need to unsubscribe, but I think we should fix the sign in bug.

tgolen · 2020-09-10T20:29:13Z

OK, thanks for verifying the sign-in problem. I wasn't sure if it was only me experiencing that or not, so let me take a deeper look into that. I agree that we should disconnect from pusher when signing out too.

marcaaron · 2020-09-10T20:37:27Z

It seems like browser notifications don't work anymore as well, but I'm not sure if that's expected?

I am seeing this log and only have one tab open

tgolen · 2020-09-10T21:10:08Z

OK, I updated this with pusher disconnect. I had to refactor a little about how we are connecting and subscribing. I also fixed the active client manager so notifications will work.

marcaaron · 2020-09-10T22:47:44Z

Ok everything is working perfect for me now 👍

marcaaron · 2020-09-10T22:48:13Z

Pushed a change to fix the strange routing issue discovered so removing myself as reviewer.

Dismissing my own review since I pushed a change

tgolen · 2020-09-10T22:51:21Z

@cead22 all you!

cead22 · 2020-09-10T23:09:30Z

README.md

+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.


callback callback

cead22 · 2020-09-10T23:11:08Z

README.md

+    - 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.


cead22 · 2020-09-10T23:16:13Z

src/lib/Ion.js

+    }
+
+    // Anything else (strings and numbers) need to be set into storage
+    AsyncStorage.setItem(key, JSON.stringify(val))


Do strings and number need to be JSON.stringified?

Ah, no they don't 👍

I guess they do actually. That's because get() does a JSON.parse() on everything (since it doesn't know what kind of a value is stored).

cead22 · 2020-09-10T23:17:45Z

src/lib/actions/PersonalDetails.js

-        .then(() => detailsFormatted);
+    queueRequest('PersonalDetails_GetForEmails', {emailList})
+        .then((data) => {
+            const details = _.omit(data, ['jsonCode', 'requestID']);


NAB but I think we should prefer _.pick over _.omit to be explicit about what we're storing and so we don't store anything we don't want to accidentally (whitelisting vs blacklisting)

OK, that's a good idea. I think we could use _.pick() with emailList and that should work.

cead22 · 2020-09-10T23:18:44Z

src/lib/actions/PersonalDetails.js

+    queueRequest('PersonalDetails_GetForEmails', {emailList})
+        .then((data) => {
+            const details = _.omit(data, ['jsonCode', 'requestID']);
+            const formattedDetails = formatPersonalDetails(details);


NAB: I'm a fan of temp variables even when they're unnecessary if their name makes the code more obvious, but I don't think it does in this case and Ion.merge(IONKEYS.PERSONAL_DETAILS, formatPersonalDetails(details)) is just as clear

Yeah, no worries, I can change this. I think I did this for debugging, and then never put it back.

cead22 · 2020-09-10T23:29:27Z

src/lib/API.js

                // 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;


We should revert this change (or remove the comment, and make sure we know exactly why this works but the previous version didn't)

OK, I can do that, however, considering that the previous version wasn't what we wanted, I don't think there is a whole lot of value in that.

Oh, I just quickly looked at it and I can tell right away why the return wasn't working. It's because setSuccessfulSignInData() was not returning the promise from Ion.merge() at the end of it, so the race condition still existed where the authToken was being accessed before it was set in Ion.

Good catch, the previous version wasn't what we wanted because it was relying on the return value of a promise right? If that's the case I agree. However, I'd also argue this isn't what we want, because we're now updating the local auth token in two places, here and the Ion connection, do you agree?

If so, what's the best way to solve this? maybe put something in ion that dictates we're ready to continue that flow, and use an Ion connection to that something to make the calls that we're making here when the value is updated

because we're now updating the local auth token in two places

Yeah, I understand what you're saying. I don't think it's really the most ideal, no. If all code started updating these local variables whenever they want, then you could end up with bugs where the local variable is not the same as what is (or will be) in Ion.

I think I keep going back to the best way to solve it is what I had posted in Slack:

start Ion off with:

SESSION: { authToken: null, login: null, password: null, }

Then connect to Ion:

Ion.connect({key: SESSION, callback: (val) => { if (!val.authToken) { // Need to authenticate() Ion.set('authenticating', true); } else if (!val.login) { // Need to create() Ion.set('authenticating', false); } }}); let authenticating; Ion.connect({key: 'authenticating', callback: (isCurrentlyAuthenticating) => { // When the app is no longer authenticating if (authenticating && !isCurrentlyAuthenticating) { // Restart the network queue } }});

Then:

407

Add request to network queue

Ion.set(SESSION.AUTHTOKEN, null) (and at this point all the steps to fill out a complete session are automatically handled)

This is a more ideal solution because essentially EVERYTHING that needs to happen is triggered off of Ion events once the data in Ion has been updated.

Perfect, I can do that here #454

cead22 · 2020-09-10T23:31:25Z

src/lib/ActiveClientManager.js


 const clientID = Guid();

+// @TODO make all this work by uncommenting code. This will work once


I meant we should have no @todos in the comment ideally, or commented out code for that matter. That's what GH issues are for (otherwise we'll go years with these TODOs polluting the code), and git, so we should remove the commented out code

cead22 · 2020-09-10T23:39:42Z

src/lib/actions/PersonalDetails.js

+            const allPersonalDetails = formatPersonalDetails(data.personalDetailsList);
+            Ion.merge(IONKEYS.PERSONAL_DETAILS, allPersonalDetails);
+
+            // Get my personal details so they can be easily accessed and subscribed to on their own key


s/Get/Set

cead22 · 2020-09-10T23:40:05Z

src/lib/actions/PersonalDetails.js

+        })
+        .catch(error => console.error('Error fetching personal details', error));
+
+    // Refresh the personal details every 30 minutes


Why do we need to poll for this, that seems contrary to our whole design philosophy for this app, is it because we're not pushing events when somebody updates their personal details? If so, can you include that in the comment?

cead22 · 2020-09-10T23:45:16Z

src/lib/actions/Session.js

-        .catch(err => Ion.merge(IONKEYS.SESSION, {error: err.message}));
+    redirectToSignIn();
+    API.deleteLogin({
+        partnerUserID: credentials && credentials.login


We shouldn't call deleteLogin with partnerID: null

Should we add doNotRetry: true to this call?

Should we add doNotRetry: true to this call?

I don't know, what is the criteria for adding that?

I saw DeleteLogin fail before due to a bug, because we didn't have the right auth token, or because create login itself failed, but I think in either of those cases, since we're signing out, it doesn't make sense to retry the request if it fails

OK, that makes sense. Thanks! I'll add it.

Oh, actually... I just looked, and API.deleteLogin() adds it in there already 🙈

Hahaha Go API! I wonder who wrote that... :ohnothing:

cead22 · 2020-09-10T23:56:16Z

Merging since this is a big PR and while there are some comments that I'd consider blockers, nothing will happen if we merge this and address those in separate PRs (cc @tgolen)

tgolen added 23 commits September 4, 2020 16:07

Remove Ion.get() from active client manager

ef4067a

Remove Ion.get() from singinredirect action

f147cfd

Remove Ion.get() from chat switcher

4b08616

Remove Ion.get() from personalDetails

0cb7111

Remove Ion.get() from report actions view

1259771

Remove the exported method \o/

f2179d9

Remove Ion.multiGet() from session action

08c231f

Remove Ion.multiGet()

ce44cac

Fix timezone

035ec30

Fixes #402

Remove path and default value

d800ef0

Move initialization of Ion values into connect()

f50712a

Remove addAsCollection

b1e7dde

Move loaders into the home page

873c032

Removed last loader

89220f1

Remove code for the loader

8dd64ed

Add more stuff to the philosophy

6ef5535

Improve explanation of callback connect

48dab35

Move data flow to the beginning

fe07f44

Wrap code in code block

b472695

Add more clarity

5d79d9f

Add more info about how the app is structured

eec5bda

Fix typo

6b431c8

Merge branch 'master' into tgolen-remve-get

25b2a15

tgolen requested review from AndrewGable, Jag96, cead22 and marcaaron September 8, 2020 22:18

tgolen added 2 commits September 8, 2020 17:18

Combine into a single merge

ae35b0d

Update code comments

fea2c7b

tgolen mentioned this pull request Sep 8, 2020

Update ion when clicking links #416

Merged

tgolen added 4 commits September 10, 2020 12:48

Fix an infinite redirect loop

cd77969

Try to correct styles in the footer

b477dfd

Address PR comments

affbdf2

Fix unread indicators

ed43ca1

tgolen added 2 commits September 10, 2020 13:45

Merge branch 'master' into tgolen-remve-get

76291f2

Fix a few more Ion.set()

887df40

marcaaron self-requested a review September 10, 2020 20:21

marcaaron previously requested changes Sep 10, 2020

View reviewed changes

tgolen added 2 commits September 10, 2020 15:02

Disconnect from pusher when signing out

8c4f564

Fix notifications

679351f

tgolen and others added 2 commits September 10, 2020 15:16

Remove unused lib

b1e127b

Add exact path for root and redirect to HomePage or SignIn

aa194ec

marcaaron requested review from cead22 and marcaaron and removed request for marcaaron September 10, 2020 22:48

marcaaron mentioned this pull request Sep 10, 2020

Pusher is subscribing to a private-user-accountID-undefined channel #429

Closed

cead22 approved these changes Sep 10, 2020

View reviewed changes

cead22 merged commit d88f9bc into master Sep 10, 2020

cead22 deleted the tgolen-remve-get branch September 10, 2020 23:56

tgolen mentioned this pull request Sep 11, 2020

Enable active client manager, multiple tabs, and previous PR cleanup #469

Merged

MelvinBot mentioned this pull request Jan 9, 2023

[Snyk] Security upgrade babel-plugin-module-resolver from 4.1.0 to 5.0.0 #14146

Merged


		const clientID = Guid();

		// @TODO make all this work by uncommenting code. This will work once

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

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

Uh oh!

Conversation

tgolen commented Sep 8, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Tests

Uh oh!

tgolen commented Sep 10, 2020

Uh oh!

marcaaron left a comment

Choose a reason for hiding this comment

Uh oh!

tgolen commented Sep 10, 2020

Uh oh!

marcaaron commented Sep 10, 2020

Uh oh!

tgolen commented Sep 10, 2020

Uh oh!

marcaaron commented Sep 10, 2020

Uh oh!

marcaaron commented Sep 10, 2020

Uh oh!

tgolen commented Sep 10, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cead22 commented Sep 10, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

tgolen commented Sep 8, 2020 •

edited

Loading