Update ion when clicking links

[stitesExpensify]

Fixes #394

Because links in the sidebar were only changing the route without letting Ion know, we were having issues where chats would not be rendered due to the state not actually updating.

The flow went like this:

1. Use the chat switcher (cmd + k) to open up a chat with someone
   • This sets the top level Expensify component's state to redirectTo: xxxx
2. Click a chat room
   • The top level component does not change
3. Try to open a chat with the same person and it will not work
   • We try to set the top level Expensify component's state to redirectTo: xxxx again, but since that's already the state, we don't actually render the chat we want to

This PR makes it so that we always update the state via Ion to have the correct redirectTo.

[stitesExpensify]

This is the linting error
##[error] 25:5 error 'onLinkClick' PropType is defined but prop is never used react/no-unused-prop-types
but it doesn't make sense because the prop is used right here https://github.com/Expensify/ReactNativeChat/pull/416/files#diff-293bbd40378d083e097973aa92e15091R39

[tgolen]

The change in this PR fixes the bug, but I don't think it's fixing the source of the bug, just one symptom of it. The bug can be described technically like this:

1. User starts out in the app so CURRENT_URL is /
2. APP_REDIRECT_TO is set to /B and user is taken to /B
3. User clicks on a link for /C
4. CURRENT_URL is set to /C
5. APP_REDIRECT_TO is set to /B again, but user stays on /C

This is because the value of APP_REDIRECT_TO in Ion did not trigger a state change to trigger <Redirect>. Setting state to the same value it previously had doesn't trigger a state change in React.

This PR doesn't fix the root cause because this bug will happen on any internal application link.

In order to fix the root problem, I think that any time CURRENT_URL is the same as APP_REDIRECT_TO, then APP_REDIRECT_TO should be cleared out. This way, the next time APP_REDIRECT_TO is set, <Redirect> will be properly triggered. In practice, this is the change in Expensify.js:

    recordCurrentRoute({match}) {
        Ion.set(IONKEYS.CURRENT_URL, match.url);
        if (match.url === this.props.redirectTo) {
            Ion.set(IONKEYS.APP_REDIRECT_TO, '');
        }
    }

There is one other change that I think should happen in this PR to clean up the code. I'm going to be clearing this up in the README so it's more clear moving forward, but the UI part of the app should avoid calling Ion directly in almost all cases. That's what actions are for. I would like to see:

1. A new action created actions/App.js
2. The recordCurrentRoute() method moved into that Action
3. Change Expensify.js to interact with the Action

Then we would also:

1. Add a method to that action called redirect()
2. Change any UI code that is calling Ion.set(IONKEYS.APP_REDIRECT_TO) to call the action method instead

[tgolen]

No need to pass all the props, just pass props.reportID

[tgolen]

If this method is moved to be inside of render() then there is no need to pass props at all, and it should make the eslint prop error go away.

[stitesExpensify]

Updated based off your comments. The only one I didn't change to use redirect is this which is an Ion.multiSet so I'm not sure what we want to do with it

[tgolen]

For the multi-set... just remove APP_REDIRECT_TO from the multiSet() and call redirect() directly. Then, if the multiSet() only has a single key left in it, you can convert it to just a normal set().

[tgolen]

This will be more apparent as my refactoring PRs get merged, but there is a better pattern for accessing the currentRedirectTo. Take a look at this code that was just merged to see how an action is properly supposed to access data that is inside of Ion. Let me know if you have any questions on how to implement it.

Currently, only the Report.js action has been refactored to use this, but I am submitting another PR right now to refactor everything else.

[stitesExpensify]

For the multi-set... just remove APP_REDIRECT_TO from the multiSet() and call redirect() directly. Then, if the multiSet() only has a single key left in it, you can convert it to just a normal set().

Will that mess up anything with the promises? Right now we're returning the multiSet promise and have things that do .then on the method. If we split them up isn't there a chance of a race condition where redirect could finish after Ion.set and the redirctTo data would be old when the .then runs?

[tgolen]

Ah, in my refactoring (you can see the PR here) most of the promise chains are getting removed so that shouldn't be an issue any longer. If you want to wait to replace that multiSet() until after my PR is merged, that is fine too.

[stitesExpensify]

Cool, I'll do that. Updated to connect to Ion correctly! (I was trying to do Ion.get(IONKEYS.APP_REDIRECT_TO) which is a promise so it didn't work, this makes much more sense 👍)

[tgolen]

This looks great! Thanks