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

infer action creators from createSlice as PayloadActionCreator #158

Merged
merged 3 commits into from
Jul 16, 2019

Conversation

phryneas
Copy link
Member

@phryneas phryneas commented Jul 15, 2019

see #157

This changes the createSlice types to use PayloadActionCreator and modifies the behaviour of PayloadActionCreator to account for P = void and P = Something | undefined.

Here are some examples:

const someNumber: number = 5;
const someString: string = "";

const voidActionCreator = createAction<void>('a');
voidActionCreator(); // Action<'a'>
voidActionCreator(undefined); // Action<'a'>

const stringActionCreator = createAction<string>('b');
stringActionCreator('x'); // PayloadAction<'x', 'b'>

const optionalStringActionCreator = createAction<string | undefined>('c');
optionalStringActionCreator(); // Action<'c'>
optionalStringActionCreator(undefined); // Action<'c'>
optionalStringActionCreator('x') // PayloadAction<'x', 'c'>

const mixedActionCreator = createAction<string | number>('d');
mixedActionCreator('x') // PayloadAction<'x', 'd'>
mixedActionCreator(someNumber) // PayloadAction<number, 'd'>
mixedActionCreator(5) // PayloadAction<5, 'd'>
mixedActionCreator(someString) // PayloadAction<string, 'd'>

const anyActionCreator = createAction('e');
anyActionCreator() // Action<'e'>
anyActionCreator(someNumber) // PayloadAction<number, 'e'>
anyActionCreator(5) // PayloadAction<5, 'e'>
anyActionCreator(someString) // PayloadAction<string, 'e'>

so payload types are now interfered much more accurate, which is especially useful for the <any> case.

There is one open question open regarding the typing of the undefined argument: if undefined is not omitted, but explicitly specified, should we return an Action or a PayloadAction? (See https://github.com/reduxjs/redux-starter-kit/pull/158/files#diff-437575fb1d5c39e3edd424ab38899f8eR27 )

PS: I could easily do away with the excessive type interference, but that would hurt the <any> case a bit in my eyes and I think it won't hurt, so I'm looking forward to your opinion on it ;)

@netlify
Copy link

netlify bot commented Jul 15, 2019

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

Built with commit a960dc6

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

@markerikson
Copy link
Collaborator

I vote that createAction() should always return a PayloadAction, for consistency (and on the grounds that we'll also probably end up throwing meta in there too in the near future too).

}
})

const x: "increment" = counter.actions.increment.type;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why are these not "counter/increment"? That's really confusing to me.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good catch.
Because I typed them wrongly and those tests only check for types at compile time, not validate that it is the real output.
That said, I'll have to remove that and only type the type as string then - can't do concatenation with types :(

@markerikson
Copy link
Collaborator

One other question to throw out there. If we do eventually tack on meta, is PayloadAction really the best term? Should it maybe be FSA instead or something?

@phryneas
Copy link
Member Author

One other question to throw out there. If we do eventually tack on meta, is PayloadAction really the best term? Should it maybe be FSA instead or something?

I'm all for that. 👍

@phryneas
Copy link
Member Author

PS: Generally, I'm for a renaming, but having changed it to FSA everywhere in the code, it looks really more like a generic than a type to me, with it being only capital letters. Not really sure if that would be good for readablility. Do we have other options?

I was thinking along the lines of:

  • FSAction
  • ExtendedAction
  • SpecifiedAction
  • DetailedAction

But reading all those above, I have to say that I'd also be fine with keeping PayloadAction and just adding a third parameter for meta... Not 100% happy with any of those.

@markerikson
Copy link
Collaborator

Eh, let's keep PayloadAction for now. Less churn.

@markerikson
Copy link
Collaborator

Cool. All right, let's get this out. Thank you so much for working on this!

@markerikson markerikson merged commit 0b50f3b into reduxjs:master Jul 16, 2019
@phryneas
Copy link
Member Author

Happy to help 😎

Just because I noticed, in case you forgot (I don't know your release workflow):

While the 0.6 is available on npm, the commit & tag are still missing on GitHub. Did you forget to push?

@phryneas phryneas deleted the PR-155 branch July 16, 2019 21:59
@markerikson
Copy link
Collaborator

Not quite. I actually did the release from work, but I'm behind a corporate proxy. I use https://github.com/sindresorhus/np to do my releases, but it seems that doesn't work great behind proxies. It was able to hit NPM to publish, but not do the Github work.

I'll get those up tonight after I get home.

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.

2 participants