Gizra / elm-restful

Elm utilities useful for working with a RESTful backend (especially Drupal)
MIT License
3 stars 0 forks source link

Proposed changes to `LoginStatus` #7

Closed amitaibu closed 6 years ago

amitaibu commented 6 years ago
type UserStatusAndData user anonymousData authenticatedData
    = AnonymousUser LoginProgress anonymousData
    | AuthenticatedUser (Authenticated user authenticatedData)
  1. LoginStatus => UserStatusAndData
  2. LoggedIn => AuthenticatedUser, Login => Authenticated
  3. CheckingCachedCredentials should IMO be moved into LoginProgress, as we're still considered Anonymous
amitaibu commented 6 years ago

@rgrempel lets discuss ^^

amitaibu commented 6 years ago

(My use is actually working fine with the existing code, but still worth discussing).

rgrempel commented 6 years ago

Setting up anonymousData inside this type would, in effect, set up three kinds of data.

Now, the purpose I had in mind for authenticatedData was to provide a structure in which data which depended upon being logged in would necessarily be thrown away upon logout. So, suppose you've fetched some data from the backend, using your credentials. If we keep track of that data inside authenticatedData, then we necessarily throw it away when we transition to a different status (e.g. when we log out). So, we necessarily know that our app doesn't remember anything that it had to be logged in to get -- neat!

Of course, there may well be other data which we don't have to be logged in to get. I had imagined that this would simply be managed outside of this structure -- if we didn't have to be logged in to get that data, then there is no reason to make sure that we throw it away. For instance, if we fetch something from the backend before logging in, we don't have to throw it away when we log in (which is what having anonymousData here will do), since that data is still fine.

However, that may not actually be true of all data. There are some endpoints which return one thing (or some subset of things) when you're anonymous and another when you're logged in. So, you could imagine this sequence of events:

  1. We fetch some data from an endpoint while anonymous.
  2. We login.
  3. Now, we ought to re-fetch the data from step 1, since the endpoint may respond differently to a logged-in user.

Of course, you could try to "catch" the login event and manually re-fetch in step 3. However, it would probably be better to model this kind of data as something that automatically gets thrown away when you login (or at least have the option to model the data that way). In essence, the data from an endpoint of this kind becomes invalid at the moment of login (as well as the moment of logout), so it's nice to be able to model that in the types.

And, how one uses it in a particular project would still be a choice. That is, you could still model some data (outside this structure) as not depending on whether you're logged in or not -- i.e. never thrown away, neither on logout or login.

One thing I haven't entirely figured out is whether this third kind of data (never thrown away) ought to be modelled here as well. It's really a question of convenience, I suppose -- is it difficult for some reason to model it elsewhere? On the whole, I'd prefer not to model it here (since we wouldn't really do anything with it), but we might do it if it ends up being convenient as a question of coding style.

Anyway, I think anonymousData makes sense, but will need to document that this is only meant for data which you want to throw away upon login -- not for all data that doesn't require login.

rgrempel commented 6 years ago

Moving CheckingCachedCredentials inside LoginProgress looks like a good idea, now that I think it through again.

The reason it isn't there already is that it seemed to me that CheckingCachedCredentials was only a sensible status as part of the startup process. That is, I thought it was not part of the flow of an actual login attempt by the user ... with a username and password ... instead, it's something that occurs before that. In fact, it's a purely transitory state ... it quickly evolves into either an "anonymous" or a "logged in" state.

Where this makes a tangible difference is the whole relogin idea, where we track that (a) we were logged in, but (b) we aren't able to contact the backend at the moment, so we don't know what the backend would say now. In that case, I thought that CheckingCachedCredentials wasn't a sensible status, since we've already done that.

But, thinking about it some more now, it seems to me that I was wrong. CheckingCachedCredentials is a sensible status in the relogin case, because we might very well periodically retry our cached credentials ... at least, we should be able to model that case.

So, I think you're right about that one too -- I'll see how it works out in code.

rgrempel commented 6 years ago

Merged in #11.