Refactor: Extract authentication and login screen from main app #2066
Conversation
dmsnell
force-pushed
the
refactor/extract-auth
branch
from
3473061
to
2eb6470
Compare
| }; | ||
|
|
||
| render() { | ||
| const systemTheme = window.matchMedia('(prefers-color-scheme: dark)') |
There was a problem hiding this comment.
I think this needs to be smarter pulling from localstorage before checking system theme.
There was a problem hiding this comment.
I'm happy to leave this for another PR, I'm not sure how it currently works.
There was a problem hiding this comment.
Note that at this point we aren't logged in. In the existing code we don't clear settings and so that stays in localStorage, which while convenient for places like this also seems wrong. If we really want to log out then I think we should forget the settings that person may have saved.
Do you feel differently?
There was a problem hiding this comment.
If you delete the settings that person set then every time they log in they will have to go change their settings. Until we have a way to preserve settings then I think it cannot be cleared.
| if (token) { | ||
| import('./boot-with-auth').then(({ bootWithToken }) => { | ||
| bootWithToken( | ||
| () => { |
There was a problem hiding this comment.
boot feels like the wrong place to have all this logic. Not now but in future PR's how do we move all this out of boot.
There was a problem hiding this comment.
What feels wrong about it?
There was a problem hiding this comment.
The code to sign a user out exists in the boot file. Analytics should be moved to middleware. Logging out functionality should be moved to an action.
There was a problem hiding this comment.
will discuss in Slack, but for the sake of responding here…
the code to sign a user out exists in the boot file
yeah I did this intentionally, specifically with the cookie handling. I wanted to move all the persistence and clearing of the authentication into one place and at the top so we can control how to load the app (with a side benefit of slimming up the logged-out page load)
if we move it out of boot where would you want it to move?
Analytics should be moved to middleware
this is something I discovered while doing this work - we're sending out analytics before we boot the app, even in develop. the analytics module appears to queue up these events until we load and until we have a reading on analytics_enabled, which is good, but it definitely looks wrong to be calling for trackers before being logged in (it looks like we're not waiting for consent even though we are)
if we want these edge calls to be in middleware then we're going to need to initialize the Redux store before login (which is what currently happens in develop). If we do that, I'd still really like to replace the store during the login, which we can do with replaceReducer()
I actually thought about doing this when creating this PR but I wanted to avoid the complexity of creating that replacement mechanism. if we move this in a future PR, I guess that would be a reasonable way to go about it.
Logging out functionality should be moved to an action.
It currently is handled by an action within the app but the Simperium middleware calls this function in response to that action or in response to an authentication expiration.
When receiving authentication from App Engine, clear storage if the account is different than the account already logged-in. Read `access_token` if stored from previous versions of the app.
Explored in 5f1f65d
Explored in e3c660e
This work exists in preparation for further and deeper work to decouple Simperium in the app data flows and to finish the internal state refactors.
The goal here is to initialize Simperium only after having proper authentication in order to allow us to move Simperium into Redux middleware. The further goal is to remove the race condition that exists in many places between making an edit or clicking a button, making a network call to Simperium (or not), updating
indexedDB, and rerendering the app.App boot is now handled on its own and centralizes the token-loading process. Upon logout it force-reloads the browser window to clear out app state. This is necessary due to the ways that things like
clientandapp-stateinitialize their variables in the module global scope. We can't reload the app state once the module has been imported the first time. Reloading the page completely resets this. There is a flash of a white screen when logging out.The
authcomponent of app state has been correspondingly removed because the app will not load without an authorization. A new actionLOGOUThas been created in order to trigger a logout in the app, driven from the Simperium middleware.Status
Ready for merge, deployed to https://app.simplenote.com already
Todo