How's the _handleLoginCallback actually working?

[bratelefant]

I'm still trying to understand how all the log in, log out, disconnect, reconnect etc business is really working in different situations. @jankapunkt got an issue #151 and #148 first looked fine but especially on bad network conditions, after resuming my app (not using NetInfo, just rely on AppState events) I still get the Meteor.user() to not be set.

I looked across the User.js and came across the _handleLoginCallback. Got a question here. Generally this method stores the resume-token in AsyncStorage and saves the userId.

If there is an error and if the request is a "resume" request coming from a token, then this code is used:

 if (!err) {
...
    } else {
       ...
      if (this._isTokenLogin) {
        setTimeout(() => {
          if (User._userIdSaved) {
            return;
          }
          this._timeout *= 2;
          if (Meteor.user()) {
            return;
          }
          User._loginWithToken(User._userIdSaved);
        }, this._timeout);
      }
      // Signify we aren't logginging in any more after a few seconds
      if (this._timeout > 2000) {
        User._endLoggingIn();
      }
      User._endLoggingIn();
      // we delegate the error to enable better logging
      Data.notify('onLoginFailure', err);
    }
    Data.notify('change');

Some questions and thoughts on this:

• Is the timed out retry of the token login necessary for too-many-requests errors?
• is User._loginWithToken ever called with a "not-nullish" value? Since if (User._userIdSaved) { return; } only allows a second try with a "nullish" value User._loginWithToken(User._userIdSaved);
• Shouldn't User._loginWithToken be called with a token, rather than a User._userIdSaved?
• What happens if the timeout exceeds 2 seconds? Longer login response times can happen, especially if you have bad network conditions or if you are changing networks, cells, aps or stuff like that.
• would be nice to be able to save just this token in SecureStorage; if you use the LocalCollections companion package, setting AsyncStorage to SecureStorage would also push all Items in LocalCollections as well; this doesn't seem a good solution, since obviously there's limits on available SecureStorage space (eg. Keychain, on iOS). Could be nice to provide a separate AsyncStorage interface for the LocalCollections companion package
• If the AsyncStorage in the core package is only used for storing the token, I'd be nice to default to SecureStorage, for security and token persistence reasons. (Is minimongo using AsyncStorage or is this in RAM only?)

[jankapunkt]

I think we should use this topic to discuss how we want it to behave :-) I think we should definitely make this way more configurable (with decent zweo-config defaults):

• configure expected timeouts
• configure what todo on timeout (strategy pattern), maybe even provide an optional custom callback or event on timeout
• support multiple storages
  • one default storage if none others are specified
  • optional different storage for keys and token
  • we could even support storage per collection or fallback to default by passing a custom storage factory function or similar

[bratelefant]

If this is a wishing list I think I'd be really nice to provide a hook managing all these connection and Meteor.user() aspects. Sort of useConnection and useCurrentUser, simply getting reactive info on connection status, setting up connections, getting current user.

This in combination with a bullet proof zero-config defaults and reliable connection management would be awesome.

[bratelefant]

I think what needs to be refactored is definitely the token login flow, in any case.

I'd consider this to be idempotent, but as far as I understand in the current implementation, a connection loss between sending the method call and getting the result in return will not be detected or handled/retried in any way.

[jankapunkt]

I currently complete all the missing tests so we can soon go forward with a potential solution!

[bratelefant]

Ok great, saw your PR on this. I'll work on a local branch experimenting with this a little bit