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

Overcome design problem with shouldRun #21

Open
fnune opened this issue Jul 7, 2017 · 5 comments
Open

Overcome design problem with shouldRun #21

fnune opened this issue Jul 7, 2017 · 5 comments

Comments

@fnune
Copy link
Contributor

fnune commented Jul 7, 2017

Currently shouldRun and crudReducer don't play nice together. shouldRun runs after the crudReducer has already cleaned up some parts of the store, so the data is invalid.

Proposal:
// ENSURE_SINGLE
// FETCH_SINGLE

Running ensure will do the check with shouldRun and then result in either FETCH_SINGLE or nothing. Using fetchSingle directly will avoid the shouldRun check and will "force" the saga to run.

The ensure action should only be generated in crudActionsNamespace and not in the more generic actionsNamespace.

@fnune
Copy link
Contributor Author

fnune commented Jul 7, 2017

Maybe the best idea is to drop shouldRun completely because:

  • It introduces a lot of design problems.
  • It can also be handled in a lean way in the components.

@fnune
Copy link
Contributor Author

fnune commented Jul 7, 2017

Maybe make the saga responsible for managing loading states (which are the main reason why shouldRun and crudReducer don't work well together). Which part of the state to change is the responsibility of the saga.

@tomhoule
Copy link
Contributor

tomhoule commented Jul 7, 2017

Detailed proposal:

  • We create a very simple loadingReducer, a dictionary of type ActionType -> boolean
  • shouldRun runs, decides if the effect should run, dispatches SET_LOADING with the action type as a payload if so.
  • On success or failure, UNSET_LOADING with the action type

We should also warn users of the library not to do things they might regret on FETCH actions in reducers.

@peterfication
Copy link
Member

Or we could extract the shouldRun in a custom action/saga that dispatches FETCH if it is needed. So we only have one extra action type.

@tomhoule
Copy link
Contributor

tomhoule commented Jul 19, 2017

export const configurePerform = failureHandler => function* (effect, action) => {
  try {
    const response = yield effect
    yield put({
        type: action.meta.successAction,
        payload: response,
        meta: { originalAction: action },
    })
  } catch (error) {
    yield failureHandler(error, action, action.meta.failureAction)
  }
}


// shouldRun would look something like that inside a saga
if (condition) {
    yield put(campaignsActions.cancelLoading('index'))
    return
}



export const simplePerform = configurePerform(defaultFailureHandler)


const configureCrudReducer = extractors => (state, action, actionTypes) => {
}

const defaultExtractors = {
    index: (action, state) => action.payload,
    meta: (action, state) => action.payload,
    error: (action, state) => action.payload,
    show: (action, state) => action.payload,
}

-> we need to add a CANCEL_LOADING action to crudActionsNamespace

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