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

Add a utility to follow thunk network status #550

Closed
HHK1 opened this issue May 11, 2020 · 6 comments
Closed

Add a utility to follow thunk network status #550

HHK1 opened this issue May 11, 2020 · 6 comments

Comments

@HHK1
Copy link
Contributor

HHK1 commented May 11, 2020

Possibly related issues:

#418

Motivation

Something I've been doing over and over when I'm creating a thunk, is to add a loading and error properties in the corresponding slice of my store, in a similar fashion as what's described in the tutorial.

In the most simple implementations, loading is a boolean, or in more advanced ones an enum like this one from the Apollo library: https://github.com/apollographql/apollo-client/blob/master/src/core/networkStatus.ts

I've found it very useful when using Apollo to have this advanced loading state/ network status provided by default, especially for paginated requests.
Would it fall in the scope for Redux-toolkit to provide a mechanism to create those for a thunk ?

Implementation suggestions

I've done a simple example where I'm creating a reducer given a thunk (returned from createAsyncThunk.) Handling of the various loading state, concurrent requests etc. is not complete (fechMore, polling are missing) but all that logic could be implemented here and provide a nice default to properly handle loading state.


export enum LoadingStatus {
  /** Initial Load, no previous data */
  loading = 1,

  /** Data has been refteched manually */
  refetch = 2,

  /** Data is paginated and another page is loading */
  fetchMore = 3,

  /** Data is polling and a new request has started */
  poll = 4,

  /** Loaded successfully */
  ready = 5,

  /** Network call failed  */
  error = 6,
}


export interface ThunkMetaState {
  loading: LoadingStatus | null;
  error: SerializedError | null;
}

const initialState: ThunkMetaState = {
  loading: null,
  error: null,
};

export const createThunkMetaReducer = <Return, ThunkArg>(
  thunk: AsyncThunk<Return, ThunkArg>
): Reducer<ThunkMetaState, AnyAction> => {
  const metaReducer = createReducer<ThunkMetaState>(initialState, builder => {
    builder.addCase(thunk.pending, state => {
      state.loading = state.loading !== null ? LoadingStatus.refetch : LoadingStatus.loading;
      state.error = null;
    });
    builder.addCase(thunk.fulfilled, state => {
      state.loading = LoadingStatus.ready;
      state.error = null;
    });
    builder.addCase(thunk.rejected, (state, action) => {
      state.loading = LoadingStatus.error;
      state.error = action.error;
    });
  });

  return metaReducer;
};
@markerikson
Copy link
Collaborator

Yeah, loading state is a complex field of thought. Part of the point of createAsyncThunk is that it doesn't dictate how you're fetching your data, processing that data in reducers, or handling the loading state - it just dispatches actions based on the lifecycle of the promise you've returned.

I agree that data fetching patterns seem to be the main focus in much of today's development world, and that there is potential value in having more built-in abstractions that simplify that. On the other hand, I don't want RTK to start turning into something like Apollo itself. I refuse to get involved in complex questions like caching, because A) that's out of scope for RTK itself, B) I don't have the knowledge to work on that sort of thing, and C) I have far too many other things on my plate to spend time on that sort of stuff.

Given that we've had success porting @ngrx/entity to RTK, I am sorta vaguely interested in keeping an eye on https://ngrx.io/guide/data as a higher-level data fetching abstraction to maybe investigate somewhere down the road.

Specific to the loading state question, David Khourshid has been strongly urging folks to manage that using enums and state machines, and he specifically added an explanatory section to the "treat reducers as state machines" section of the Redux Style Guide showing how that can work.

Summarizing all that: I'm not against having some built-in pieces that manage loading state somehow, but I do see it as a potential trap of sorts that could lead to a lot of time spent bikeshedding and trying to cover too many use cases at once. I'm open to suggestions, but it's not something that's a high priority for me right now.

@HHK1
Copy link
Contributor Author

HHK1 commented May 12, 2020

Ok makes total sense !

I'm not against having some built-in pieces that manage loading state somehow, but I do see it as a potential trap of sorts that could lead to a lot of time spent bikeshedding and trying to cover too many use cases at once. I'm open to suggestions, but it's not something that's a high priority for me right now.

My idea is to have something similar to createEntityAdapter: it covers the vast majority of uses cases, and if you have specific needs, you can write your own reducer. So no caching, only metadata relative to the loading state and the error for that thunk.

If you want to, I can post more code samples as I explore the idea, and if you have additional guidance or examples from other projects, I'm all ears

@markerikson
Copy link
Collaborator

Sure, it's always easier to discuss concrete code ideas :)

@vkochev
Copy link

vkochev commented May 15, 2020

What do you think of creating a separate slice that stores all data fetch states of an application?

const requestStatesAdapter = createEntityAdapter<RequestState>({
  selectId: (requestState) => requestState.id,
});

const slice = createSlice({
  name: 'requestStates',
  initialState: requestStatesAdapter.getInitialState(),
  reducers: {
    setInProgress(state, { payload: { id } }: PayloadAction<WithId<InProgressState>>) {
      requestStatesAdapter.upsertOne(state, { isLoading: true, id, error: null });
    },
    setFailed(state, { payload: { id, error } }: PayloadAction<WithId<FailedState>>) {
      requestStatesAdapter.upsertOne(state, { isLoading: false, id, error });
    },
    setSucceed(state, { payload: { id } }: PayloadAction<WithId>) {
      requestStatesAdapter.upsertOne(state, { isLoading: false, id, error: null });
    },
    clearState: requestStatesAdapter.removeOne,
  },
});

It can be used as an event bus for all requests. Errors can be handled in both ways: by custom entity's components and by a universal component of requests errors.
It's a way not to think about the request state for every entity.

But one thing. It requires a component to know what ID it has to use to get the request state.
AFAIK, the toolkit uses nanoid and there is no way to pass a custom request ID generator to payloadCreator.
But request ID can be passed via arg.

What do you think? Is it a kind of suitable approach?

@markerikson
Copy link
Collaborator

Having said that "I don't want to get involved in caching", I am unfortunately starting to rethink that, per #603 .

@markerikson
Copy link
Collaborator

The upcoming "RTK Query" API for Redux Toolkit seems to cover this:

https://rtk-query-docs.netlify.app

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants