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

Create Async Action #76

Closed
jamescmartinez opened this issue Jan 10, 2019 · 66 comments
Closed

Create Async Action #76

jamescmartinez opened this issue Jan 10, 2019 · 66 comments
Labels
enhancement New feature or request
Milestone

Comments

@jamescmartinez
Copy link

redux-starter-kit includes redux-thunk but doesn't include an async version of createAction. I want to float the idea of a createAsyncAction function that uses thunks; I can create a pull request if all looks good.

Here's a rough initial API proposal:

// To be added:
// (Inspired by the current implementation of `createAction`)
function createAsyncAction(type, thunk) {
  const action = payload => thunk(payload);
  action.toString = () => type;
  return action;
}

// Usage example:
const someAsyncAction = createAsyncAction("SOME_ASYNC_ACTION", payload => {
  return dispatch => {
    return setTimeout(() => {
      // One second later...
      dispatch(someAsyncActionSuccess());
    }, 1000);
  };
});

const someAsyncActionSuccess = createAction("SOME_ASYNC_ACTION_SUCCESS");

The only new proposal is at the top of that snippet, the function createAsyncAction. It takes two parameters: type (same as createAction) and thunk.

What are everyone's thoughts?

@denisw
Copy link
Contributor

denisw commented Jan 11, 2019

A helper function for thunk actions sounds like a great idea to me!

I have one thought about the design. In the case of async actions without payload, I could see myself forgetting about specifying the outer function (the one returning the thunk) and directly writing the thunk instead.

// FAIL: I pass a thunk instead of a function returning a thunk
const someAsyncAction = createAsyncAction('SOME_ASYNC_ACTION', dispatch => {
  // ...
})

// I need to remember to do this instead
const someAsyncAction = createAsyncAction('SOME_ASYNC_ACTION', () => dispatch => {
  // ...
})

Not sure how much of a problem it is, especially if you are using TypeScript. But maybe there is a tweak that could be done to mitigate this issue; perhaps throwing a helpful error if no thunk is returned from the passed function. Something along the lines of:

function createAsyncAction(type, thunkCreator) {
  const action = payload => {
    const thunk = thunkCreator(payload);

    if (!(thunk instanceof Function)) {
      throw new TypeError(
        `The function you passed to createAsyncAction('${type}') did not `
        'return a thunk. Did you maybe pass a thunk directly instead of a '
        'function returning a thunk?')
    }

    return thunk;
  };

  action.toString = () => type;
  return action;
}

const someAsyncAction = createAsyncAction('SOME_ASYNC_ACTION', dispatch => {
  // ...
})

someAsyncAction() // TypeError

@markerikson
Copy link
Collaborator

I'm not sure what the point of this specific proposal is. @jamescmartinez , can you clarify?

@denisw
Copy link
Contributor

denisw commented Jan 11, 2019

Oops, I think I had the same confusion as @jamescmartinez when commenting. I had assumed the value-add is to bundle action creator and type together into one object like createAction does. But a thunk action creator doesn't have or need and action type! It just returns a thunk, and that never reaches the reducer (only its dispatched actions do, and those can of course be defined with createAction).

@markerikson
Copy link
Collaborator

Right, exactly.

The phrase "async action" is confusingly used to refer to two different things:

  • Dispatching a thunk function, which allows you to write async logic while having access to dispatch and getState
  • Defining multiple action types that relate to the lifecycle of AJAX requests, like REQUEST_STARTED, REQUEST_SUCCEEDED, REQUEST_FAILED

@denisw
Copy link
Contributor

denisw commented Jan 11, 2019

Yeah. Here is what can already do with the current redux-starter-kit:

const fetchTodosStarted = createAction("todos/fetch/started")
const fetchTodosSucceeded = createAction("todos/fetch/succeeded")
const fetchTodosFailed= createAction("todos/fetch/failed")

const fetchTodos = () => dispatch => {
  return TodosAPI.fetchAll().then(
    todos => dispatch(fetchTodosSucceeded(todos)),
    error => dispatch(fetchTodosFailed(error))
  )
}

Indeed, createAsyncAction() as described in the original issue description wouldn't add anything on top of this. We could alternatively think about making the "thunk + set of async lifecycle actions" pattern more convenient, though. Perhaps something like:

// Example

const fetchTodo = createAsyncAction('todos/fetch', () => {
  return TodosAPI.fetchAll()
})

fetchTodo.started  // equivalent to createAction('todos/fetch:started')
fetchTodos.succeeded  // equivalent to createAction('todos/fetch: succeeded')
fetchTodos.failed  // equivalent to createAction('todos/fetch:failed')
  
// Dispatches fetchTodos.started() immediately, and then
// fetchTodos.succeeded(todos) or fetchTodos.failed(error)
// depending on the promise result.
fetchTodos()
  .then(todos => console.log(`fetchTodos.started was dispatch with: ${todos}`)
  .catch(error => console.log(`fetchTodos.failed was dispatch with: ${error}`)

// Usage with createReducer()

const initialState = {
  todos: null,
  loading: false,
  loadingError: false
}

const reducer = createReducer(initialState, {
  [fetchTodos.started]: state => { 
    state.loading = true
    state.loadingError = false
  },
  [fetchTodos.succeeded]: (state, { payload: todos }) => { 
    state.loading = false
    state.todos = todos
  },
  [fetchTodos.failed]: (state, { payload: error }) => { 
    state.loading = false
    state.loadingError = true
  }
})

// Implementation

function createAsyncAction(type, asyncFn) {
  const started = createAction(`${type}:started`)
  const succeeded = createAction(`${type}:succeeded`)
  const failed = createAction(`${type}:failed`)

  const run = payload => {
    dispatch(started(payload))
    return promiseFn(promise).then(
      result => {
        dispatch(succeeded(result))
        return result
      },
      error => {
        dispatch(failed(error))
        throw error
      }
  )
}

@markerikson
Copy link
Collaborator

markerikson commented Jan 11, 2019

Yeah, there's about a bazillion libraries out there already for generating request action triplets. Question is, is that a pattern that's worth building in to RSK, and if so, how much abstraction should be used?

I actually asked about some of this on Twitter yesterday: https://twitter.com/acemarke/status/1083431099844444161

This also gets off into more complicated questions about how to structure entity data vs "loading state" data (in the items, in the "item type" slice next to the items, or in a totally separate slice for all loading state).

@denisw
Copy link
Contributor

denisw commented Jan 11, 2019

True, there are lots of libraries, but as we are already establishing a bunch of conventions through createAction and createReducer, a custom-made version could be made simpler by being opinionated towards these conventions.

I just answered to the Twitter poll. In pretty much every React-Redux project I have participated in within the last three years (about 5), each of them had separate actions of the _STARTED / _SUCCEEDED / _FAILED variety, made a lot of use of _STARTED for loading indicators, and often used _FAILED for showing error messages (at the very least, the developers used them for debugging in devtools).

I don't quite understand your point on entity data vs loading data. Isn't this orthogonal to the very humble goals of a createAsyncAction as specified above?

@markerikson
Copy link
Collaborator

Right, that's my point. This easily veers off into much more complicated territory.

@denisw
Copy link
Contributor

denisw commented Jan 11, 2019

Right, that's my point. This easily veers off into much more complicated territory.

It can, but doesn't have to. By choosing a very narrow scope (codifying the common started/succeeded/failed pattern on top of redux-thunk), I feel it would be possible to offer a useful helper that at the same time leaves full flexibility in all other respects. It's not closing doors - you can still write thunk action creators manually for the cases not covered.

That being said, I understand that redux-starter-kit needs to be very careful about what to include as it will be considered the "official" toolbelt for Redux. I'm also not sure whether such a createAsyncAction should make the cut based on that.

@markerikson
Copy link
Collaborator

Well, I'm open to suggestions on this topic, and I see the potential value, but I"m also not immediately sure how important it would be to have this included.

@markerikson
Copy link
Collaborator

An example that looks like it's very related to this discussion: https://medium.com/@ankeet.maini/type-redux-with-typescript-without-writing-any-types-5f96cbfef806

@markerikson
Copy link
Collaborator

redux-thunk-actions looks like it's basically what I want in terms of generating a thunk that handles an API request.

@drd
Copy link

drd commented Feb 17, 2019

Yeah. Here is what can already do with the current redux-starter-kit:

const fetchTodosStarted = createAction("todos/fetch/started")
const fetchTodosSucceeded = createAction("todos/fetch/succeeded")
const fetchTodosFailed= createAction("todos/fetch/failed")

const fetchTodos = () => dispatch => {
  return TodosAPI.fetchAll().then(
    todos => dispatch(fetchTodosSucceeded(todos)),
    error => dispatch(fetchTodosFailed(error))
  )
}

My question is how can I integrate this fetchTodos action into the nice structure of redux-starter-kit … in other words, where do I put this, and how do I call it from my component? I can't seem to put normal thunk actions into createReducer, because the function is put into the state by immer. There are no docs that I can find that clarify this seemingly common (and, due to the inclusion of redux-thunk, within the scope of the library) use case. If I'm missing something obvious, please do enlighten me! :)

@markerikson
Copy link
Collaborator

markerikson commented Feb 17, 2019

@drd : you wouldn't use a thunk action creator as a key in createReducer, because A) it doesn't correspond to a single specific action type string, and B) it also doesn't have its toString() overridden anyway.

You would use it as part of your call to connect(), like const mapDispatch = {fetchTodos} and call it as this.props.fetchTodos(), same as always. Nothing about that has changed.

Where you define that function is also entirely up to you.

@drd
Copy link

drd commented Feb 18, 2019

I figured that out, but only after a frustratingly long debugging session… It seems to me that either redux-starter-kit should have an officially-blessed way to integrate async actions with createReducer / createSlice (e.g., an asyncActions: key) or at least have a clear explanation in the docs that even though this library includes redux-thunk you can't use it with createReducer or createSlice. I suppose that's the thing that ultimately led me astray — I conflated the inclusion of redux-thunk with direct support by these higher-level functions ... I'm happy to contribute code and/or docs to clarify this, but I do think at the moment it's a bit murky!

@markerikson
Copy link
Collaborator

@drd : that's sort of the point. Side effect approaches have never had a direct correlation to a reducer, because a thunk function or an observable or a saga never directly interacts with a reducer - they just dispatch "normal" actions that a reducer can actually handle.

I'm sorry to hear you were confused, but there's nothing special about RSK in that regard. All that RSK does is add applyMiddleware(thunk) by default so you don't have to do that yourself.

@tim-hm
Copy link

tim-hm commented Feb 20, 2019

I've stumbled upon this thread after going through a similar learning/discovery process to @drd. I would love for async actions to be closely linked with slices. I appreciate that technically side effects are separate from reducers, but in practice they are closely linked. One library which i feel dealt with this nicely was @rematch where async actions are available on the model. IMO, a slicker dev experience with RSK would follow their example and look like this:

const aSlice = createSlice({
  initialState: { ... },
  reducers: {
     update: (state, action) => action
  },
  slice: { ... },
  effects: (dispatch) => {
     someAsyncAction: async (someValue) => {
        const result = await fetch(...)
        dispatch.aSlice.update(result)
     }
  }
})

@devlead125
Copy link

devlead125 commented Mar 26, 2019

How about this approach?

const {
  actions: { getByIdReq, getByIdSuccess, getByIdFail }
} = createSlice({
  reducers: {
    getByIdReq(state, action) {},
    getByIdSuccess(state, action) {},
    getByIdFail(state, action) {}
  }
});

export const getById = createAsyncAction(
  [getByIdReq, getByIdSuccess, getByIdFail],
  (payload, rootState) => Api.getById(payload) // You can also access rootState when you need
);

I'm not experienced developer so it maybe wrong implementation but I'd love to introduce implementation idea based on Redux Thunk.

const createAsyncAction = ([req, success, fail], promise) => payload => (
  dispatch,
  getState
) => {
  dispatch(req());
  promise(payload, getState())
    .then(data => dispatch(success(data)))
    .catch(error => dispatch(fail(error)));
};

@SergeyVolynkin
Copy link

SergeyVolynkin commented May 20, 2019

@tim-hm's approach is awesome, let's implement that.

Todos example with this approach:

export const todos = createSlice({
  slice: "todos"
  initialState: { ... },
  reducers: {
     set: (state, action) => { ... }
  },
  effects: ({dispatch, getState, actions}) => {
     fetch: async () => {
        const result = await fetch(...)
        dispatch(actions.set(result.todos))
     }
  }
})

P.S.
actions arg in effects are actions that been defined in the current slice

cc @markerikson

@renatoruk
Copy link

renatoruk commented May 29, 2019

@tim-hm's approach is awesome, let's implement that.

Todos example with this approach:

export const todos = createSlice({
  slice: "todos"
  initialState: { ... },
  reducers: {
     set: (state, action) => { ... }
  },
  effects: ({dispatch, getState, actions}) => {
     fetch: async () => {
        const result = await fetch(...)
        dispatch(actions.set(result.todos))
     }
  }
})

P.S.
actions arg in effects are actions that been defined in the current slice

cc @markerikson

I see how this methodology would benefit even the most trivial projects. Starting with bare redux you very quickly end up having too much boilerplate, especially with typescript. Ducks pattern just delegates that problem to different files, but cohesiveness is almost nonexistent. This way you could directly map your view logic with the model in a reasonable and predictive way. If implemented (fingers crossed), this example should be the first page in redux documentation, in my honest opinion. :) It's just that good.

@Yakimych
Copy link

Nice! We already put all thunks into an "effects" folder in our projects, and it really helps with an overview of what parts of code are effectful.

One thing that could be tricky with this approach, however, is that thunks don't necessarily belong to a chunk/slice of state (therefore the getState parameter that returns the whole state object). But I suppose the idea is that those can stay "cross-cutting" just as before, while effects that belong to a slice of state can be grouped along with actions and reducers?

@SergeyVolynkin
Copy link

Nice! We already put all thunks into an "effects" folder in our projects, and it really helps with an overview of what parts of code are effectful.

One thing that could be tricky with this approach, however, is that thunks don't necessarily belong to a chunk/slice of state (therefore the getState parameter that returns the whole state object). But I suppose the idea is that those can stay "cross-cutting" just as before, while effects that belong to a slice of state can be grouped along with actions and reducers?

Yes, the proposal is correctly understood. It's an addition rather than an approach change.

@mustafahlvc
Copy link

I prefer export the slice object directly itself, so there is no need to import every action separately or extra definition in the slice file.

I m using as below as a workaround:

const todos = createSlice({
    slice       : "todos",
    initialState: {...},
    reducers    : {
        set: (state, action) => {
        }
    }
});

_.assign(todos, {
    effects: {
        fetch: () => {
            return (dispatch) => async () => {
                const result = await fetch();
                dispatch(todos.actions.set(result.todos))
            }
        }
    }
});

export default todos;

i could also create a custom createSlice function.

So it would be good to have effects in the slice object.

@quantumpotato
Copy link

Bump, this is critical.

@quantumpotato
Copy link

What is the "effects"? How do I use this?
@mustafahlvc

@markerikson
Copy link
Collaborator

@quantumpotato: please provide further details on what exactly you want to do, and what potential API would be useful for you.

@quantumpotato
Copy link

I'm wondering two things. One, what is the use of "effects".
Two, I want an example of making http requests through async actions. These asynchronous examples without network activity are contrived for learning the basics and not practical for real world applications.
I am currently looking at https://github.com/zhuoli99/radiotrax-code-challenge/blob/master/client/src/reducers/devicesSlice.js as an example and attempting to follow it. Any comments or suggestions on their structure is appreciated.

@markerikson
Copy link
Collaborator

@quantumpotato : please see these articles on working with side effects in Redux:

https://redux.js.org/introduction/learning-resources#side-effects-basics

Per the discussion in this thread, RSK adds the thunk middleware to your store by default in configureStore, but other than that, writing and using thunks is up to you.

The notion of "effects" in this spread specifically is about adding specific syntax for attaching / declaring thunk functions as part of createSlice().

I am currently working on the "Advanced" tutorial for RSK, which will show how to write and use thunks alongside slices. You can see the WIP preview at:

https://deploy-preview-179--redux-starter-kit-docs.netlify.com/tutorials/advanced-tutorial

@quantumpotato
Copy link

quantumpotato commented Sep 1, 2019

Cool, looking forward to covering the networking.
I currently am stuck on infinite loops when I call dispatch. The one solution I found was "your mapDispatch h should be an object not a function" and it is.
EDIT: debugging finished, my inner dispatch function was written incorrectly. If you are getting an infinite loop making async, make sure you are sending dispatch in as one variable to the fn returned by your initial call.

@machadogj
Copy link

Sure, but that is as opposed to something like:

export const fetchUser = asyncAction('fetchUser', userId => $.getJSON('....'+userId));

For example, not to get picky, but you forgot to pass the e variable to the fetchUserError action in your sample: dispatch(fetchUserError())
We all agree it's fine and not a big deal, but it's one less thing to look out for.

@PredokMiF
Copy link

Sure, but that is as opposed to something like:

export const fetchUser = asyncAction('fetchUser', userId => $.getJSON('....'+userId));

For example, not to get picky, but you forgot to pass the e variable to the fetchUserError action in your sample: dispatch(fetchUserError())
We all agree it's fine and not a big deal, but it's one less thing to look out for.

  1. I did not forget to pass e to the AC. I need to know that loading failed, but the details are not important to me
  2. The community needs to be very careful when extanding or changing the API. If there are many kind of solutions or you don’t know how to fix it - you need to do nothing. I think current API is pretty good and minimal now, no reason to extent it, at least by createAsyncAction

@markerikson
Copy link
Collaborator

markerikson commented Feb 10, 2020

Been thinking about this a bunch more the last few days.

On the one hand, I agree with @phryneas that this can easily spiral into complex questions about caching and normalization that I don't want us to deal with.

On the other hand, fetching data and caching it in Redux is something that most folks do, and as is evidenced by the plethora of libraries out there that try to abstract portions of it, it's both a common use case and a pain point in terms of how much code you have to write to do it "by hand".

Given that, I do think it's reasonable to include some minimal API in Redux Toolkit to simplify the most common basic use case. Per my vision statement for Redux Toolkit:

Simplify Common Use Cases and Remove Boilerplate

I want to remove as many potential pain points from the equation as possible:

  • Reduce the amount of actual physical characters that need to be typed wherever possible
  • Provide functions that do some of the most obvious things you'd normally do by hand
  • Look at how people are using Redux and want to use Redux, and offer "official" solutions for what they're doing already

People use the word "boilerplate" to refer to lots of different things, but mostly it's "writing more repeitive code by hand". The biggest examples are defining action types, writing action creators, and immutable update logic, and to some extent setting up the store. I want the user to be able to do those automatically if they choose to.

Handling async request lifecycles falls into this category as well. Most people follow the "action triplet" pattern laid out in the docs, and have thunks that fetch from an API and dispatch actions accordingly.

There's lots of potential APIs that we could draw inspiration from. Just off the top of my head, some of the variations out there are:

  • Abstract the process of defining actions and dispatching, at the thunk level
  • Automatically look for a promise and dispatch actions when it resolve
  • Handle all the real async fetching work in a centralized middleware and dispatch descriptive "go get this endpoint" actions

At the moment, I'd say that the "abstract thunk usage and request status" approach is the approach that I'd want to go for. Relatively minimal API to implement and explain, and definitely saves code.

Looking at the usage in @jonjaques 's gist, it basically matches the API I had in my head:

export const fetchStuff = createAsyncAction('fetchStuff', (id) => api.fetch(id))

// fetchStuff is a thunk action creator that accepts args and dispatch in sequence
// It also has these plain action creators attached as fields:

export default createReducer<IState>({loading: false}, builder => builder
  .addCase(fetchStuff.pending, (state, action) => {
  })
  .addCase(fetchStuff.fulfilled, (state, action) => {
  })
  .addCase(fetchStuff.rejected, (state, action) => {
  })
  .addCase(fetchStuff.finished, (state, action) => {
  })
)

We allow the user to entirely bring-your-own data fetching and response handling. All that matters is that the callback you provide returns a promise, and it handles generating the appropriate actions types and doing the action dispatch sequencing.

I also particularly like that it's in TS already and relies on RTK APIs and types.

For now, I think I'd want to still skip adding any special option syntax for defining these directly as part of createSlice. It's still something we could add down the road if desired, but I'd rather move more slowly, since any API that we add is something we have to maintain indefinitely (implementation, documentation, etc). Better to do things one step at a time.

Notionally, you'd call createAsyncAction() up front in a slice file, and then use these action creators in the extraReducers arg for createSlice(). That does mean manually defining the action prefix, but again I think that's an acceptable limitation for the moment.

@phryneas , thoughts on the types in that gist atm?

(And of course, we can bikeshed over the desired action type / function names...)

As a related note, over in #333 I'm playing around with the idea of adding some prebuilt types and logic to handle working with normalized state. Also still early experimental stage atm, but I think there may be a 50%-ish solution there as well that would be of benefit.

@jonjaques
Copy link

jonjaques commented Feb 10, 2020

My two cents in some practical usage of the above pattern (the types are crap btw, please halp!) is that occasionally I call an API where I need access to the arguments the action was called with in the fulfilled/error cases. My solution/hack was to add a meta object in the payload itself, mostly cause I couldn't figure out the types.

action.payload = { result, args }

It would be really nice if any implementation actually attached these on the meta object instead.

@markerikson
Copy link
Collaborator

markerikson commented Feb 10, 2020

@andrewtpoe pointed me to a similar implementation that he'd put together:

I particularly like the name createRequestAction better than either createAsyncAction (as originally suggested in this thread) or createAsyncThunk (which I'd been considering as an option).

@jonjaques : we've already got a pattern for having "prepare callbacks" that return {payload?, meta?, error?}, so perhaps that could be applied here too.

@markerikson
Copy link
Collaborator

markerikson commented Feb 10, 2020

Since there's overlap between the "fetch stuff" and "store stuff" use cases, here's what a notional example usage might look like if we had both createRequestAction from this thread, and the EntityAdapter logic for normalization from #333 :

const usersAdapter = createEntityAdapter<User>();

const fetchUsers = createRequestAction(
    "users/fetch",
    () => usersAPI.fetchAll()
);

const usersSlice = createSlice({
    name: "users",
    initialState: usersAdapter.getInitialState(),
    reducers: {
        userAdded: usersAdapter.addOne,
        userRemoved: usersAdapter.removeOne,
        // etc
    },
    extraReducers: {
        // would also want to handle the loading state cases, probably with a state machine
        [fetchUsers.fulfilled]: usersAdapter.upsertMany
    }
});

@phryneas
Copy link
Member

phryneas commented Feb 10, 2020

My two cents in some practical usage of the above pattern (the types are crap btw, please halp!) is that occasionally I call an API where I need access to the arguments the action was called with in the fulfilled/error cases. My solution/hack was to add a meta object in the payload itself, mostly cause I couldn't figure out the types.

action.payload = { result, args }

It would be really nice if any implementation actually attached these on the meta object instead.

(I took offense to your "shit" comment here, but I might have misread your comment, so I'm sorry. Didn't mean to snap - too many meetings today, I'm kinda tired.)

There was a bug that prevented meta and error from bein typed correctly. I'm opening a PR (#350) to fix this, so you'll be able to use the prepare notation with the next release of RTK. If you find the time, I'd appreciate it if you gave the CodeSandbox build from that PR a try and give feedback in the issue if it works for you now.

You can install it with
yarn add https://pkg.csb.dev/reduxjs/redux-toolkit/commit/9023d835/@reduxjs/toolkit

For further reference on how to type error and meta, see our type tests.

@arye-eidelman
Copy link

#76 (comment)

I figured that out, but only after a frustratingly long debugging session… It seems to me that either redux-starter-kit should have an officially-blessed way to integrate async actions with createReducer / createSlice (e.g., an asyncActions: key) or at least have a clear explanation in the docs that even though this library includes redux-thunk you can't use it with createReducer or createSlice. I suppose that's the thing that ultimately led me astray — I conflated the inclusion of redux-thunk with direct support by these higher-level functions ... I'm happy to contribute code and/or docs to clarify this, but I do think at the moment it's a bit murky!

I've been adding my thunk actions to slice.actions for ease of use when importing and exporting.

const usersSlice = createSlice({
  name: "users",
  initialState: { byId: {} },
  reducers: {
    // ...
  },
})
userSlice.actions.loadUserProfile = payload => async (dispatch, state) => {
  // ...
}

@phryneas
Copy link
Member

phryneas commented Feb 10, 2020

So yeah, completely missed the point here, I was somehow buried in notification emails and only read the wrong ones.

As for the gist from @jonjaques: that looks pretty solid in itself. It would be great if we could get some way of cancellation in there and maybe something like configurable "auto-cancelling" on re-request.

For the manual cancelling, something along the lines of

export function createAsyncAction<
  ActionType extends string,
  PayloadCreator extends AsyncActionCreator<any, Dispatch, any, undefined>
>(type: ActionType, payloadCreator: PayloadCreator) {

+ const controller = new AbortController();

  function actionCreator(args?: ActionParams) {
- return async (dispatch: any, getState: any, extra: any) => {
+ async funcion thunk (dispatch: any, getState: any, extra: any) => {
      try {
        dispatch(pending({ args }))
        const result: Await<ReturnType<PayloadCreator>> = await payloadCreator(
          args,
          dispatch,
          getState,
          extra,
+ signal: controller.signal
        )
        return dispatch(fulfilled({ args, result }))
      } catch (err) {
        dispatch(rejected({ args, error: err }))
      } finally {
        dispatch(finished({ args }) 
      }
      }
+ thunk.abort = (dispatch) => { 
+  controller.abort()
+  dispatch(aborted)
+}
+ return thunk
  }

//...
}

That could be used like a normal thunk, but it would also expose a abort thunk on the thunk function itself to cancel stuff. Just as some braindump.

@markerikson
Copy link
Collaborator

Sure, seems pretty reasonable to me.

One other observation: Dan has pointed out a number of times that wrapping both an async call and the response usage in a catch is a bad idea, because both network errors and app code errors will get swallowed by the same catch. Dan pointed to the 2-arg form of .then(success,failure) as the right answer. Unfortunately, with async/await, it's tricker - you have to stick a variable outside the try block and handle success later:

let repoDetails
  try {
    repoDetails = await getRepoDetails(org, repo)
  } catch (err) {
    dispatch(getRepoDetailsFailed(err.toString()))
    return
  }
  dispatch(getRepoDetailsSuccess(repoDetails))
}

We'd want to handle that nicely somehow.

@PredokMiF
Copy link

If API will be extended with createAsyncAction, need to think about big problem of thunk actions: rejections. How to reject async action from the external code (1) or reject action by starts a new same action (2).
Example 1: Async action dispatched from selectbox to get list items but user move to another page and there no selectbox now. Of course, we can finish fetch, but in some case we can have issue.
Example 2: selectbox with search. For evere typed symbol we need to get result from server and prevent previous requests if they not finished. In my practice was cases when previos fetch take more time then last fetch and I see result of previos fetch (not last). Race between requests.

@phryneas
Copy link
Member

If API will be extended with createAsyncAction, need to think about big problem of thunk actions: rejections. How to reject async action from the external code (1) or reject action by starts a new same action (2).
Example 1: Async action dispatched from selectbox to get list items but user move to another page and there no selectbox now. Of course, we can finish fetch, but in some case we can have issue.
Example 2: selectbox with search. For evere typed symbol we need to get result from server and prevent previous requests if they not finished. In my practice was cases when previos fetch take more time then last fetch and I see result of previos fetch (not last). Race between requests.

See #76 (comment)

@markerikson
Copy link
Collaborator

A couple quick API design thoughts:

  • The issue title suggests a name of createAsyncAction. I rather liked createRequestAction as I said a few comments ago, but it's been pointed out that folks might be doing non-request-y things as well. So, I'm now leaning towards createAsyncThunk instead.
  • Since the number of parameters to the user's promise callback keeps growing, perhaps we should pass them in a single options object instead of positional arguments: ({args, dispatch, getState, extra, abort})

@PredokMiF
Copy link

Start/Done/Fail/Cancelled it is more then createAsyncAction... createLifecycle that return action to start this and maby second returned action to cancel them is better for our needs

@jaysoo
Copy link

jaysoo commented Feb 13, 2020

One additional feature that you might want to consider is handling correct ordering of async responses.

The @ngrx/effects library has an advantage here since RxJS is assumed, so correct ordering is left up to the user -- which is almost always to use concatMap operator.

For async thunks, you would usually want the fulfillment to be dispatched in the same order that the corresponding thunks were created -- this would be the same behavior as concatMap.

I put together an example repo to show the ordering issue here: https://github.com/jaysoo/async-thunk-example


Note: There could be other ways you want to handle multiple calls of the same thunk, so perhaps there could be an option to turn ordering off... or switch out different strategies. However if anything, other orders of operation are almost always wrong.

@markerikson
Copy link
Collaborator

@jaysoo : can you clarify the kind of scenario you're thinking of with regard to ordering
?

Looking at the code, I'm also not sure I understand what that promise chaining is really doing to result in this ordering.

@lacolaco
Copy link

As far as I understood, I feel users/fetch doesn't seem following the redux style guide.
https://redux.js.org/style-guide/style-guide/#model-actions-as-events-not-setters

Is fetch an event? Should I choose users/fetchRequested rather?
I'd like to know the thought about the recommendation of async actions.

@markerikson
Copy link
Collaborator

Yeah, per my comment in #352 :

I'm not worried about style guide stuff atm. I'm just trying to get the API working the way I want it to, and throwing together small snippets to try to exercise the code. Don't read anything into the action type names in the tests and examples, they're not relevant or an official guidance.

@jaysoo
Copy link

jaysoo commented Feb 13, 2020

can you clarify the kind of scenario you're thinking of with regard to ordering

Sure. When the app calls the incrementCounter thunk, the response can be returned out of order (depending on latency and processing time on server).

Thus you may have the following fulfilled actions dispatched.

{ type: 'increment/fulfilled', payload: { result: 1 } }
{ type: 'increment/fulfilled', payload: { result: 2 } }
{ type: 'increment/fulfilled', payload: { result: 3 } }
{ type: 'increment/fulfilled', payload: { result: 5 } }
{ type: 'increment/fulfilled', payload: { result: 4 } }

Each request is supposed to increment the counter on API-side by one so you'd expect it to return in order. However due to async nature of the requests, in this scenario you end up with the wrong state of 4 instead of 5.

What you actually want is for each action to be dispatched in the order that the corresponding thunk was called: 1, 2, 3, 4, 5.


As an aside, with @ngrx/effects you'd normally do something like this:

class CounterEffects {
  increment$ = createEffect(() => this.actions$.pipe(
    ofType('[Counter] Increment Counter'),
    concatMap(() => incrementApi()),
    map(value => ({ type: '[Counter] Counter Incremented FulFilled', payload: value })
  );

  // ...
}

The concatMap operator guarantees that the [Counter] Counter Incremented FulFilled actions are dispatched in the same order of their corresponding [Counter] Increment Counter actions. You can also apply other flattening operators like mergeMap instead, which can result in ordering issues.

@puppybits
Copy link

puppybits commented Feb 13, 2020

@phryneas @markerikson @machadogj @SergeyVolynkin Great discussion and input from everyone! Async is really hard, which is why it was good for Redux to punt on it for a while but its critical for real-world apps.

It sounds like Thunk Action Creators and Extra Reducers go against some of the core principles of Redux. There might be a way to take features of both while maintaining Redux's core goals and supporting async.

Redux goals (as I understand them):

  • Idempotent, synchronous mutations (via reducers)
  • unidirectional data flow
  • record requests for mutation (reducers don't just dump payload on state, the payload are the params to properly mutate state)
  • mutation requests can be serialized and replayed (for a predictable state control)
  • reducers aren't concerned with data flow (but a log action history does and it's chronological data flow).

Where thunk action creators and thunk middleware struggle

  1. No explicit action Thunks are just functions. So there is no initial action message created that communicate the real intent for the mutation.
  2. Data flow actions Instead thunks create multiple actions inside to start processing the data, without dispatching an action to say why. These actions aren't the original intent (like persist x data model) but multiple stage of an implicit request (like loading, sending, error and success).
  3. Sequential actions are disbursed throughout the action history Async flows are easy to grok when they're are all together. But thunks break data flows up into multiple functions that jump around a store and other actions are being fired in the meantime making action logs hard to grok.

Concerns with Extra Reducers

  1. Async != Idempotent In the real-world async work is almost never idempotent. Async's core use case is getting data that lives outside of RAM. Because of that external state you have zero guarantee that given the same arguments the function will always produce the same result.

Concerns with Effects

  1. Table flipping Giving access to everything (dispatch, state and actions) means a developer can do anything, good and bad. There's no direction, it's basically a table flip because the library doesn't provide direction. Without direction developers will do whatever is easy, not what's simple and repeatable.
  2. Long running, rouge mutations If Effects have state and do async they will be running over multiple JS run loops. Those long running functions can easily be mutating state without a corresponding action logged. The UI might have moved on and this async work is no longer needed. The result is that Redux would no longer have consistent mutations.

Prior art to consider

There are others systems and languages that have been thinking about and managing async problems longer and at a bigger scale than Redux. I find it helpful to see how they've solved issues to get insight for Redux async. There's a few patterns that would be good to research.

  • Go Lang has praised for fantastic async data flow. Using CSP channels and patterns built on top of it can offer great insight.
  • Distributed systems. They have the same core concerns as Redux but at a much larger scale. Specifically the CQRS pattern has good insight with using action messages.
  • Clojure. They were the first community to really promote React. Clojure devs invented React Hooks and were using it in prod 3 years before React Hooks came to React. Om Next is a great example of Redux-like state management but with a solution for async built in.
  • others... I'd love to know if there's others to consider.

A possible solve without breaking Redux core goals

  1. Specific actions UI should create an action so we can log the actual intent. Instead of calling a thunk directly, start by dispatching an action for what the UI actually wants done. (ie call dispatch({type: "load_data"}) instead of loadData() which dispatches a loading, then other actions which the reader of the action log doesn't know why/what it's loading)
  2. Let reducers defer Reducers are the best place to decide if the state is in the right place to accept an action payload. If the state can not be mutated properly, then it should defer and request a side effect to fix the state.
  3. Async without mutation The function that handles async should get dispatch and the action that the reducer has deferred, but no state access (see above for rational).
  4. Prefer simple over easy Simple is doing just one thing. Easy is just what's close at hand. Let the async just do one thing while working INSIDE the reducer framework.
  5. Embrace the real-world Multi-stage data flows with a data request that then requires user input to move to the next stage is common. Ideally a simple solution doesn't preclude ignoring complex data flows in the real-world.

Example of async while supporting Redux's core goals

export const todos = createSlice({
  slice: "todos"
  initialState: {todos:[]},
  reducers: {
     insert: (state, {payload:{indexes, todos}}) => {
         if (indexes && !indexes.every(idx => idx > 0 && idx < state.length-1)) {
             // if the state doesn't support the action's request, 
             // then defer to an async effect
             return "fetch" 
         }
         todos.forEach(i => {
             let insertAt = indexes?[i] || i
             state.todos[insertAt] = todos[i]
         })
     }
  },
  effects: {
     fetch: async (dispatch, action) => {
        const {todos} = await fetchTodos()
        // the result of the effect is to dispatch an action with a proper payload
        dispatch(actions.insert({todos}))
        // this dispatch is sync, a reducer will now accept the request for mutation
        // and the UI will be redrawn with the new data
     }
  }
})

The return "fetch" is to trigger the effect. It would be shorthand for sending return {...action, {effect:"fetch"}. Deferring this lets the reducer forward, append or create the action payload to another place that handles async. Only the return of a reducer can trigger an async effect, it's not directly exposed for UI to trigger. This does expand the normal action message from {id, type, payload} to {id, type, effect, payload} because a idempotent transform requests should be different than a async where anything can happen and there's no guarantees on the result. We can also contain multi-stage data flows that include the server data and user input into a single effect the developer can break it into across multiple effects. Reducer get closer to the original purpose which is data mutation and less about transient states of async while still being able to log and update UI.

This solution combines the effects option and thunks while keeping the core control inside idempotent reducers. The Action history shows a clear intent and would be fully reproducible/replayable while still supporting the core goals of Redux.

This simple approach also supports complex flows that communicate with a server and getting feedback from the user and can support error handling with fallback or errors that will only be handled once.

@machadogj
Copy link

My recommendation is to keep it simple. Aim to provide a good solution to the 80% of the cases with minimum API surface and magic.

Ordering: I never had this problem myself. Even if someone did, I believe that it could somehow be dealt with using metadata (something like an auto-incremental or timestamp field) that is dispatched in every action and a fairly simple reducer. Even if it was not possible (or suitable) to do with async thunk creators, I think it falls outside of the 80% of the cases.

return "fetch": I've never seen this, and would be super surprised if I did in some project. I do agree that redux feels clumsy for complex flows, but again, not sure this is the problem that it's being addressed in this issue.

@markerikson
Copy link
Collaborator

markerikson commented Feb 13, 2020

@puppybits :

Appreciate the detailed comment. However, it seems like the points you're making are going in a different direction than the goals for this issue, and I'm not sure I agree with some of the concerns you're describing.

We currently recommend that Redux users should use thunks as the default method for working with async logic, and dispatch multiple actions to represent the lifecycle of an async call. All I'm trying to accomplish at this point is providing a built-in utility that minimizes how much code users have to write to accomplish that.

If you'd like to discuss alternate approaches for working with async logic, please comment over in issue #349: Discussion: declarative side effects and data fetching approaches.

edit

As an additional note, I responded to some similar concerns about thunk usage a couple years ago, in my post Thoughts on Thunks, Sagas, Abstraction, and Reusability.

@markerikson markerikson added this to the 1.3 milestone Feb 15, 2020
@markerikson
Copy link
Collaborator

I've merged #352 into a new integration branch, and there's a tracking PR for that in #374 as we plan to do additional work for v1.3.0.

I've put up a v1.3.0 roadmap issue at #373 .

@markerikson
Copy link
Collaborator

Just released https://github.com/reduxjs/redux-toolkit/releases/tag/v1.3.0-beta.0 , which has a (hopefully) stable and documented version of createAsyncThunk.

@markerikson
Copy link
Collaborator

v1.3.0 with createAsyncThunk is now LIVE! https://github.com/reduxjs/redux-toolkit/releases/tag/v1.3.0

That resolves this issue.

markerikson pushed a commit that referenced this issue Apr 20, 2021
* update createApi docs
* Add danger tag to internalActions

Co-authored-by: Matt Sutkowski <msutkowski@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests