-
-
Notifications
You must be signed in to change notification settings - Fork 51
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
Comments
The more I think about it, the bigger change I will remove Also, having both |
The idea behind the Example:
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 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 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. 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 Looking forward to seeing new features, improvements, and docs. Maybe one day my current project will work with |
@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:
All those things will work simultaneously and with simplified API in comparison to current version. This will be possible, because
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 Also, before 1.0 I will move docs to https://docusaurus.io/ , with included tutorials and guides too ofc. |
@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. |
@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 |
@strdr4605 see roadmap here #306 |
Done in v0.27.0 |
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 ofrequestsReducer
s , which means that:requestsReducer
anymore, actually no more reducers for your remote dataThe 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 usingnetworkReducer
andrequestsReducer
(directly).So I think we should get rid of
requestsReducer
completely, but I am raising this issue here as many could disagreeThe text was updated successfully, but these errors were encountered: