Originally, this diff was just making sure that the dispatch didn't occur before REHYDRATE. But when testing that change I found that calling dispatch from a reducer triggered an error. More context in this comment, but this diff is now just removing the dispatch, with the expectation that the error-handling functionality here will be improved following the work in ENG-1974.
Details
Details
- Reviewers
atul tomek kamil - Commits
- rCOMMde3b477b669f: [native] Don't call dispatch from reducer
I tested to make sure the app was still killed if an exception was thrown. I also tested that the dispatch error did not get thrown anymore:
You may not call store.getState() while the reducer is executing. The reducer has already received the state as an argument. Pass it down from the top reducer instead of reading it from the store.
Diff Detail
Diff Detail
- Repository
- rCOMM Comm
- Branch
- ashoat/androidNotifIssue
- Lint
No Lint Coverage - Unit
No Test Coverage
Event Timeline
Comment Actions
I just tested this codepath on a release Android build and found that it always throws an error as a result of using a dispatch from within a reducer:
You may not call store.getState() while the reducer is executing. The reducer has already received the state as an argument. Pass it down from the top reducer instead of reading it from the store.
This dispatch was added about a year ago in D2154 by @jacek, who confirmed on that diff that he tested to make sure the dispatch worked in release mode. Perhaps something has changed since then, or perhaps iOS differs from Android in some way?
Regardless, the error makes sense... one should not be calling dispatch from a reducer. Besides that, the behavior here is questionable for another reason, which is that it can cause an infinite loop: dispatching a setNewSessionActionType can generate more ops, which might trigger the same error if they fail to be processed.
I think the intentional here is basically: "If we fail to process the ops, that means our SQLite store is not updating, so we should throw it away and generate a new one. We will force a new log-in by clearing the cookie from Redux with a SET_NEW_SESSION action, and then killing the app and forcing the user to restart it."
The better solution for force-logging-out the user is described in ENG-2071. @kamil's plan is to set some value in expo-secure-store and then kill the app. And then later when the app starts again, SQLiteContextProvider will check this value, and if it sees it will call clearSensitiveData (deleting the SQLite store) and fetchNewCookieFromNativeCredentials (logging the user in again).
Updating SQLiteContextProvider is currently blocked on @marcin's work on ENG-2137, but once @kamil gets to it we should make sure we update the codepath in this diff as well.
In the meantime, I think I will simply remove the dispatch here.