-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Comments
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 |
I'm not sure what the point of this specific proposal is. @jamescmartinez , can you clarify? |
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 |
Right, exactly. The phrase "async action" is confusingly used to refer to two different things:
|
Yeah. Here is what can already do with the current 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, // 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
}
)
} |
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). |
True, there are lots of libraries, but as we are already establishing a bunch of conventions through 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 I don't quite understand your point on entity data vs loading data. Isn't this orthogonal to the very humble goals of a |
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 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 |
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. |
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 |
|
My question is how can I integrate this |
@drd : you wouldn't use a thunk action creator as a key in You would use it as part of your call to Where you define that function is also entirely up to you. |
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 |
@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 |
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:
|
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)));
}; |
@tim-hm's approach is awesome, let's implement that. Todos example with this approach:
P.S. 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. |
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 |
Yes, the proposal is correctly understood. It's an addition rather than an approach change. |
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. |
Bump, this is critical. |
What is the "effects"? How do I use this? |
@quantumpotato: please provide further details on what exactly you want to do, and what potential API would be useful for you. |
I'm wondering two things. One, what is the use of "effects". |
@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 The notion of "effects" in this spread specifically is about adding specific syntax for attaching / declaring thunk functions as part of 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 |
Cool, looking forward to covering the networking. |
Sure, but that is as opposed to something like:
For example, not to get picky, but you forgot to pass the |
|
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:
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 Notionally, you'd call @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. |
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.
It would be really nice if any implementation actually attached these on the |
@andrewtpoe pointed me to a similar implementation that he'd put together:
I particularly like the name @jonjaques : we've already got a pattern for having "prepare callbacks" that return |
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 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
}
}); |
(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 You can install it with For further reference on how to type |
I've been adding my thunk actions to const usersSlice = createSlice({
name: "users",
initialState: { byId: {} },
reducers: {
// ...
},
})
userSlice.actions.loadUserProfile = payload => async (dispatch, state) => {
// ...
} |
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 |
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 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. |
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). |
See #76 (comment) |
A couple quick API design thoughts:
|
Start/Done/Fail/Cancelled it is more then |
One additional feature that you might want to consider is handling correct ordering of async responses. The 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 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. |
@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. |
As far as I understood, I feel Is |
Yeah, per my comment in #352 :
|
Sure. When the app calls the Thus you may have the following fulfilled actions dispatched.
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 What you actually want is for each action to be dispatched in the order that the corresponding thunk was called: As an aside, with
The |
@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):
Where thunk action creators and thunk middleware struggle
Concerns with Extra Reducers
Concerns with Effects
Prior art to considerThere 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.
A possible solve without breaking Redux core goals
Example of async while supporting Redux's core goals
The return "fetch" is to trigger the effect. It would be shorthand for sending 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. |
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.
|
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. |
Just released https://github.com/reduxjs/redux-toolkit/releases/tag/v1.3.0-beta.0 , which has a (hopefully) stable and documented version of |
v1.3.0 with That resolves this issue. |
* update createApi docs * Add danger tag to internalActions Co-authored-by: Matt Sutkowski <[email protected]>
redux-starter-kit
includesredux-thunk
but doesn't include an async version ofcreateAction
. I want to float the idea of acreateAsyncAction
function that uses thunks; I can create a pull request if all looks good.Here's a rough initial API proposal:
The only new proposal is at the top of that snippet, the function
createAsyncAction
. It takes two parameters: type (same ascreateAction
) andthunk
.What are everyone's thoughts?
The text was updated successfully, but these errors were encountered: