Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Consider always dispatching session state instead of only onRefreshTokenExpired #9

Closed
roxk opened this issue Sep 22, 2020 · 1 comment
Assignees
Labels
enhancement New feature or request

Comments

@roxk
Copy link
Contributor

roxk commented Sep 22, 2020

Motivation

Given the current design as follows:

val onRefreshTokenExpired: OnRefreshTokenExpiredListener?
suspend fun authorize(options: AuthorizeOptions)
suspend fun logout()

The design has 2 small problems.

Same state, multiple handlers

Current design mandates caller of authorize and logout to handle success/failure, when the results of these actions, i.e. changes of session state, might be depended upon by several distinct, unrelated components of an app. For example, a user-preference repository might pre-fetch user-preference in the background once user had logged in. Since there is no delegate for login success, user is forced to call userPreferenceRepository.fetch() after authorize returns success. This gets worse if there are more components which "reacts" on session state.

Same handler, multiple call site

It is also likely that user would want to perform the same action upon logout and token expiry. Current design only provide token expiry delegate, but not logout, forcing user to perform the same action at possibly two logically separated portion of an app, e.g. start up (it is usually where global delegate is configured) and setting/logout screen.

This problem gets worse as the app provides more paths to logout/authorize: each time the user calls authorize, he would need to remember to call the corresponding handler (e.g. there might be login button in both setting screen, login screen, post feedback screen, etc). Granted, this can be mitigated by developer's own good abstraction (e.g. an app-wide app.handleLogin(), app.handleLogout()), but this adds unnecessary complexity to developers.

Snippets that demonstrate this problem:

// Setting screen:
mLoginButton.setOnClickListener(view -> mAuthgear.authorize(new Listener {
  @Override
  void onTokenObtained() {
    // Do something with the token that is not specific to login flow in this screen
    settingScreenSpecificUiUpdate();
  }
}));

// Login screen:
mLoginButton.setOnClickListener(view -> mAuthgear.authorize(new Listener {
  @Override
  void onTokenObtained() {
    // Do the same something with the token that is not specific to login flow in this screen
    navigateToMain();
  }
}));

// Post feed back screen:
mLoginButton.setOnClickListener(view -> mAuthgear.authorize(new Listener {
  @Override
  void onTokenObtained() {
    // Do the same something with the token that is not specific to login flow in this screen
    populateSenderWithUserInfo();
  }
}));

Depending on the app's DI setup, it is likely that the portion that repeats would be wrapped in some kind of utility function/specific services. Worst case scenario is that said portion gets copy and pasted a bunch of times.

Proposal

Core

If we view session state as a reactive state, and whatever actions that mutate them (authorize and logout) would simply trigger a new event for the new state, the two problems can be solved easily:

  • The same state can be observed by multiple observers
  • The same observer would be triggered regardless of triggers

Thus this proposal suggest the following addition and removal:

+ interface OnSessionStateChangedListener { /*Omitted for brevity*/ }
+ val sessionState: SessionState
+ val onSessionStateChanged: List<OnSessionStateChangedListener>
+ fun addOnSessionStateChangedListener(listener: OnSessionStateChangedListener)
+ fun removeOnSessionStateChangedListener(listener: OnSessionStateChangedListener)
- val onRefreshTokenExpired: OnRefreshTokenExpiredListener?

Note that this does not mean authorize and logout should not have return value/callback, since users probably still need to know when these specific actions finishe. This proposal only adds the ability to handle more general cases without removing the ability to handle specific code paths.

Change trigger

@kiootic suggests that user might still want to distinguish between logout/expiry in the listener, thus we can modify OnSessionStateListener to also supply an enum ChangeTrigger, as follows:

enum class ChangeTrigger {
  NoToken,
  FoundToken,
  Authorize,
  Logout,
  Expiry
}

// State diagram:
                                                           Logout/Expiry
                                                 +-----------------------------------------+
                                                 v                                         |
SessionState.Unknown --- NoToken ----> SessionState.NoSession ---- Authorize -----> SessionState.LoggedIn
      |                                                                                    ^
      +------------------------------------------------------------------------------------+
                           FoundToken

How it Solves the Problem

Same state, multiple handlers

// Login screen:
mLoginButton.setOnClickListener(view -> mAuthgear.authorize(new Listener {
  @Override
  void onTokenObtained() {
    navigateToMain();
  }
}));

// User preference repository:
mAuthgear.addOnSessionStateChangedListener(sessionState -> preFetchPreference());

// At start up:
mAuthgear.addOnSessionStateChangedListener(sessionState -> {
  if (sessionState == NoSession) {
    forceLogoutToLoginScreen();
  }
});

Same handler, multiple call site

// User preference repository:
mAuthgear.addOnSessionStateChangedListener(sessionState -> preFetchPreference());

// ...and the rest

// Screens. Notice how each callback only need to handle screen-specific login/logout flow now.
// Setting screen:
mLoginButton.setOnClickListener(view -> mAuthgear.authorize(new Listener {
  @Override
  void onTokenObtained() {
    settingScreenSpecificUiUpdate();
  }
}));

// Login screen:
mLoginButton.setOnClickListener(view -> mAuthgear.authorize(new Listener {
  @Override
  void onTokenObtained() {
    navigateToMain();
  }
}));

// Post feed back screen:
mLoginButton.setOnClickListener(view -> mAuthgear.authorize(new Listener {
  @Override
  void onTokenObtained() {
    populateSenderWithUserInfo();
  }
}));
@roxk roxk added the enhancement New feature or request label Sep 22, 2020
@roxk roxk self-assigned this Sep 22, 2020
@roxk roxk added this to Planning in Sept 2020 Milestone Sep 22, 2020
@roxk
Copy link
Contributor Author

roxk commented Sep 25, 2020

Would adopt. Naming need some update, e.g. maybe should change ChangeTrigger to simply ChangeReason.

The documentation for ChangeReason must be throughout and to the point and not bring confusion.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
No open projects
Development

No branches or pull requests

1 participant