Use `void` instead of `any` for undefined payloads #174

Krisztiaan · 2019-08-14T11:00:19Z

Additional layer of type security, don't let the user implicitly opt out of type checking

netlify · 2019-08-14T11:02:14Z

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

Built with commit e9a9ad3

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

markerikson · 2019-10-07T00:47:26Z

Hmm. A bunch of type tests fail because of this change. I'm not clear what improvement this is supposed to make.

Going to close this for now. If you still feel it's a point of concern, please let me know and we can discuss further.

Krisztiaan · 2019-10-09T09:04:02Z

@markerikson thanks for taking a look, we've made this exact patch in our project using this library, and it revealed a bunch of missed or mismatched action payload typings, while forcing us to specify payload where it was expected. I would look into fixing the tests if we can agree this would be beneficial to have merged.

markerikson · 2019-10-09T15:52:46Z

Sure, sounds good.

Hmm. I don't suppose this is related to #211 at all?

markerikson · 2019-10-10T02:12:18Z

@phryneas : since you're consulting, any thoughts on this one?

markerikson · 2019-10-10T02:31:34Z

So looking at the type errors, some of them from "raw" uses of PayloadAction like this:

  const action: PayloadAction = { type: '', payload: 5 }
  const numberPayload: number = action.payload

Also:

  const increment = createAction('increment')
  // Error: Argument of type '1' is not assignable to parameter of type 'void | undefined'
  const n: number = increment(1).payload

Those are fixable by adding generic to PayloadAction<number> and createAction<number>. Potentially reasonable. But, I'm not entirely sure that's "right" here.

phryneas · 2019-10-10T06:21:47Z

Well, it would definitely change the defined behaviour.

Especially this test defines that a PayloadAction should fall back to any if there's no type defined. And #165 shows that people are using it like this.

I guess all other of those tests are building on this "acceptance criteria" that was formulated in this first test.

As for me, I'd like this to change, as those types being more strict will prevent bugs like people accidentally forgetting the type parameter and then doing werid stuff.
But this will be breaking for people.

Your call if you want this :)

Krisztiaan · 2019-10-10T13:31:59Z

Sure, sounds good.

Hmm. I don't suppose this is related to #211 at all?

This does not seem related, as the PR would only cover the default payload type that is used when nothing is supplied

markerikson · 2019-10-11T01:43:10Z

I guess it would help if I better understood the ramifications of the type change, in terms of end-user experience, actual code usage, and tests that would have to change.

Can anyone add some clarification?

phryneas · 2019-10-11T06:46:54Z

I think basarat describes this a whole lot better than me: https://basarat.gitbooks.io/typescript/docs/options/noImplicitAny.html

The point is: he describes noImplicitAny, which is a compiler option the user could turn on or off (they should, TS becomes pretty much useless without it). But we're a lib here, we can't leave that decision for the user and we can't detect if the user uses noImplicitAny, we have to make a decision for everyone.

The question is do we allow all our users to do something potentially dangerous by accident, or do we require them to explicitly do that dangerous thing if they really want to?

Either we allow people to allow createAction without type argument, which then defaults to createAction<any> and eliminates essentially all type checks along the way. (This might cause a lot of bugs for ppl who normally use it but just forget it once in a while)
Or we allow no-one to use createAction without a type argument (by making it useless, defaulting to something like void). They could still use createAction<any> if they really wanted, but they's have to specify that. (This might be a little inconvenient for ppl who use TS but don't care about type safety?)

As for the tests: currently, it seems to be a "business requirement" that it works as it currently does. I don't know who initially formulated it like this, and what they had in minde 🤷. I guess a few test would need to be replaced with other tests. But I also guess that would be fine.

markerikson · 2019-10-11T15:48:14Z

Yeah, I think I can get behind making the typing more explicit here, especially since most folks would be using createSlice anyway vs calling createAction themselves.

Would one of you be able to tackle this? I'd want to get it into 0.8 as well, but I want to start working on updating the tutorial contents.

markerikson · 2019-10-13T19:45:23Z

Okay, I re-created the branch off of v0.8-integration, and fixed up the type tests so they pass. I'm not 100% sure the type test changes are correct, though. Could both of you look them over?

phryneas · 2019-10-13T21:26:29Z

type-tests/files/createAction.typetest.ts

@@ -22,16 +22,19 @@ function expectType<T>(p: T): T {
 * Test: PayloadAction type parameter is optional (defaults to `any`).


defaults to any -> should not default to any

phryneas · 2019-10-13T21:28:26Z

type-tests/files/createAction.typetest.ts

@@ -58,10 +61,12 @@ function expectType<T>(p: T): T {
      payload
    }),
    { type: 'action' }
-  ) as PayloadActionCreator
+  ) as PayloadActionCreator<number>


I think this should be number|undefined

phryneas · 2019-10-13T21:28:38Z

type-tests/files/createAction.typetest.ts


  expectType<PayloadAction<number>>(actionCreator(1))
+  // typings:expect-error


and these two should actually not fail then

phryneas · 2019-10-13T21:29:57Z

type-tests/files/createAction.typetest.ts

@@ -110,22 +115,21 @@ function expectType<T>(p: T): T {
 }

 /*
- * Test: createAction() type parameter is optional (defaults to `any`).
+ * Test: createAction() type parameter is required, not inferred (defaults to `void`).


this test doesn't really make any more sense (description does not match what it tests right now), so I would delete it

yeah, lemme rewrite this one

phryneas · 2019-10-13T21:59:30Z

Looks good from my side.

markerikson · 2019-10-13T22:03:49Z

great, thanks!

* Create slice changes (#197) * Include case reducers in createSlice result (#209) * Fix missing name field in type test * Include provided case reducers in createSlice output - Added `caseReducers` field to the createSlice return object - Added test and type test for returned case reducer - Removed "Enhanced" terminology from types, and replaced with "CaseReducerWithPrepare" and "CaseReducerDefinition" - Restructured logic to only loop over reducer names once, and check case reducer definition type once per entry * Add type tests for correct case reducer export inference * Rewrite caseReducers types for correct inference of state + action * Add type test for reducers with prepare callback * Fix type inference for actions from reducers w/ prepare callbacks * Clean up type names and usages * Use a generic State type in ActionForReducer * Re-switch Slice generics order * Use `void` instead of `any` for undefined payloads (#174) * Change default createAction payload type to void to make it required * Fix createAction type tests per review * Update tutorials for v0.8 (#213) * Update basic tutorial with createSlice changes * Update intermediate tutorial with createSlice changes * Update advanced tutorial with createSlice changes * Change existing commit links to point to reduxjs org repos

markerikson closed this Oct 7, 2019

markerikson reopened this Oct 9, 2019

markerikson changed the base branch from master to v0.8-integration October 13, 2019 18:34

Change default createAction payload type to void to make it required

6d3528a

markerikson mentioned this pull request Oct 13, 2019

Update tutorials for v0.8 #213

Merged

phryneas requested changes Oct 13, 2019

View reviewed changes

Fix createAction type tests per review

e9a9ad3

markerikson merged commit cd8f9e5 into reduxjs:v0.8-integration Oct 13, 2019

markerikson mentioned this pull request Nov 9, 2019

Q: createAction with payload typings #211

Closed

markerikson pushed a commit that referenced this pull request Apr 20, 2021

Docs: correct import statement for setupListeners (#174)

b7f6b42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use `void` instead of `any` for undefined payloads #174

Use `void` instead of `any` for undefined payloads #174

Krisztiaan commented Aug 14, 2019

netlify bot commented Aug 14, 2019 •

edited

Loading

markerikson commented Oct 7, 2019

Krisztiaan commented Oct 9, 2019

markerikson commented Oct 9, 2019 •

edited

Loading

markerikson commented Oct 10, 2019

markerikson commented Oct 10, 2019

phryneas commented Oct 10, 2019

Krisztiaan commented Oct 10, 2019

markerikson commented Oct 11, 2019 •

edited

Loading

phryneas commented Oct 11, 2019

markerikson commented Oct 11, 2019

markerikson commented Oct 13, 2019

phryneas Oct 13, 2019

phryneas Oct 13, 2019

phryneas Oct 13, 2019

phryneas Oct 13, 2019

markerikson Oct 13, 2019

phryneas commented Oct 13, 2019

markerikson commented Oct 13, 2019

		@@ -22,16 +22,19 @@ function expectType<T>(p: T): T {
		* Test: PayloadAction type parameter is optional (defaults to `any`).


		expectType<PayloadAction<number>>(actionCreator(1))
		// typings:expect-error

Use void instead of any for undefined payloads #174

Use void instead of any for undefined payloads #174

Conversation

Krisztiaan commented Aug 14, 2019

netlify bot commented Aug 14, 2019 • edited Loading

markerikson commented Oct 7, 2019

Krisztiaan commented Oct 9, 2019

markerikson commented Oct 9, 2019 • edited Loading

markerikson commented Oct 10, 2019

markerikson commented Oct 10, 2019

phryneas commented Oct 10, 2019

Krisztiaan commented Oct 10, 2019

markerikson commented Oct 11, 2019 • edited Loading

phryneas commented Oct 11, 2019

markerikson commented Oct 11, 2019

markerikson commented Oct 13, 2019

phryneas Oct 13, 2019

Choose a reason for hiding this comment

phryneas Oct 13, 2019

Choose a reason for hiding this comment

phryneas Oct 13, 2019

Choose a reason for hiding this comment

phryneas Oct 13, 2019

Choose a reason for hiding this comment

markerikson Oct 13, 2019

Choose a reason for hiding this comment

phryneas commented Oct 13, 2019

markerikson commented Oct 13, 2019

Use `void` instead of `any` for undefined payloads #174

Use `void` instead of `any` for undefined payloads #174

netlify bot commented Aug 14, 2019 •

edited

Loading

markerikson commented Oct 9, 2019 •

edited

Loading

markerikson commented Oct 11, 2019 •

edited

Loading