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(action): support optional error field #222

Merged
merged 3 commits into from
Oct 22, 2019

Conversation

tvanier
Copy link
Contributor

@tvanier tvanier commented Oct 19, 2019

Allow prepareAction to return an optional error field as per the Flux specification
https://github.com/redux-utilities/flux-standard-action#actions

@netlify
Copy link

netlify bot commented Oct 19, 2019

Deploy preview for redux-starter-kit-docs ready!

Built with commit 987bdef

https://deploy-preview-222--redux-starter-kit-docs.netlify.com

@markerikson
Copy link
Collaborator

Y'know, that's a good point.

It's frankly the least-used part of the FSA spec (most folks just dispatch SOME_ACTION_ERROR instead), but this would be a reasonable thing to include.

Can you add some types tests for this functionality as well?

@phryneas , any thoughts here?

@phryneas
Copy link
Member

phryneas commented Oct 19, 2019

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 // expect-error cases to existing tests, meaning that if no error is returned from prepareAction, no error property is present on the returned action.

@markerikson
Copy link
Collaborator

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).

@tvanier
Copy link
Contributor Author

tvanier commented Oct 19, 2019

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 1.0 release if this PR is not ready! (although supporting error would help our project)

Copy link
Member

@phryneas phryneas left a 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)
Copy link
Member

@phryneas phryneas Oct 20, 2019

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.

Copy link
Contributor Author

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()

Copy link
Member

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.

type-tests/files/createAction.typetest.ts Outdated Show resolved Hide resolved
type-tests/files/createAction.typetest.ts Show resolved Hide resolved
Copy link
Member

@phryneas phryneas left a 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.

@tvanier
Copy link
Contributor Author

tvanier commented Oct 21, 2019

Thanks @phryneas .
@markerikson It would be great if this could be merged before releasing 1.0, I am adding the starter kit to a project where action.error is used already.

@markerikson
Copy link
Collaborator

Cool. Awright, in it goes!

@markerikson markerikson merged commit c4bb2ae into reduxjs:master Oct 22, 2019
@tvanier
Copy link
Contributor Author

tvanier commented Oct 22, 2019

Great, thanks @markerikson and @phryneas !
Do we have an ETA for 1.0, or 0.10 maybe?

@tvanier tvanier deleted the action-error branch October 22, 2019 14:37
@markerikson
Copy link
Collaborator

Coming Soon (TM) :)

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