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

feat(createAsyncThunk): async condition #1496

Merged
merged 6 commits into from
Oct 1, 2021

Conversation

thorn0
Copy link
Contributor

@thorn0 thorn0 commented Sep 10, 2021

closes #1494

@netlify
Copy link

netlify bot commented Sep 10, 2021

✔️ Deploy Preview for redux-starter-kit-docs ready!

🔨 Explore the source changes: f3228a1

🔍 Inspect the deploy log: https://app.netlify.com/sites/redux-starter-kit-docs/deploys/613b4f499f18e60008ea1b57

😎 Browse the preview: https://deploy-preview-1496--redux-starter-kit-docs.netlify.app

@codesandbox-ci
Copy link

codesandbox-ci bot commented Sep 10, 2021

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 f3228a1:

Sandbox Source
Vanilla Configuration
Vanilla Typescript Configuration
rsk-github-issues-example Configuration
@examples-query-react/basic Configuration
@examples-query-react/advanced Configuration

options.condition(arg, { getState, extra }) === false
) {
let conditionResult = options?.condition?.(arg, { getState, extra })
if (conditionResult instanceof Promise) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is instanceof good enough or do we want to support 3rd-party thenables?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also what should happen when an error is thrown in a synchronous condition?

Copy link
Member

@phryneas phryneas Sep 10, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm. "then" in conditionResult might be better, you're right.

As for an error in condition, so far that was just thrown as it was and I'm pretty okay. Might even make sense to remove the try..catch block above to mimick that.

Copy link
Contributor Author

@thorn0 thorn0 Sep 10, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As for an error in condition, so far that was just thrown as it was and I'm pretty okay. Might even make sense to remove the try..catch block above to mimick that.

Okay, let's do it that way. (I somewhat dislike the fact that in this case rejected is dispatched without corresponding pending, but that's a separate issue.)

@phryneas
Copy link
Member

phryneas commented Sep 10, 2021

Looks good to me!

I'm gonna approve this one, but not merge it yet because I think we'll still want to release a patch release before the next minor and this is stuff for a minor - so please feel free to use the CodeSandbox build in the meantime.

@phryneas phryneas added the enhancement New feature or request label Sep 10, 2021
@phryneas phryneas added this to the 1.7 milestone Sep 10, 2021
@phryneas phryneas changed the base branch from master to v1.7.0-integration October 1, 2021 08:15
@phryneas phryneas merged commit 3fb4526 into reduxjs:v1.7.0-integration Oct 1, 2021
@thorn0 thorn0 deleted the async-condition branch October 9, 2021 17:02
@phryneas phryneas removed the enhancement New feature or request label Oct 28, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Async condition in createAsyncThunk
2 participants