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

Should new networkReducer replace requestsReducer #275

Closed
klis87 opened this issue Aug 22, 2019 · 7 comments
Closed

Should new networkReducer replace requestsReducer #275

klis87 opened this issue Aug 22, 2019 · 7 comments

Comments

@klis87
Copy link
Owner

klis87 commented Aug 22, 2019

At https://github.com/klis87/redux-saga-requests/tree/network-reducer/examples/mutations/src you can see NetworkReducer I will release as a new version. It is like a manager of requestsReducers , which means that:

  • your won't need to use requestsReducer anymore, actually no more reducers for your remote data
  • you won't need to write selectors for your remote state, as everything will be kept in one reducer and remote state organized by action types
  • easier to use React bindings, as you could get a given state again just by providing action type

The question is now, does it make any sense to keep requestsReducer as part of API. Keeping it as an internal representation would make things much easier, as now for instance operations/mutations need to be handled differently when using networkReducer and requestsReducer(directly).

So I think we should get rid of requestsReducer completely, but I am raising this issue here as many could disagree

@klis87
Copy link
Owner Author

klis87 commented Sep 1, 2019

The more I think about it, the bigger change I will remove requestsReducer, as it will be much easier for me then to implement incoming features like cache persistence.

Also, having both requestsReducer and networkReducer makes docs complicated and possibly hard to understand, especially after dividing requests into queries and mutations. My priority is to make this library simple to learn and use, and networkReducer together with selectors can do everything requestsReducer can.

@strdr4605
Copy link
Contributor

strdr4605 commented Oct 10, 2019

The idea behind the networkReducer is interesting, but this approach is new and different from what developers used to do. One of the biggest issue for my team was a strict separation of the network state and the local state which is close to the network state.

Example:

API provides a list of items and users can filter them.

Some developers will implement (using TypeScript, typesafe-actions, and redux-saga-requests) this functionality like:

export interface State {
  filteredItems: IItem[];
  items: IItem[];
  pending: number;
  error: AxiosException | null;
}

const initialState: State = {
  filteredItems: [],
  items: [],
  pending: 0,
  error: null,
};

export function itemsReducer(state: State = initialState, action: ActionType | AnyAction): State {
  if (isActionOf(actions.setFilteredItems, action)) {
    return { ...state, filteredItems: action.payload };
  }
  if (ActionEnum.API_ITEMS_GET_REQUEST === action.type) {
    return { ...state, pending: state.pending + 1 };
  }
  if (success(ActionEnum.API_ITEMS_GET_REQUEST) === action.type) {
    return { ...state, items: action.payload.data.value, pending: state.pending - 1 };
  }
  if (error(ActionEnum.API_ITEMS_GET_REQUEST) === action.type) {
    return { ...state, error: action.payload, pending: state.pending - 1 };
  }
  return state;
}

In example above, items and filteredItems are closed related. And in React components (using Hooks NOT connect, for the sake of example) they can be accessed like:

const items: IItems[] = useSelector<RootState, IItems[]>(
    (state: RootState) => state.itemsState.items,
  );
const filteredItems: IItems[] = useSelector<RootState, IItems[]>(
    (state: RootState) => state.itemsState.filteredItems,
  );

If using networkReducer the items will be accessed like:

const items: IItems[] = useSelector<RootState, IItems[]>(
    (state: RootState) => getQuery<IItems[]>({ type: ActionEnum.API_ITEMS_GET_REQUEST })(state).data,
  );

and probably filteredItems will be in a reducer under state.local.itemsState and accessed like:

const filteredItems: IItems[] = useSelector<RootState, IItems[]>(
    (state: RootState) => state.local.itemsState.filteredItems,
  );

It can be hard to work with and debug in Redux DevTools when the state will become complex with a lot of requests and local reducers.
This approach also can be confused for developers that will join the project in the future and probably maintain the "legacy codebase" 😅. (Along with other requirements to start working on react/react-native app with redux-saga-requests, reading redux-saga-requests docs should be a top priority)

Also, in my opinion, to truly understand queries and mutations first you should read the API several times, then make some pet project, then read again, and again then implementing in production. Can you provide more samples and explanations on this topic?


I think the API for redux-saga-requests should be separated in several .md docs for each feature.
If you bet so much on networkReducer I would suggest having a separate section where you compare networkReducer, requestsReducer and custom Reducer (something like Motivation).
Code snippets with different approaches, pros and cons will be great for teams to see if there share the same mindset in creating reducers and managing state from components and if this approach works for them.
Having sample projects is also a good idea but they should be commented with explanations why?how?...


Looking forward to seeing new features, improvements, and docs. Maybe one day my current project will work with networkReducer and use all the power of this package. But I will definitely consider this package for the next projects.

@klis87
Copy link
Owner Author

klis87 commented Oct 11, 2019

@strdr4605 thx for detailed feedback

regarding 1st part, there are several things. first of all, according to best programming practices and redux guidance you should never keep filtered state in reducer, you should just use selector do derive from state (unless in your case filtered items are sth else than items)

putting this aside, if you have a local state you would like to merge with remote state, it can be easily achieved by selectors, like:

const remoteBooksSelector = getQuery({ type: 'FETCH_BOOKS', multiple: true });

const localBooksSelector = state => state.books;

const mergeBooks = createSelector(
  remoteBooksSelector,
  localBooksSelector,
  (remoteBooks, localBooks) => ({
    ...remoteBooks,
   data: [...remoteBooks.data, ...localBooks],
  }),
)

This is just an example, but generally it does not matter where state is stored, as long as it is easy to find, and networkReducer keeps everything by action type

I know that having the whole remote state in 1 place could be considered as less flexibility, but really if you use selectors and you should, then it does not matter really.

And in exchange I will accomplish:

  • caching with TTL support, also with possibility to cache multiple versions of a query type
  • super easy ssr inside redux itself, no react involved
  • optional automatic cache persistence, like in local storage
  • automatic normalisation and denormalisation
  • automatic optimized selectors for remote state

All those things will work simultaneously and with simplified API in comparison to current version. This will be possible, because

  • remote state is in 1 place,
  • library knows where to find remote state as it stored like designed by the library

Also performance will be better, as many checks in 1 networkReducer will be done once per action instead of multiplifying it by number or requestsReducers


Regarding React native it should just work, it should also work for Angular and anything really, as this is just Redux addon.


Examples you can find here https://github.com/klis87/redux-saga-requests/tree/master/examples


Regarding docs, I am completely aware of it, I need to finish all the features 1st though before I work on it. Getting rid of requestsReducer will make things simpler too as now API in multiple places is more complex as I need to support 2 alternative usages - requestsReducer and networkReducer

Also, before 1.0 I will move docs to https://docusaurus.io/ , with included tutorials and guides too ofc.

@strdr4605
Copy link
Contributor

@klis87, as the packages weren't updated in a while, can you please create a milestone with the status of all the changes and features that will be implemented. So it can be clear for me and my team if we consider using redux-saga-requests as core middleware.
Probably as you mentioned better to create 2 milestones v0.x.x and v1.x.x.
Also would be nice to have a good issue description of all features and maybe I and other users can contribute to finishing everything faster.

@klis87
Copy link
Owner Author

klis87 commented Jan 15, 2020

@strdr4605 good idea, I will definitely do this in the following days. If you are wondering btw, development hasnt stopped, just everything is going on dev branch

@klis87
Copy link
Owner Author

klis87 commented Jan 17, 2020

@strdr4605 see roadmap here #306

@klis87
Copy link
Owner Author

klis87 commented Mar 22, 2020

Done in v0.27.0

@klis87 klis87 closed this as completed Mar 22, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants