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

Add getStatus to createAsyncThunk to return promise state #418

Closed
wants to merge 1 commit into from
Closed

Add getStatus to createAsyncThunk to return promise state #418

wants to merge 1 commit into from

Conversation

msutkowski
Copy link
Member

@codesandbox-ci
Copy link

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit ddcee04:

Sandbox Source
epic-hooks-oxwcm Configuration
fancy-bush-soxxp Configuration
infallible-bas-g5hnd Configuration

@phryneas
Copy link
Member

I wouldn't really want to make the promise that observable - it will keep moving logic from the reducers/middleware to the component level.

Also, I think this doesn't solve the problem the code example tried to solve.
This works fine if you keep a reference of the dispatched object - but usually, this will already require juggling of a useRef to only keep that reference within the same component.
But if two components want to both be dispatching the same asyncPromise that should only run once, they'd have to share state with each other. And this way, that would need to be shared outside of redux, which kind of defeats the purpose of redux.

@phryneas
Copy link
Member

Which also brings us to the point that we currently do not have a way to cancel a running asyncThunk that we do not have an object handle on - which is all we can do with only the thunkMiddleware right now. But it means that another component cannot abort a running asyncThunk that was started elsewhere.

If we wanted to have a way to cancel running asyncThunks, we could add a middleware that

  • is added before the thunkMiddleware
  • keeps a reference on every asyncThunk promise returned from the thunkMiddleware, together with the requestId
  • on a pending/rejected action, deletes that promise reference
  • on a new abort action, it executes the abort() method on the promise for a certain requestId

What do you two think @markerikson @msutkowski ?

@msutkowski
Copy link
Member Author

msutkowski commented Mar 10, 2020

That's actually where I was going to go with this idea. I see value doing this outside of redux, but like I said in that issue a user can create a simple wrapper and achieve the same thing.

@msutkowski
Copy link
Member Author

msutkowski commented Mar 10, 2020

@phryneas Thought about a few things in regards to this.

  1. Do we need to be concerned with a user making thunks that have the same type (most likely in error)? (ex: const fetchUserData = createAsyncThunk('fetchData', ()=>{}) and const fetchBookData = createAsyncThunk('fetchData', ()=>{})
  2. Do we need to provide any helpers that a user could use in a component if they wanted to check the status (ex: if (hasPending(myThunkFunction)) { or if (pendingCount(myThunkFunction)) > 2 {. Similarly, do we bake something like this into the thunkAPI? Note: i don't see any real use for a count thing, but is just an example
  3. Is this opt-in behavior, or the default?
  4. What would the usage look like if say you had a useEffect(() => { dispatch(fetchLongRunningQuery(params)), [params]}) and you always wanted to abort the pending request and initiate a new one?
    • Would you do this in the component or in the payload creator? Do we recommend one over the other?

Edit:
Just wanted to pull this in from the other issue for further context:

Also, I don't know if the payloadCreator itself should generally check that it should be running. It might make sense in this case, but that is a very edgy case and I wouldn't really use that as an example then. Generally: It has been started and not been cancelled, so it runs. I would assume that it's only the job of the reducer to ignore (or not) actions sent from the thunk.

I think regarding that idea, I might disagree slightly and lean more towards what is seen here: https://redux.js.org/advanced/async-actions/#actionsjs-with-fetch with shouldFetchPosts(). It seems like the payload creator should have some control here.

@phryneas
Copy link
Member

@phryneas Thought about a few things in regards to this.

1. Do we need to be concerned with a user making thunks that have the same type (most likely in error)? (ex: `const fetchUserData = createAsyncThunk('fetchData', ()=>{})` and `const fetchBookData = createAsyncThunk('fetchData', ()=>{})`

I don't really think we need to worry about this. All other actions would be duplicates as well. That's not an error you don't notice for a long time.

2. Do we need to provide any helpers that a user _could_ use in a component if they wanted to check the status (ex: `if (hasPending(myThunkFunction)) {` or `if (pendingCount(myThunkFunction)) > 2 {`. Similarly, do we bake something like this into the thunkAPI? _Note: i don't see any real use for a count thing, but is just an example_

If the user wanted to access that information, they could always store it in the state. I wouldn't allow for too much non-redux logic here. The store should be the source of all truth.

3. Is this opt-in behavior, or the default?

Such a middleware would need to be part of defaultMiddlewares, otherwise nobody would use it. It shouldn't be big.

4. What would the usage look like if say you had a `useEffect(() => { dispatch(fetchLongRunningQuery(params)), [params]})` and you always wanted to `abort` the pending request and initiate a new one?

Maybe something like

useEffect(() => { 
  dispatch(fetchLongRunningQuery.abortAll())
  dispatch(fetchLongRunningQuery(params))
}, [params])
   * Would you do this in the component or in the payload creator? Do we recommend one over the other?

Doing this from the payloadcreator might lead to a circular reference unless we'd add those abort-action-creators to the thunkApi object. We might consider doing that to enable both usages, though.

Edit:
Just wanted to pull this in from the other issue for further context:

Also, I don't know if the payloadCreator itself should generally check that it should be running. It might make sense in this case, but that is a very edgy case and I wouldn't really use that as an example then. Generally: It has been started and not been cancelled, so it runs. I would assume that it's only the job of the reducer to ignore (or not) actions sent from the thunk.

I think regarding that idea, I might disagree slightly and lean more towards what is seen here: https://redux.js.org/advanced/async-actions/#actionsjs-with-fetch with shouldFetchPosts(). It seems like the payload creator should have some control here.

Hmm. Of course, a payloadCreator can always do an early return/reject depending on the state, but at the time the payloadCreator is executed, the pending action has already been sent. If someone really wanted to prevent that, they'd have to wrap the asyncThunk in another thunk:

function wrapWithStateCondition(asyncThunk, conditionFn) {
  return args =>(dispatch, getState) => {
    if (conditionFn(getState()) {
      dispatch(asyncThunk(args));
    }
  }
}

// usage:
const wrappedAsyncThunk = wrapWithStateCondition(myAsyncThunk, state => state.users.state === 'idle');
dispatch(wrappedAsyncThunk(args));

And while we can add that as an example, going so far as supporting it per default would probably be too much for RTK, which aims for creating an easy API for the 80% use case.

@markerikson
Copy link
Collaborator

I'm feeling like this train of thought is getting out of hand.

Cancellation has always been a weak point for thunks. I really don't want to spend any more time trying to solve that kind of problem.

We've provided the AbortController mechanism, so that seems like it should be sufficient to let users do something if they really want to.

@msutkowski
Copy link
Member Author

@markerikson Agreed. The good thing is I don't think this is a blocking issue at all, because you could go with @phryneas 's idea or potentially using what I presented and call it a day. Is it worth documenting the ideas we've outlined as a starting point or no?

In general, I do like the concept of what we're discussing here. It'd be a good long term solution depending on createAsyncThunk adoption. Do you want to move it into a separate issue or just drop it for now? If so, I'll go ahead and close this.

@markerikson
Copy link
Collaborator

Yeah, let's close this, but try to add some more usage guide docs on cancelation or tracking requests.

@phryneas
Copy link
Member

I'm feeling like this train of thought is getting out of hand.

I admit I was having a similar thought. Maybe we should wait for the adoption and if people are actually running into these problems. No real use solving problems nobody's gonna have in the end.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants