reduxjs / redux-toolkit

The official, opinionated, batteries-included toolset for efficient Redux development
https://redux-toolkit.js.org
MIT License
10.69k stars 1.16k forks source link

Create Async Action #76

Closed jamescmartinez closed 4 years ago

jamescmartinez commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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

denisw commented 5 years ago

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 commented 5 years ago

Right, exactly.

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

denisw commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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

denisw commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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

drd commented 5 years ago

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 commented 5 years ago

@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 commented 5 years ago

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 commented 5 years ago

@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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

@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 commented 5 years ago

@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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

Bump, this is critical.

quantumpotato commented 5 years ago

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

markerikson commented 5 years ago

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

quantumpotato commented 5 years ago

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 commented 5 years ago

@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 commented 5 years ago

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.

RichiCoder1 commented 4 years ago

I'm super late to this, but maybe a better idea than action actions might be an async slice?

Super duper rough draft silly idea:


interface AsyncSliceOptions<TData, TRequestPayload> {
    name: string;
    initialState: TData;
    request: (payload: TRequestPayload, dispatch: Dispatch) => Promise<any>;
    mapSuccess?: (payload: unknown, dispatch: Dispatch) => TData;
    mapError?: (payload: unknown, dispatch: Dispatch) => any;
}

export function createAsyncSlice<TData, TRequestPayload>(options: AsyncSliceOptions<TData, TRequestPayload>) {
    const slice = createSlice({
        name: options.name,
        initialState: {
            loading: false,
            data: options.initialState as TData | null,
            error: null as any | null,
        },
        reducers: {
            requestStart(state) {
                state.loading = true;
                state.error = null;
            },
            requestSuccess(state, { payload }) {
                state.error = null;
                state.loading = false;
                state.data = payload;
            },
            requestFailure(state, { payload }) {
                state.error = payload;
                state.loading = false;
                state.data = null;
            }
        }
    });

    const action = (payload: TRequestPayload) => async (dispatch: Dispatch) => {
        try {
            dispatch(slice.actions.requestStart());
            let result = await options.request(payload, dispatch);
            if (options.mapSuccess) {
                result = options.mapSuccess(result, dispatch);
            }
            dispatch(slice.actions.requestSuccess(result));
        } catch (e) {
            let error = e;
            if (options.mapError) {
                error = options.mapError(error, dispatch);
            }
            dispatch(slice.actions.requestFailure(error));
        }
    };

    return [slice, action];
}

It seems a common enough pattern and gives apollo-ish ergonomics. Just an off the wall idea though.

Glinkis commented 4 years ago

A minor infliction in this discussion. It would be awesome if the action names were all the same length, if only to keep the code aesthetically pleasing. The pattern I usually use myself, is something like request, success, failure, or loading, success, failure.

machadogj commented 4 years ago

Another one late to the discussion ✋... I was also surprised to not find a solution for working with async actions in this toolkit. My take on this is that providing something that standardizes the implementation of async actions is super beneficial. I've seen people reinvent the wheel over and over again leading to either bugs or most likely a TON of code duplication (like some of the examples above).

This toolkit already took the first important step which is "to be opinionated", now you can find an opinionated solution to this.

(Disclaimer, I am the maintainer of) redux-thunk-actions which is one of the many solutions to this problem out there and the code isn't that complex, but the benefit is HUGE:

let myFetch = createActionThunk('MY_FETCH', () => api.fetch());

Generate two of three possible actions:

MY_FETCH_STARTED MY_FETCH_SUCCEEDED MY_FETCH_FAILED MY_FETCH_ENDED

We've used this pattern in several apps and barely never had any problems, and even when we had, it was either fixed with a small backward compatible fix to the lib or by creating a good old thunk for a super specific case.

i-oliva commented 4 years ago

@machadogj could you show us an example of how it will integrate with the current createReducer?

phryneas commented 4 years ago

Honestly though, I'm personally not a big fan of using redux for data fetching (there, I said it).

I believe that in the wake of many hook libraries already doing a pretty good job at that (react-async etc.) and suspense which will most likely (even if fetching is still done somehow in redux) require different patterns, adding such a thing now will encourage people to buy into a pattern that is (probably) almost end-of-life.

And yes, I know this can be applied to other asynchronous operations, but this simplified start/finish/error model is very much optimized for data fetching. Many other async operations might benefit more from different actions, and if we give people this specific blueprint they will not think about the actions their asynchronous model might benefit from, but they will start to blindly force it on their use case.

machadogj commented 4 years ago

@pavilion I haven't tested it, but looking at the documentation I think that something like this:

const getOrders = createActionThunk('MY_FETCH', () => api.getOrders());

const counterReducer = createReducer(0, {
  [getOrders.SUCCEEDED]: (state, action) => ({...state, orders: action.payload}),
  [getOrders.STARTED]: ...,
  [getOrders.FAILED]: ...,
  [getOrders.ENDED]: ...,
})

A nice thing about having a standard way of dispatching async stuff (which can be done without any lib as well...) is that if you name your actions properly, much of the "loading", "error handling", etc... can be done with a single (more generic) reducer.

machadogj commented 4 years ago

@phryneas

Honestly though, I'm personally not a big fan of using redux for data fetching (there, I said it).

Taste aside, can you tell us why, what or when you don't like it?

I believe that in the wake of many hook libraries already doing a pretty good job at that (react-async etc.) and suspense which will most likely (even if fetching is still done somehow in redux) require different patterns, adding such a thing now will encourage people to buy into a pattern that is (probably) almost end-of-life.

Actually, I don't necessarily think it's a bad idea to go with another approach. But if that is really the case, then I believe the toolkit could at least provide guidance and recommendations (if not helpers...) on how to do this. My objection is that the current approach seems to be this:

export const fetchIssuesCount = (
  org: string,
  repo: string
): AppThunk => async dispatch => {
  try {
    const repoDetails = await getRepoDetails(org, repo)
    dispatch(getRepoDetailsSuccess(repoDetails))
  } catch (err) {
    dispatch(getRepoDetailsFailed(err.toString()))
  }
}

And yes, I know this can be applied to other asynchronous operations, but this simplified start/finish/error model is very much optimized for data fetching.

It's modeled after/optimized for promises, just like react-async.

Many other async operations might benefit more from different actions, and if we give people this specific blueprint they will not think about the actions their asynchronous model might benefit from, but they will start to blindly force it on their use case.

I am not sure about Many. Other types of operations that can't be modeled after promises will not be suitable for this, like EventEmitter events. But in my personal experience, they are not very common (YMMV).

phryneas commented 4 years ago

@phryneas

Honestly though, I'm personally not a big fan of using redux for data fetching (there, I said it).

Taste aside, can you tell us why, what or when you don't like it?

Okay, I'll try and elaborate on why I don't think that Redux is a good choice for data fetching.

Let's take a look at some different examples:

Non-Normalized state, no cache expiration

Api endpoint used by one component and their children

I guess this is the most common usage, what most tutorials are explaining and what most people are actually doing. People create one slice per api endpoint, which contains the usual started/done/failed action-triple, a thunk to bind it all together and have one component that triggers the thunk on mount. Probably multiple children of that one component are consuming the api endpoint slice. This is a lot of (I hate that word) boilerplate for something that could be achieved with a one-line useFetch call by react-async. In that case, composition and a little bit of prop-drilling is enough to pass all required state down to children.

Api endpoint used by multiple distinct component

As useFetch is tied to a component's local state, if multiple components try to use the same API, things might go out of sync. This is where redux might come in handy, but if one were to stick to react-async, I've done an experimental wrapper that introduces a shared cache. (This is against an old version of react-async, so it's more a proof-of-concept, but it's not much code, so easily to be recreated.) Also, there are other hook fetching libraries already support something like that (react-hooks)

Cache expiration

This is something to consider, especially when multiple query results are being held from one more generic reducer: the stored memory is getting bigger and bigger. As far as I'm concerned, I don't know of a "cookbook recipe" for redux here, essentially state has to track which api responses are currently being used by mounted components - and clear that cache after a while of non-subscription. This adds a second level of "keeping subscriptions" to redux. I'm not a big fan of that. On the other side, with some hook library this feels almost natural. See my experimental use-cached-fetch mentioned above. If you're not using the same API in different application parts, it's even more easy: put it in local state. Once the component unmounts, it's cleaned up by default.

Normalization

Optimally, all API responses should be stored normalized, to prevent overlapping APIs to create out-of sync state. Doing so requires a lot of work and I haven't seen many people actually do this - leading to inconsistencies.

Also, this is only really feasible with either

Also, when normalizing the part with cache expiration gets even more tricky, if you really want to track which cache part is used by how many mounted components.

So...

To sum this up: everything beyond the most simple use case is really complicated and so most people stick to that most simple use case. But that leads to a lot of (repetitive) code written, that just doesn't make sense to me seeing that there are libraries out there that are really specialized on this stuff and handle (especially the simple cases) with one line of code.

We could start to add some helpers to make those "simple" cases easier to handle. But we couldn't possibly start to help in the more complicated cases, as they are just too different.

So, in the end, everyone using redux is essentially bound to start abstracing after the second or third API they connect to and essentially write a new (more often than not suboptimal) fetching library on top of redux - which gets more in the way than it helps, given the alternatives.

I've come to the conclusion that I don't see API Cache state as something that I every again want to manually hold in a global state management solution. There are better ways to do that. Also, this leaves redux for stuff it really shines with: event-based business logic (which api cache state arguably is not).

quantumproducer commented 4 years ago

Yes. I'd like to see a full, real example project. a TODO list with asynchronous registering, signing in, handling tokens, CRUD TODO items. I've had to write + rewrite my own networking tools and then I get on projects and see others have written similar things, always with unique flavors. I would appreciate a standard, easy to work with tool.

jonjaques commented 4 years ago

I've implemented this in my own app, just gonna throw this in here in case anyone wants it: https://gist.github.com/jonjaques/ccca52fd73f24bd90f7ac3d1d53a6e26

There are most certainly type bugs ^ and probably something stupid because I just pulled it out of my app without executing the sample code. But it functions in the most basic sense in that TS will correctly infer argument and return types.

Note: it does not require a promise middleware, it just relies on Thunk; Dunno if that's a good idea or not, but works for me. I'm also thinking that it would be better if the args were defined under a meta object, but that adds more complexity to the types.

machadogj commented 4 years ago

Hi Jon, looks pretty good. Is very similar to the one I showed. A small suggestion that proved useful in my case, is to add a “finished” action. We used this in reducers to track a “pending” status and stuff.

On 18 Jan 2020, at 22:26, Jon Jaques notifications@github.com wrote:

 I've implemented this in my own app, just gonna throw this in here in case anyone wants it: https://gist.github.com/jonjaques/ccca52fd73f24bd90f7ac3d1d53a6e26

There are most certainly type bugs ^ and probably some stupid because I just pulled it out of my app without executing the sample code.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or unsubscribe.

PredokMiF commented 4 years ago

I think no reason to extend API of toolkit. Thunk perfectly work out of the box!

const userSlice = createSlice({
    name: ...,
    initialState: ...,
    reducers: {
        // Some simple (sync) actions
        fetchUserStart: (state) => { ... },
        fetchUserFinish: (state, { payload: user }) => { .... },
        fetchUserError: (state) => { .... },
    }
})
const { fetchUserStart, fetchUserFinish, fetchUserError} = userSlice.actions

// Thunk async action
export const fetchUser = (userId) => async (dispatch, getState) => {
    try {
        dispatch(fetchUserStart())
        const user = await $.getJSON('/api/user/' + userId)
        dispatch(fetchUserFinish(user))
    } catch (e) {
        alert('Error: ' + e)
        dispatch(fetchUserError())
    }
}
machadogj commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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:

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 commented 4 years ago

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 commented 4 years ago

@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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

https://github.com/reduxjs/redux-toolkit/issues/76#issuecomment-464609140

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 commented 4 years ago

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.