-
-
Notifications
You must be signed in to change notification settings - Fork 98
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 support for error field to createAction #108
Conversation
Hi @piotrwitek do you have any feedback for this PR? Just checking in, as I see the merge conflicts accumulate :-) |
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.
Thanks for the reminder, you did a great job here, a couple of review comments:
- Documentation improvements are great, I would merge it ASAP if you could extract it to separate PR.
action
andcreateAction
needs new tests covering all the possible permutations of parameters to cover complexity of added definitions- Both of above needs type-tests for each case using dts-jest - this is actually really simple to do, just take a look how I did it here: cd22179#diff-4ecc0e1a3e5d6b6fefbdeb5966f18b3b
- I see a lot of duplication in new conditional types and action overloads that can be clearly simplified. I have also doubts that would work as you expect it to, I would advise to setup a type-tests suite to help you debug all possible parameters permutations and see if they are actually matching expected parameters.
Without complete type-tests suite I will not be able to merge it as it is impossible to confirm that it actually works.
@@ -15,13 +15,13 @@ and **complexity** (explained in [motivation](#motivation)). | |||
[![NPM Downloads](https://img.shields.io/npm/dt/typesafe-actions.svg)](https://www.npmjs.com/package/typesafe-actions) | |||
[![Bundlephobia Size](https://img.shields.io/bundlephobia/minzip/typesafe-actions.svg)](https://www.npmjs.com/package/typesafe-actions) | |||
|
|||
> #### :star: _Found it useful? Want more updates?_ [**Show your support by giving a :star:**](https://github.com/piotrwitek/typesafe-actions/stargazers) |
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.
Could you please disable truncating line endings in md files, they are intentional for formatting the document
@@ -18,17 +22,42 @@ export function action<T extends StringType, P = undefined, M = undefined>( | |||
meta: M | |||
): PayloadMetaAction<T, P, M>; | |||
|
|||
export function action<T extends StringType, E = undefined>( |
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 didn't checked it in IDE but I can see with naked eye, this cannot work correctly, there are duplicated overload down the line (overload with the same params cannot have different return types because that would not work), a different method should be used, probably conditionals and E should be constrained with boolean to have a correct overload lock in.
You heavn't setup type test-cases covering all the parametres permutations and that would gave you a important feedback loop during design phase.
error: E, | ||
): ErrorAction<T, E>; | ||
|
||
export function action<T extends StringType, P = undefined, E = undefined>( |
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.
same here, it's basically duplication of: export function action<T extends StringType, P = undefined, M = undefined>(
compiler would match only the first overload, never second
type ActionWithPayload<T, P> = { type: T; payload: P }; | ||
type ActionWithMeta<T, M> = { type: T; meta: M }; | ||
type ActionWithPayloadAndMeta<T, P, M> = { type: T; payload: P; meta: M }; | ||
type ActionWithError<T, E> = { type: T; error: E }; |
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.
ActionWithError type is the same as ActionWithMeta, without constrains it will not work
@piotrwitek Thanks for the feedback, this is great. I will probably have to do some trial and error plus research, but at least I know it's worth working on 👍 |
@sjsyrek you're welcome :) I reminded myself one thing that I was experimenting with something similar in the past and I hit the wall that it was impossible to do with current type system limitations, especially stay away from overloads (overloads does break in few type inference scenarios), conditionals are the way to go. That's why I have refactored a lot of internal types from overloads to conditional and I still have some of the refactoring left on a local branch that I plan to merge this week. So I would wait for this because then you'll have a cleaner and more robust baseline to work on. I'll be also refactoring and simplifying all the remaining type-tests to dts-jest. After all technical debt is cleared I have one cool feature I've been working on in secret for a while :) |
@piotrwitek I saw the recent changes... do they make this PR unnecessary? Or is it still useful to have a FSA creator that includes an error field? I like the brevity of |
Hey @sjsyrek, yes I would definitely extend createAction to cover this common case, but I would do it with slightly different API. As for solution proposal I would go with a third argument being a bag object, that will be merged with action object. Here is a usage example: createAction(types.WITH_PAYLOAD_META_ERROR, action => {
return (id: number, token: string, error: boolean) => action(id, token, { error });
}); I would accept this solution as this will cover this and many different cases in the future. If you're interested in tackling it, please copy this proposal and create a new issue, where we will continue API design discussion. |
@piotrwitek Sure, I could take a crack at it. Created the issue here: #116 Side note: would it not also be worth having support for Redux Thunk, so you could also use |
@sjsyrek as for But please create a separate issue for the discussion as it is offtopic. |
@sjsyrek I think this is obsolete right? Should I close it? |
@piotrwitek I haven't had time recently to look at this but would still like to contribute if there's a need. If you think this is obsolete, I'm fine with closing it. |
Hey @sjsyrek, thanks a lot for stepping in! |
Hey @sjsyrek,
type Action = {
type: string;
error?: boolean;
}
What are your thoughts? PS: I would like to close this PR because it's outdated and create a new issue tracking this feature. |
Closing this in favor of a new proposal at #207 |
Add support for an
error
field tocreateAction
. This change does not make it a requirement that thepayload
is anError
object, and theerror
field itself is optional, both of these in accordance with the Flux standard action description. Theerror
is also not required to be boolean, as it seems up to the client to determine what to do with these values.Thanks for a great library! I really want to use this API to reduce boilerplate, but I need the
error
field, so I thought I'd offer the minimal change required to get that functioning. I added some tests and updated the README as best I could (including some general grammatical fixes) to reflect the changes.Usage
Notes
check
script, so I assume that meantnpm run ci-check
types.spec.ts
, but these depend on utilsthat use with
createStandardAction
, and I didn't want to start making my modification larger just for this purposeRelated Issue
#52
npm run reinstall
npm run check
Extra checklist:
#
tagFor bugfixes:
For new feature: