klis87 / redux-requests

Declarative AJAX requests and automatic network state management for single-page applications
https://redux-requests.klisiczynski.com/
MIT License
367 stars 51 forks source link

Network history proposal #5

Closed poteirard closed 6 years ago

poteirard commented 6 years ago

Hello !

First of all thank you for this package. Our application has many api requests calls and this is going to make our life much simpler.

After playing with your library we realized we could make something we called "network history". It is basically a root reducer which takes all the actions (success, error, request) and with that it fills a network tree in the state that later we use to populate our app with selectors.

Here is the code I have so far:

import {createRequestInstance, watchRequests} from "redux-saga-requests/src";
import {isRequestAction} from "redux-saga-requests/src/sagas";
import axiosConfig from './axiosConfig'

const initialState = {};

/**
 * All network response actions has meta.request object in the payload
 */
const isNetworkResponseAction = action => 
  action.payload
  && action.payload.meta
  && action.payload.meta.request

/**
 * A success network action always has data in the meta
 */
const isSuccessAction = action => isNetworkResponseAction(action) && action.payload.data;

/**
 * An error network action always has error in the meta
 */
const isErrorAction = action => isNetworkResponseAction(action) && action.payload.error;

/**
 * REDUCER
 */
export default function network(state = initialState, action) {

  if (isRequestAction(action)) {

    const {type} = action;
    return {
      ...state,
      [type]: {
        ...state[type],
        fetching: true,
      },
    }
  }
  if (isSuccessAction(action)) {

    return {
      ...state,
      [action.payload.meta.type]: {
        fetching: false,
        data: action.payload.data,
        error: '',
      },
    }
  }
  if (isErrorAction(action)) {
    return {
      ...state,
      [action.payload.meta.type]: {
        ...state[action.payload.meta.type],
        fetching: false,
        error: 'Error',
      },
    }
  }
  //We can also have _ABORT action but we still not need it

  return state;
}

/**
 * SELECTORS
 */

export const selectNetworkResource = (state, resource: string) => state.network[resource] && state.network[resource].data

/**
 * LISTENERS
 */
export function* networkListeners() {
  yield createRequestInstance(axiosConfig)
  yield watchRequests();
}

After adding this network reducer in the root reducer with combineReducers we have a tree like this when we have one success call: webpack_app

So now we can use a selector in mapStateToProps to access the data:

const mapStateToProps = state => ({
  products: selectNetworkResource(state, PRODUCTS_REQUEST) || [],
});

We do this in orther to get rid of all the network related logic, in you example this:

- const booksReducer = (state = defaultState, action) => {
-     switch (action.type) {
-       case FETCH_BOOKS:
-         return { ...defaultState, fetching: true };
-      case FETCH_BOOKS_SUCCESS:
-      case success(FETCH_BOOKS):
-         return {
-           ...defaultState,
-           data: { ...action.payload.data },
-         };
-      case FETCH_BOOKS_ERROR:
-      case error(FETCH_BOOKS):
-         return { ...defaultState, error: true };
-       default:
-         return state;
-     }
-   };

As you see in the network reducer we don't have an easy way to know which actions are SUCCESS, ERROR, ABORT...

klis87 commented 6 years ago

Hello!

I am glad you find this library useful. Really an interesting idea with abstracting reducers as well. Regarding your question, your logic to check SUCCESS, ERROR is correct - but the easiest I guess would be to just check an action type's suffix - like isSuccess = action => action.type.endsWith('_SUCCESS') (or another suffix you set for your app).

If really think we could consider adding a reducer to this library, as indeed most of the logic could be abstracted for different api calls. However, instead of network reducer, maybe it would be more flexible to create a higher order reducer, which could handle state change for success, error and abort actions, but it could allow people to add handlers for different action types as well? Also, such reducers should handle only get and read only requests I think, there is no point for storing responses for UPDATE, EDIT, DELETE type of requests, what do you think?

poteirard commented 6 years ago

Hey !

Regarding your question, your logic to check SUCCESS, ERROR is correct - but the easiest I guess would be to just check an action type's suffix - like isSuccess = action => action.type.endsWith('_SUCCESS') (or another suffix you set for your app).

I thought of going to suffix but I think it can have unexpected errors. SUCCESS is a very common word. My suggestion would be to use custom types names, like react-router has in his actions . If that's not possible, my suggestion would be to add a meta key in every action of the library to make the check easier. What do you thing?

it would be more flexible to create a higher order reducer, which could handle state change for success, error and abort actions, but it could allow people to add handlers for different action types as well?

Excellent idea, I didn't know about high order reducers until know that I made a bit of research. It makes totally sense.

Also, such reducers should handle only get and read only requests I think, there is no point for storing responses for UPDATE, EDIT, DELETE type of requests, what do you think?

It depends on the needs of your app. For example we need to have a history of all the things that are happening in the network for logging purposes.

poteirard commented 6 years ago

it would be more flexible to create a higher order reducer, which could handle state change for success, error and abort actions, but it could allow people to add handlers for different action types as well?

After thinking about it, does it make sense? I mean you can actually create your own reducer like:

  const booksReducer = (state = defaultState, action) => {
    switch (action.type) {
      case FETCH_BOOKS:
        return { ...defaultState, fetching: true };
       case success(FETCH_BOOKS):
        return {
          ...defaultState,
          data: { ...action.payload.data },
        };
      case FETCH_BOOKS_ERROR:
      case error(FETCH_BOOKS):
        return { ...defaultState, error: true };
      default:
        return state;
    }
  };
klis87 commented 6 years ago

You can change suffixes like in this example. It uses redux-act but you can use this technique without it as well. I really should add this to docs btw, I will do it soon.

Regarding your reducer, why do you do duplicated cases in your reducers?

case FETCH_BOOKS_ERROR:
case error(FETCH_BOOKS):

both cases are the same, error, success, abort functions are only used as convenience so you dont need to add suffixes yourself.

Regarding higher order reducer, I mean sth like this:

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

const getInitialState = (state, reducer) => {
  if (!reducer) {
    return initialState;
  }

  return reducer(undefined, {});
};

const requestsReducer = (actionType, reducer) => (state, action) => {
  // to get reducer initial state when state is undefined
  state = state === undefined ? getInitialState(state, reducer) : state;

  switch (action.type) {
    case actionType:
      return { ...state, fetching: true, data: null, error: null };
    case success(actionType):
      return { ...state, fetching: false, data: action.payload.data, error: null };
    case error(actionType):
      return { ...state, data: null, fetching: false, error: action.payload.error };
    default:
      return reducer ? reducer(state) : state;
  }
};

const initialPhotoState = {
  data: null,
  fetching: false,
  error: null,
};

const photoReducer = (state = initialPhotoState, action) => {
  switch (action.type) {
    case 'CLEAR_PHOTO':
      return initialPhotoState;
    default:
      return state;
  }
};

const photoReducerWithRequestLogic = requestsReducer('FETCH_PHOTO', photoReducer);
const booksSimpleReducerWithJustRequestLogic = requestsReducer('FETCH_BOOKS');

This is just a prototype, but real code would be mostly the same. With requestsReducer, you can quickly create new reducers. They can be really simple with data fetching and error key in state, but they could also do more things like photoReducer with additional CLEAR_PHOTO case, generally requestsReducer could add networking logic to any reducer.

poteirard commented 6 years ago

why do you do duplicated cases in your reducers?

Because I made a copy paste of your diff example and I forgot to delete that line. Sorry my bad.

You can change suffixes

I didn't know, that's really nice !

Regarding higher order reducer, I mean sth like this

I understand your point now. It's really interesting and I'm going to start working on it right now !

poteirard commented 6 years ago

I just made the first approach:

import {createRequestInstance, watchRequests, error, success} from 'redux-saga-requests/src';
import {Reducer} from "redux";

interface INetworkInterface {
  network: {
    [actionType: string]: {
      data: null,
      fetching: false,
      error: null,
    }
  }
  local: any
}

/**
 * REDUCER
 */
function network(state, action, requestActionType) {
  switch (action.type) {
    case requestActionType:
      return {
        ...state,
        fetching: true,
        data: null,
        error: null,
      }
    case success(requestActionType):
      return {
        ...state,
        fetching: false,
        data: action.payload.data,
        error: null,
      }
    case error(requestActionType):
      return {
        ...state,
        fetching: false,
        data: null,
        error: action.payload.error
      }
    default:
      return {
        ...state,
        fetching: false,
        data: null,
        error: null
      }
  }
}

const getInitialState = (state, reducer) => {
  const initialState: INetworkInterface = {
    network: {},
    local: {}
  };

  return reducer
    ? {...initialState, local: reducer(undefined, {})}
    : initialState
};

/**
 * HIGHER ORDER REDUCER
 */
export default (requestActionTypes: string[], reducer?: Reducer<any>) => (state, action) => {

  state = state === undefined ? getInitialState(state, reducer) : state;

  const nextState = {...state}

  requestActionTypes.forEach((requestActionType) => {
    nextState.network = {
        [requestActionType]: {
          ...nextState.network[requestActionType],
          ...network(nextState.network[requestActionType], action, requestActionType),
      }

    }
  })

  if (reducer) {
    nextState.local = {
      ...nextState.local,
      ...reducer(nextState.local, action),
    }
  }

  return nextState
};

/**
 * LISTENERS
 */
import axiosConfig from './api/axiosConfig';

export function* networkListeners() {
  yield createRequestInstance(axiosConfig);
  yield watchRequests();
}

Then you can use it like this

export default requestReducer([PRODUCTS_REQUEST, OPTIONS_REQUEST, ...], LandingPage)

And you will end up with a tree like this: webpack_app

klis87 commented 6 years ago

Thanks for this.

Looking at your code, I think that your version of higher order reducer cannot go into this package directly, as it is too specific to your use case. However, I really think that requestsReducer I suggested below is flexible enough (and we will be able to make it even more flexible by providing some config).

This requestsReducer could be used as the building block for you scenario as well. Here is my suggestion:

// redux-saga-requests code

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

const getInitialState = (state, reducer) => {
  if (!reducer) {
    return initialState;
  }

  return reducer(undefined, {});
};

const requestsReducer = (actionType, reducer) => (state, action) => {
  // to get reducer initial state when state is undefined
  state = state === undefined ? getInitialState(state, reducer) : state;

  switch (action.type) {
    case actionType:
      return { ...state, fetching: true, data: null, error: null };
    case success(actionType):
      return { ...state, fetching: false, data: action.payload.data, error: null };
    case error(actionType):
      return { ...state, data: null, fetching: false, error: action.payload.error };
    default:
      return reducer ? reducer(state) : state;
  }
};

// app code

import { combineReducers } from 'redux';
import { requestsReducer } from 'redux-saga-requests';

import { LandingPage } from 'reducers';
import { PRODUCTS_REQUEST, OPTIONS_REQUEST } from 'constants';

const initialState = {
  network: {},
  local: {}
};

const getNetworkReducers = actionTypes => (
  actionTypes.reduce((prev, current) => {
    return { ...prev, [current]: requestsReducer(current) };
  }, {})
);

const network = combineReducers(getNetworkReducers([PRODUCTS_REQUEST, OPTIONS_REQUEST]));

/* or just...
const network = combineReducers({
  PRODUCTS_REQUEST: requestsReducer(PRODUCTS_REQUEST),
  OPTIONS_REQUEST: requestsReducer(OPTIONS_REQUEST),
});
*/

const app = combineReducers({
  network,
  local: LandingPage
});

What do you think? If you like it, are you interested in making a PR to add requestsReducer to this package?

poteirard commented 6 years ago

You are right.

This is my final code after your suggestions:

import { createRequestInstance, watchRequests, error, success } from 'redux-saga-requests/src';
import { Reducer, combineReducers } from 'redux';

/**
 * START redux-saga-requests code (future PR in the library)
 */
interface IInitialState {
  data: any;
  fetching: boolean;
  error: any;
}

const initialState: IInitialState = {
  data: null,
  fetching: false,
  error: null,
};

const getInitialState = (state, reducer) => reducer 
  ? reducer(undefined, {}) 
  : initialState;

const requestsReducer = (actionType: string, reducer?: Reducer<any>) => (state, action) => {
  // to get reducer initial state when state is undefined
  const newState = state === undefined ? getInitialState(state, reducer) : state;

  switch (action.type) {
    case actionType:
      return { ...newState, fetching: true, data: null, error: null };
    case success(actionType):
      return { ...newState, fetching: false, data: action.payload.data, error: null };
    case error(actionType):
      return { ...newState, data: null, fetching: false, error: action.payload.error };
    default:
      return reducer ? reducer(newState, action) : newState;
  }
};

/**
 * END redux-saga-requests code
 */

const getNetworkReducers = actionTypes => (
  actionTypes.reduce((prev, current) => {
    return { ...prev, [current]: requestsReducer(current) };
  }, {})
);

export default (actionTypes: string[], reducer?: Reducer<any>) => {
  const network = combineReducers(getNetworkReducers(actionTypes));
  return combineReducers(reducer ? { network, local: reducer } : { network });
};

/**
 * LISTENERS
 */
import axios from 'axios';

export function* networkListeners() {
  yield createRequestInstance(
    axios.create({
      baseURL: `${window.location.origin}/api`, // global axios config
    },
    ),
  );
  yield watchRequests();
}

What do you think? It's basically the same but I changed my app code.

If you like the code I can begin working on the tests and documentation for the PR.

Thanks

klis87 commented 6 years ago

It is looking great. We will need to extend it slightly to support custom suffixes (by adding factory to create requestsReducer, but we will tackle this later :)

Thank you!

klis87 commented 6 years ago

@ebardajiz Regarding assumption 1, I think that we just need to document that if someone wants to use requestsReducer with custom reducer and custom initial state, then its initial state must be an object - if not, then there is no point using requestsReducer in the first place, as object is optimal for storing pending, data and error states, you wouldn't be able to achieve this with string (not it a normal way :)).

Regarding question 2, I think we should made those key names configurable. This is what I recommend:

import { success, error } from 'redux-saga-requests';

const defaultConfig = {
  getSuccessSuffix: success,
  getErrorSuffix: error,
  dataKey: 'data',
  errorKey: 'error',
  fetchingKey: 'fetching',
};

const getInitialState = (state, reducer, { dataKey, errorKey, fetchingKey }) => {
  if (!reducer) {
    return {
      [dataKey]: null,
      [fetchingKey]: false,
      [errorKey]: null,
    };
  }

  return reducer(undefined, {});
};

export const createRequestsReducer = (userConfig = {}) => (actionType, reducer) => (state, action) => {
  const config = { ...defaultConfig, ...userConfig };

  // to get reducer initial state when state is undefined
  const newState = state === undefined ? getInitialState(state, reducer, config) : state;

  const {
    getSuccessSuffix,
    getErrorSuffix,
    dataKey,
    errorKey,
    fetchingKey,
  } = config;

  switch (action.type) {
    case actionType:
      return {
        ...newState,
        [dataKey]: null,
        [fetchingKey]: true,
        [errorKey]: null
      };
    case getSuccessSuffix(actionType):
      return {
        ...newState,
        [dataKey]: action.payload.data,
        [fetchingKey]: false,
        [errorKey]: null
      };
    case getErrorSuffix(actionType):
      return {
        ...newState,
        [dataKey]: null,
        [fetchingKey]: false,
        [errorKey]: action.payload.error
      };
    default:
      return reducer ? reducer(newState) : newState;
  }
};

export const requestsReducer = createRequestsReducer(); // this will be exported as the default requestReducer

// someone could make own version of requestReducer
const customRequestReducer = createRequestsReducer({ fetchingKey: 'pending' });

This would also solve the problem with custom suffixes. Do you think this would give people enough flexibility?

klis87 commented 6 years ago

Btw, another think that comes to my mind, in getInitialState when reducer is there, should we return reducer(undefined, {}), or { ... { [dataKey]: null, [fetchingKey]: false, [errorKey]: null } , ...reducer(undefined, {}) }?

I think it is better just to return reducer(undefined, {}), otherwise we would have problems in cases like this:

const initialPhotoState = {
  data: null,
  fetching: false,
  error: null,
};

const photoReducer = (state = initialPhotoState, action) => {
  switch (action.type) {
    case 'CLEAR_PHOTO':
      return initialPhotoState;
    default:
      return state;
  }
};

With the second option, an app dev could just give initial state as {} and it would be magically extended by requestsReducer. But 2 problems I see is - it add too much magic, second, in CLEAR_PHOTO case you wouldn't be able to just return initialPhotoState, because then data, fetching and error keys would be lost. On the other side, for each custom reducer all of those keys would need to be explicitely defined in initialState. I cannot decide which option is better.

klis87 commented 6 years ago

I think you are right, this will be better. So, people would need to do the following for such custom reducer:

const initialPhotoState = {};

const photoReducer = (state = initialPhotoState, action) => {
  switch (action.type) {
    case 'CLEAR_PHOTO':
      return {
        data: null,
        fetching: false,
        error: null,
      };
    default:
      return state;
  }
};

This is the case I am little worried about, it seems confusing, because in CLEAR_PHOTO case we add keys we didn't have in initialState. Will people understand this "magic"? Someone could also define data, fetching, error in initialState explicitly. What do you think about this case?

poteirard commented 6 years ago

Provably that's why people like the ones of redux-undo injects a level in the top of the HOR's state. https://github.com/omnidan/redux-undo#history-api

klis87 commented 6 years ago

The problem I see with your initial solution is that it keeps all networking state in 1 place. You woudn't be able for example to create reducer with state like:

{
  data: null,
  pending: false,
  error: false,
  someOtherKeyRelatedToThisReducer: null // impossible with your approach, you would add this to local state which belongs really here
}

And moreover, what if a part of a state tree could be mutated by different api requests? for example user could be mutated by FETCH_USER, EDIT_USER, etc. With your solution, you would end up with duplicated user state.

Also, with centralized reducer, I don't actually understand local part there, it seems independent on network state, so instead of reducer injected in networkReducer with local state, you could just do normal networkReducer (not implemented as HOR as local state is separate anyway), and combine it with other reducers by combineReducers.

That's why I really prefer my suggestion, it is really more flexible, as you can achieve your centralized tree with example I showed you before, but you cannot extend any request related state with yours.

Regarding redo/undo, you can decorate multiple reducers with undoable, see this issue, for example to create separate redo undo actions for independent parts of your state.

I suggest continuing with the flexible approach, with getInitialState returning { ...{ [dataKey]: null, [fetchingKey]: false, [errorKey]: null }, ...reducer(undefined, {}) }

aryzing commented 6 years ago

You woudn't be able for example to create reducer with state like

Correct, and it was not my intention to be able to do so. The reasoning behind is that, should you have a books api, /api/books/*, there would probably be multiple endpoints and verbs used

and all of these would need their own { fetching, data, error } object, which I'd like to have "close" to the books reducer.

With your solution, you would end up with duplicated user state

The information would be potentially available in multiple places, although pointing to the same in-memory object.

Also, my particular view on fetching data from the network is that it might not always be in the best shape suitable to your application. Say that when contacting the books api, you get an array of books,

{
  "response": "200 OK",
  "books": [{"id": "guid1", "title": "Title 1"}, {"id": "guid2", "title": "Title 2"}]
}

So I always like to store the raw response in one place, and the transformed response in another. In this case, we may want to normalize the data by id. This way, when things don't work out as expected, you can always see what you received from the network, and how it was subsequently transformed. This is great for debugging.

local

Thats just the particular name that was chosen for the key whose state is controlled by the reducer passed to the HOR in an earlier version of the implementation. I completely agree, combine with combineReducers we can achieve the same functionality and I believe we are now using it.

klis87 commented 6 years ago

Correct, and it was not my intention to be able to do so. The reasoning behind is that, should you have a books api, /api/books/*, there would probably be multiple endpoints and verbs used GET /api/books (get all books) POST /api/books (create a new book) GET /api/books/by-author/agatha christie and all of these would need their own { fetching, data, error } object, which I'd like to have "close" to the books reducer.

But with your solution you will actually have 3 reducers, as all 3 endpoints you mentioned would have separate Redux actions, for example:

How would you implement creation of multiple books? For example:

Also, how would you handle books updates?

The information would be potentially available in multiple places, although pointing to the same in-memory object.

Could you please show some example, how would you reuse 1 object for a user with two actions, FETCH_USER and EDIT_USER?

So I always like to store the raw response in one place, and the transformed response in another.

Changing data structure could be handled in selectors. Duplicated data kept in reducer is again, suboptimal, why would you keep both versions of your data in your reducer, if you could derive from 1 version of your state by reselect? You made a good point here though. People should be allowed to process data in any way desired, for example, as you mentioned, to normalize the data by id. We could achieve this by passing a callback like processData, which would default to data => data. And what's nice, in my version of requestsReducer, each reducer could have a different processData, so it would be really flexible.

To sum up, I am sure all above use cases could be easily handled by my requestReducer. I cannot see how it could be done in yours (without duplicating the state and additional code in local reducer, which would defeat the purpose of reducer helper in the first place), but I am interested as maybe you have something in mind and I might be missing something. But if those things are hard, we really should choose the flexible approach, especially you could use it to build exactly your version networkReducer using my requestReducer.

aryzing commented 6 years ago

How would you implement creation of multiple books?

By having the reducer that is passed to the hor (will call it hor-ed reducer from now on) listen for the relevant network actions. That way:

how would you reuse 1 object for a user with two actions

What I meant was with the same action. If you have the different reducers listening to the same action, the same in-memory payload object can be added to two different parts of the store. This brings me back to (*1). If the data does not need to be normalized, you may chose to either pull it from the network reducer, or from the hor-ed reducer.

Out of curiosity, how would you handle fetching multiple different books? Wouldn't they be overwritten using your approach?

klis87 commented 6 years ago

Could you please give me a real code example for multi book creation and fetching?

Regarding reusing objects, I refered to different actions, with RESTful API, you can have multiple endpoints related to the same entity, hence we need multiple actions. So I guess, with you approach, you would create a reducer with a data state, waiting for actions like FETCH_USER, EDIT_USER, USER_LOGIN, USER_LOGOUT, DELETE_USER etc, and at the same time in network reducer data for each action type would be kept separately and duplicated?

Out of curiosity, how would you handle fetching multiple different books? Wouldn't they be overwritten using your approach?

const bookReducer = requestReducer('FETCH_BOOKS', (state, action) => {
  switch (action.type) {
    case success('CREATE_BOOK'):
      return { ...state, data: [...state.data, action.data] };
    default:
      return state;
  }
});
klis87 commented 6 years ago

Btw, in our previous examples, initial data was set to null. Personally for data like books, I would prefer data set as []. Then above code wouldn't fail if the created book was the first one (data would be null and you couldn't just do ...data). So actually, maybe API should look like this:

const bookReducer = requestReducer(
  { actionType: 'FETCH_BOOKS', multiple: true },
  (state, action) => {
    switch (action.type) {
      case success('CREATE_BOOK'):
        return { ...state, data: [...state.data, action.data] };
      default:
        return state;
    } 
  }
);
klis87 commented 6 years ago

If I understand your approach correctly, I believe that it's not kept in the state.

If you want to preserve the data, you could just allow requestReducer to save data as data => data, (this would be the default, but we could make it as configurable by passing processData callback to config of requestReducer.

In my example, only state for FETCH_BOOKS was kept in state. But you could easily track CREATE_BOOK as well by creating another reducer:

const newBookReducer = requestReducer({ actionType: 'CREATE_BOOK' });

And, if you wanted to keep track of all requests, you could use a helper I suggested in a previous post:

const getNetworkReducers = actionTypes => (
  actionTypes.reduce((prev, current) => {
    return { ...prev, [current]: requestsReducer(current) };
  }, {})
);

const network = combineReducers(getNetworkReducers([PRODUCTS_REQUEST, OPTIONS_REQUEST]));

Perhaps the issue at hand is that I'm giving the "raw" response from the server excessive importance, and should instead not bother with recording it. What do you think?

It depends on your use case. If you need it just for logging purposes, maybe it would be better to set some redux-saga-request interceptor? Or if you could need it later, then reducers are the best place to keep it. And with getNetworkReducers, you could easily achieve that.

Also, in the case where multiple endpoints could alter the state, for example, of known books (fetch books and fetch books by author), would you take an approach similar to mine or do you have another strategy in mind?

I like your approach, with my version of requestReducer, I just woudn't need handle FETCH_BOOKS cases, as this would be handled by the requestReducer.

klis87 commented 6 years ago

Closed with #13

klis87 commented 5 years ago

@poteirard long time no written, but for your information, this library changed a lot and with time centralizing all network state within one place made more and more sense. 0.26.0 version added networkReducer. Thx for the idea!