Skip to content

set error to true when payload instanceof Error#136

Merged
globalchubby merged 3 commits intoredux-utilities:masterfrom
xiaohanzhang:patch-1
Sep 27, 2016
Merged

set error to true when payload instanceof Error#136
globalchubby merged 3 commits intoredux-utilities:masterfrom
xiaohanzhang:patch-1

Conversation

@xiaohanzhang
Copy link
Copy Markdown
Contributor

Should set error to true when payloadCreator return error

Should set error to true when payloadCreator return error
Copy link
Copy Markdown
Contributor

@globalchubby globalchubby left a comment

Choose a reason for hiding this comment

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

@xiaohanzhang Thanks for the contribution!

  • can you write some tests around this?
  • can you describe some specific use cases (when I have an error, I usually pass the error to the action creator directly)?

@globalchubby
Copy link
Copy Markdown
Contributor

#43 is related.

@xiaohanzhang can you describe the use case?

@timche if the use case makes sense, I don't see a problem with merging this, as it seems to have been asked for and supported before.

@xiaohanzhang
Copy link
Copy Markdown
Contributor Author

@yangmillstheory Thanks for great work and quick reply, I added a test for it
use case:

const fooAction = createAction('FOO', (...args) => {
  return isValid(args) ? {foo: 'bar'} : new Error('arguments is not valid');
});

I feel it will make more sense to put arguments validator inside actions, otherwise we need check it in every components.
With current implementation, I need create another wrapper for real action:

const fooActionWrapper = (...args) => {
  return isValid(args) ? fooAction(...args) : fooAction(new Error());
}

@globalchubby
Copy link
Copy Markdown
Contributor

Thanks @xiaohanzhang. I think we should merge (pending @timche's feedback), but I would like to understand this a bit deeper, beyond avoiding writing wrappers.

Can you give a couple of examples of an actual flow in your application where this comes in useful? What kind of validation are you doing in your payload creators?

@xiaohanzhang
Copy link
Copy Markdown
Contributor Author

We have some rest api action, I want add more client-side validation before it send to server.
It been used by many components and projects. So I hope can put the validator inside action.

@globalchubby
Copy link
Copy Markdown
Contributor

@timche please let us know if there are any blockers.

@holybom
Copy link
Copy Markdown

holybom commented Sep 24, 2016

Hi, this new change doesn't seem to affect payloadCreator that returns a Promise, there's no error field in the dispatched action. I'm using redux-promise as well.

export const myAction = createAction('ACTION_TYPE', () => myAsyncFunc());

myAsyncFunc does some network requests and returns a Promise that can resolve into an Error. Can you suggest a solution? Is it fine to resolve the payload before checking if it's an Error?

@xiaohanzhang
Copy link
Copy Markdown
Contributor Author

@holybom I usually just throw an error inside myAsyncFunc, redux-promise will catch that error:
https://github.com/acdlite/redux-promise/blob/master/src/index.js#L19

Copy link
Copy Markdown
Member

@timche timche left a comment

Choose a reason for hiding this comment

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

👍

@globalchubby globalchubby merged commit f0f3c94 into redux-utilities:master Sep 27, 2016
@xiaohanzhang xiaohanzhang deleted the patch-1 branch October 6, 2016 20:13
@TimothyGu TimothyGu mentioned this pull request Oct 30, 2016
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.

4 participants