-
-
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
feat(action): support optional error field #222
Conversation
Deploy preview for redux-starter-kit-docs ready! Built with commit 987bdef https://deploy-preview-222--redux-starter-kit-docs.netlify.com |
Y'know, that's a good point. It's frankly the least-used part of the FSA spec (most folks just dispatch Can you add some types tests for this functionality as well? @phryneas , any thoughts here? |
Looks good to me, but as you said, this still need type-tests. @tvanier : take a look over here : https://github.com/reduxjs/redux-starter-kit/blob/master/type-tests/files/createAction.typetest.ts Some additional tests that if prepareAction returns an error, it is also defined on the created Action with the same type. And additionally, just add some |
If we can get this in in the next day or two, we can get this into 1.0. Otherwise, we'll just add it afterwards (1.0.1 or something). |
Sorry for having missed some tests, I am not too familiar with type-testing, just added some as suggested by @phryneas , let me know what you think. And please do not block the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added a few comments - unfortunately our type-tests aren't as specific on what they're testing as I'd like them to be everywhere, so they might not have been a very good example
@@ -156,6 +156,8 @@ function expectType<T>(p: T): T { | |||
|
|||
// typings:expect-error | |||
expectType<string>(strLenAction('test').payload) | |||
// typings:expect-error | |||
expectType<boolean>(strLenAction('test').error) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please replace this with something like
// typings:expect-error
const x: any = strLenMetaAction('test').error
you want to see that the property does not exist at all, while the
// typings:expect-error
expectType<boolean>(...)
just ensures that it isn't boolean.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I updated the test as suggested.
Out of curiosity, would it be possible to add Jest expectations here?
// typings:expect-error
const error: any = strLenAction('test').error
expect(error).toBeUndefined()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think that code is actually ever run, it should only be compiled to look for compiler warnings. Doing something like this would work better in the normal tests.
We just want to make sure that there's no regression in the types & autocompletion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me now.
Thanks @phryneas . |
Cool. Awright, in it goes! |
Great, thanks @markerikson and @phryneas ! |
Coming Soon (TM) :) |
Allow
prepareAction
to return an optionalerror
field as per the Flux specificationhttps://github.com/redux-utilities/flux-standard-action#actions