Fix loading state for account history

[aristidesstaffieri]

Problem:
The account history page suffers from a jittery loading state. The reason this happens is because we model the loading state with a boolean, and this value is initialized as false then set to true while history data is being fetched, then set to false after the response is handled.

Potential Solutions:
We can "fix" this with a one line change, initializing the loading flag to true.

This improves the UX but makes the code brittle and less understandable. We can improve this by modeling our data fetching state differently, for example -

We could model our loading state more accurately with something like

export enum RequestState {
  IDLE = 'IDLE',
  LOADING = 'LOADING',
  SUCCESS = 'SUCCESS',
  ERROR = 'ERROR'
}

and we could write a hook that is reusable and encapsulates the loading/error/data logic, a simplified version could be -

export function useGetHistory() {
  const [state, setState] = React.useState(RequestState.IDLE)
  const [data, setData] = React.useState([] as HistoryItem[])
  const [error, setError] = React.useState(null as null | string)

  async function getHistory(
    ...args
  ) {
    setState(RequestState.LOADING)
    try {

      ...fetch data...

      setState(RequestState.SUCCESS)
      setData(data)
    } catch (error) {
      setState(RequestState.ERROR)
      setError(error)
      setData([])
    }
  }

  return {
    data,
    error,
    state,
    getHistory
  }
}

Then the loading state can be applied in the view more accurately and better communicate what it should be like -

{ hookLoadingState === RequestState.IDLE || RequestState.LOADING ? <Loader /> : <..view stuff... /> }

[aristidesstaffieri]

@piyalbasu @CassioMG off the cuff, what do you all think of this data fetching pattern?
I could also go with something closer to what exists today by implementing a different store.

My thinking is that when we fetch data that must be as fresh as possible, like history, we should avoid keeping the state in a store because it requires manual resetting/cleaning up of store state. A hook to do the same thing can "piggy-back" on the react component lifecycle and requires no clean up but provides us the same reusability.

[CassioMG]

@aristidesstaffieri I like your approach of modeling the loading state with more values, I think it's a nice improvement.

I have a few questions and suggestions below.

{ hookLoadingState === RequestState.IDLE || RequestState.LOADING ? <Loader /> : <..view stuff... /> }
^ are you proposing that the <Loader /> should also be shown on the IDLE state? Is my understanding correct here?

Could we display a little spinner ON TOP of the Transactions List instead of replacing the whole list with a Loader component while things are loading? This way users would never lose sight of the existing transactions, meaning it'd be a little less disruptive from a UX perspective

How do we feel about using an object instead of an enum to represent the request status? Something like:

export interface RequestState {
  isLoading: boolean;
  hasLoaded: boolean;
  isSuccess: boolean;
  error?: any;
}

I think either way works fine, but since we have only a few different possibilities in regards to a request response I personally find the ergonomics of the object a little better so we could simply check state.isLoading instead of comparing enum values with ===. The hasLoaded state there should mean the request "is done" regardless if it was a success or not. We'd also have the option to handle the error object if we want to.

The hook would look like this with the object approach:

export function useGetHistory() {
  const [state, setState] = React.useState({
    isLoading: false,
    hasLoaded: false,
    isSuccess: false,
  });
  const [data, setData] = React.useState([] as HistoryItem[]);

  async function getHistory(
    ...args
  ) {
    setState({
      isLoading: true,
      hasLoaded: false,
      isSuccess: false,
    });
    try {

      ...fetch data...

      setState({
        isLoading: false,
        hasLoaded: true,
        isSuccess: true,
      });
      setData(data);
    } catch (error) {
      setState({
        isLoading: false,
        hasLoaded: true,
        isSuccess: false,
        error,
      });
      setData([]);
    }
  }

  return {
    data,
    state,
    getHistory
  }
}

What do you guys think?

[aristidesstaffieri]

@CassioMG

{ hookLoadingState === RequestState.IDLE || RequestState.LOADING ? : <..view stuff... /> }
^ are you proposing that the should also be shown on the IDLE state? Is my understanding correct here?

Could we display a little spinner ON TOP of the Transactions List instead of replacing the whole list with a Loader component while >things are loading? This way users would never lose sight of the existing transactions, meaning it'd be a little less disruptive from a UX >perspective

yeah so for history, there are no existing transactions since we load the data with every render of the page. I think every consuming component can choose what arrangement of loading state to use to trigger the loader but in the case of history I think it works to show a loader for the idle and loading state so that the end use experience is that the user sees a loader and then the history without an intermediary glitch.

How do we feel about using an object instead of an enum to represent the request status? Something like:

export interface RequestState {
isLoading: boolean;
hasLoaded: boolean;
isSuccess: boolean;
error?: any;
}
I think either way works fine, but since we have only a few different possibilities in regards to a request response I personally find the ergonomics of the object a little better so we could simply check state.isLoading instead of comparing enum values with ===. The hasLoaded state there should mean the request "is done" regardless if it was a success or not. We'd also have the option to handle the error object if we want to.

Yeah I think I see what you're after with the object. Typically in React it's not recommended to model your state for useState as an object in cases where you need to update keys independently because internally it uses shallow comparison for state diffs which needs to be handled more delicately.

I think we can get something similar though by using some type constraints and switching the state to a useReducer instead., maybe like this?

interface SuccessState<T> {
  loading: false;
  data: T;
  error: null;
}

interface ErrorState<T> {
  loading: false;
  data: null;
  error: T;
}

interface InitialState {
  loading: false;
  data: null;
  error: null;
}

interface LoadingState {
  loading: true;
  data: null;
  error: null;
}

type State<T> = InitialState | LoadingState | SuccessState<T> | ErrorState<T>

type Action<T> =
  | { type: 'FETCH_DATA_START' }
  | { type: 'FETCH_DATA_SUCCESS'; payload: T }
  | { type: 'FETCH_DATA_ERROR'; payload: T };

const initialState: IdleState = {
  loading: false,
  data: null,
  error: null,
};

const reducer = <T>(state: State<T>, action: Action<T>): State<T> => {
  switch (action.type) {
    case 'FETCH_DATA_START':
      return { loading: true, error: null, data: null };
    case 'FETCH_DATA_SUCCESS':
      return { error: null, loading: false, data: action.payload };
    case 'FETCH_DATA_ERROR':
      return { data: null, loading: false, error: action.payload };
    default:
      return state;
  }
};

The use of useReducer let's us model the state as one object without the useState pitfalls. Additionally, modeling the state as a union of the different possible states makes the impossible states actually impossible. This forces clients to correctly handle the state(there can't be anything in error if the state is success for example).

I'm not sure if we really need dataLoaded? It seems like it is functionally the same as state === success | error but introduces another value to track that composite state, maybe we could ship a little helper with the hook as well to cover that?

const isDone = (state) => state === success | error

What do you think of all of this?

[CassioMG]

yeah so for history, there are no existing transactions since we load the data with every render of the page.

ah, I see what you mean. We won't have a persistent data to be shown while the new fetch is in progress and this would result in a UI glitch. Makes sense!

I now also better understand what the IdleState is for, this is the initial state where the app hasn't fetched anything yet. It was not very clear to me at the beginning. I initially thought it was a state which should be used whenever the app was not fetching anything (e.g. after a successful fetch it would come back to the idle state). Should we rename it to InitialState instead?

I'm not sure if we really need dataLoaded?

yeah we don't really need it, it was more like a convenient prop so we wouldn't need the extra helper. But with the reducer approach I agree that having this extra DataLoadedState state would add more complexity than it's worth.

I like the reducer approach, it seems more robust and reliable than the other approach. :)

[aristidesstaffieri]

yeah so for history, there are no existing transactions since we load the data with every render of the page.

ah, I see what you mean. We won't have a persistent data to be shown while the new fetch is in progress and this would result in a UI glitch. Makes sense!

I now also better understand what the IdleState is for, this is the initial state where the app hasn't fetched anything yet. It was not very clear to me at the beginning. I initially thought it was a state which should be used whenever the app was not fetching anything (e.g. after a successful fetch it would come back to the idle state). Should we rename it to InitialState instead?

I'm not sure if we really need dataLoaded?

yeah we don't really need it, it was more like a convenient prop so we wouldn't need the extra helper. But with the reducer approach I agree that having this extra DataLoadedState state would add more complexity than it's worth.

I like the reducer approach, it seems more robust and reliable than the other approach. :)

Ah yeah sorry I should have explained that better. Yeah I think InitialState makes enough sense as well. I'll rename it to that.