Include case reducers in createSlice result #209

markerikson · 2019-10-07T03:36:18Z

- 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

markerikson · 2019-10-07T03:37:42Z

@phryneas , @denisw , @Dudeonyx , @Jessidhia , @nickmccurdy : can I get some eyes on this to make sure I didn't do anything really stupid?

The unit tests and type tests still pass, and it looks like everything is being inferred correctly in the output.

netlify · 2019-10-07T03:40:20Z

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

Built with commit 8f6add0

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

phryneas

It needs the round-trip to something with the initial reducer types, or it just accepts anything.

If you want to, I can give it a try in the evening - except if you want to experiment around with it? :)

phryneas · 2019-10-07T06:51:27Z

src/createSlice.ts

@@ -35,14 +35,19 @@ export interface Slice<
   * reducer.
   */
  actions: ActionCreators
+
+  caseReducers: SliceCaseReducers<State, any>


This is too generic, it needs some relation to the original reducers or it will broaden to accept any type of reducer with just any key. I'll add two example tests below.

Trying to poke at this now. Got any suggestions for approaches?

A possible solution (that is potentially breaking) is to change the type of the second generic argument on most things in this file; a lot of things here are overly broad, not just the changes done in this PR.

Make them all use the PA extends PayloadActions instead of types that are derived from it; put the derivation in the parameter list / helper types / interface declarations themselves. The fundamental units here are the State object and the PayloadActions that are handled, as they're present in almost all types and/or are used to generate type arguments for other types.

Speaking of which, PayloadActions itself is erasing types; it does not preserve the "individuality" of each PayloadAction it contains; fixing that is even more involved than this, though, and I think the current problems we're pointing out can be fixed before/without addressing that as it's only used in extends clauses instead of concrete instances (higher kinded types yay 😓).

For example, this Slice interface should receive State and PA extends PayloadActions, the type of actions should be (what currently is) CaseReducerActions<SliceCaseReducers<State, PA>> (they're all already createActioned, you can no longer have CaseReducerWithPrepare objects in the result), and type of of caseReducers should be SliceCaseReducers<State, PA>.

Thanks for the suggestions.

I will freely admit that part of me wants to throw my hands up and say "someone else fix this!", and it may come to that. But, I'd also like to start becoming at least not completely incompetent in this area.

For the moment, is there a simpler approach that would help improve the current PR without trying to redo the rest of the types right now? Should we just get this PR in and tackle everything else in a follow-up?

For a simple approach:

the createSlice method signature passes CaseReducers on to Slice instead of currently CaseReducerActions<CaseReducers>

Slice changes it's second generic argument from ActionCreators extends { [key: string]: any } = { [key: string]: any }to CaseReducers extends SliceCaseReducerDefinitions<State, unknown> (I believe the default value would even satisfy that, as broad as it is)

actions: ActionCreators becomes type CaseReducerActions<CaseReducers>

that way, you preserve given behaviour, but within Slice, you now have access to the CaseReducers instead of the derived CaseReducerActions.

Then you have to create a new type that maps the values of CaseReducers to always the reducer, and use that for the caseReducers.

Another option with maybe even less changes would be to change your definition of

// from type SliceCaseReducers<State, PA extends PayloadActions> = { [ActionType in keyof PA]: CaseReducer<State, PA[ActionType]> } // to something like type SliceCaseReducers<State, ActionCreators extends { [key: string]: unknown }> = { [ActionType in keyof ActionCreators]: CaseReducer<State, ReturnValue<ActionCreators[ActionType]>> }

And actually pass ActionCreators as a second parameter to this interface, instead of any.

But honestly, I'm not sure if that will work out in the long run (also, I've written this without actually testing it, so it might not work)

Trying your CaseReducers suggestion now, and I'm getting two errors:

Type 'unknown' does not satisfy the constraint 'Record<string, WithPayload<any, Action<string>>>'

Required type parameters may not follow optional type parameters.

I can resolve the first one by replacing unknown with PayloadAction (the record type). Trying to figure out what an appropriate default type might be here.

Resolved the second error by simply swapping the order of the generics in Slice :)

So. Coming up with a type for the reducers. I feel like the CaseReducerActions type is roughly what I want to mimic. I "just" need to figure out how to reference the reducer functions instead.

And I think I have something!

phryneas · 2019-10-07T06:52:21Z

type-tests/files/createSlice.typetest.ts

+
+  expectType<(state: number, action: PayloadAction<number>) => number | void>(
+    counter.caseReducers.increment
+  )


these two additional tests should be failing, but currently don't

// typings:expect-error expectType<(state: number, action: PayloadAction<string>) => number | void>( counter.caseReducers.increment ) // typings:expect-error expectType<(state: number, action: PayloadAction<string>) => number | void>( counter.caseReducers.someThingNonExistant )

Thanks! These were a huge help!

I'd add one non-error-check for decrement here, as currently you're just testing for expect-errors on that one.

something like this one.

// Should match positively for reducers with prepare callback expectType<(state: number, action: PayloadAction<number>) => number | void>( counter.caseReducers.decrement )

nickserv · 2019-10-07T15:36:44Z

src/createSlice.ts

-  CR extends SliceCaseReducers<State, any> = SliceCaseReducers<State, any>
+  CR extends SliceCaseReducerDefinitions<
+    State,
+    any


Use unknown instead of any (requires TypeScript 3)

This one was there already, but yeah, we can try that.

Nope. Adding unknown in the places you're suggesting just broke all the existing type tests.

What's the error? It's possible the type tests are running on an old version of TS, or that they need type guards.

redux-starter-kit/src/createSlice.ts (49, 5) Type 'unknown' does not satisfy the constraint 'Record<string, WithPayload<any, Action<string>>>'.

And no, we're on TS 3.4.3 right now.

I’m not sure why that doesn’t match, but if the return type uses any anyway, maybe this can be left alone for now

You can pretty easily change all those parts in the extends statement from any to unknown without consequence.
But doing it for the default value will in some places be a breaking change for people who didn't specify a type manually and where the type could not be inferred.
Arguably, this is how they shouldn't be using TS in the first place if they want it for type-safety, but it definitely is used that way (see #165), so maybe that should be done with caution in a different PR.

src/createSlice.ts

nickserv · 2019-10-07T15:40:54Z

src/createSlice.ts

+
+  const sliceCaseReducersByName: SliceCaseReducers<State, any> = {}
+  const sliceCaseReducersByType: SliceCaseReducers<State, any> = {}
+  const actionCreators: any = {}


By forcing these types to be any, the inference on the returned reducers will be broken. It's better to use inference features (such as mapped types) and rely on a generic defaulting to any/unknown types instead. Let me know if you need help figuring this out.

The actionCreators : any here was actually how the existing code worked as far as I can see, and the types of the exported action creators are still correct based on VS Code's inspection and the type tests.

As the method has a defined return type, as long as the value you are returning somehow satisfies that type, you can do pretty much inside - the outside interface is defined.
Even casting to any would be valid here, and in some places I think it is to make stuff less difficult.

As an alternative, for example you could type the actionCreators to CaseReducerActions<CaseReducers>. As a result, the assignment of {} would fail, and you would have to assign it {} as any instead.
But you've got to die one death here, and I think that would be acceptable, as it would at least guard all assignments. (Even though it could never make sure all the values were actually assigned, just that those being assigned are correct. But still, more typesafe than any alone)

But, as I said, even typing this as any as it is is probably okay - as long as the method is tested thoroughly, and that it is.

nickserv · 2019-10-07T15:41:06Z

src/createSlice.ts

+    const type = getType(name, reducerName)
+
+    let caseReducer: CaseReducer<State, any>
+    let prepareCallback: PrepareAction<any> | undefined


See previous comment

markerikson · 2019-10-08T02:00:45Z

@phryneas :

It needs the round-trip to something with the initial reducer types, or it just accepts anything.

Not sure I quite understand what you're saying here. Can you clarify?

markerikson · 2019-10-09T04:59:47Z

Awright, I think I actually have a working solution here!

I added the two type tests suggested by @phryneas , and attempted to implement the suggested solution ( #209 (comment) ).

I fixed the "can't have a required type after a default type" error by swapping the order of the two generics in Slice.

From there, I tried to set up some kind of mapping, loosely based on the CaseReducerActions type. My first couple attempts got the caseReducers.nonExistent test failing as desired, but the mismatched payload test was incorrectly passing without error due to the payload being inferred as any:

type ExtractPayloadActions<T> = T extends SliceCaseReducerDefinitions<
  any,
  infer PA
>
  ? PA
  : never

type OnlyCaseReducers<
  CaseReducers extends SliceCaseReducerDefinitions<any, any>,
  State = any
> = {
  [Type in keyof CaseReducers]: CaseReducer<
    State,
    ExtractPayloadActions<CaseReducers>[Type]
  >
}

I eventually figured out that it was basically falling back to a default PayloadAction<any> somehow.

That clued me in that I need to extract the specific action type for this situation. Inspired by the PayloadForReducer type, I came up with this:

type ActionForReducer<R, ActionType = Action<string>> = R extends (
  state: any,
  action: PayloadAction<infer P>
) => any
  ? PayloadAction<P>
  : ActionType

type OnlyCaseReducers<
  CaseReducers extends SliceCaseReducerDefinitions<any, any>,
  State = any
> = {
  [Type in keyof CaseReducers]: CaseReducer<
    State,
    ActionForReducer<CaseReducers[Type]>
  >
}

That got both the "mismatched payload" and "nonexisting field" type tests throwing errors as expected.

I'm not sold on the name OnlyCaseReducers, but I'm open to suggestions.

phryneas · 2019-10-09T06:38:38Z

You're on a good way here, but not quite there yet (and those types are complicated, so every little bit of frustration you might be feeling is perfectly understandable!).

The important thing here is to identify why your ExtractPayloadActions type or ActionForReducer might fail.

First your ExtractPayloadActions type: in theory that's perfectly fine! But the SliceCaseReducerDefinitions type itself is unfortunately so complex, that TS doesn't manage fitting "the right thing" into the Generic any more and ends up with something that definitely fits: any (at least that's how I explain it to myself, others to the rescue).

In your ActionForReducer, you're doing exactly the right next step: when TS can't figure it out, you need to be more specific. And for the first part, that does look good to me. The "else" part needs a bit works still, though.

Take a look at potential shapes that could be passed to the R parameter.
The default shape is the one you already identified: ( state: any, action: PayloadAction<P>) => any
But then you're just falling back to ActionType - and as ActionType has no further mentioning in the type, it can't be inferred by TS and will always be it's default value Action<string>.

But there is another shape you can test for that we definitely know of:

{
  reducer(state: any,  action: PayloadAction<P>): any
}

so you can do a second nested ternary there, testing for that type and inferring P from there.

As for the "does not match any of the above" branch, I would not go for your ActionType or any Action, but just for never, void, unknown or the likes. If it doesn't match those, we're in a case that should never happen, so there's no need for a sensible fallback, in contrary: that fallback might prevent us from catching bugs if it isn't abviously unfitting.

I would suggest you add another test for the case with an enhanced reducer (including expect-error tests like I wrote above) - and then you're really super close :)

markerikson · 2019-10-09T15:55:59Z

@phryneas : thanks for coaching me through this! As I said, part of me just wants someone to fix all this and make it work so we can push RSK up to 1.0 ASAP, but at the same time I really need to start becoming not-totally-incompetent with this complex type stuff.

I'll try to tackle your suggested directions myself and see what happens.

markerikson · 2019-10-10T02:00:07Z

@phryneas : had to read through your instructions a couple times, but turns out you were really walking me through this one, weren't you :)

(I recognize the "tell the clueless n00b the exact steps they need to do, politely" phrasing. Goodness knows I've done that enough times myself.)

Added the additional type test, implemented the ternary, and saw that test start failing as expected.

From there, I cleaned up a few of the types in createSlice a bit, and decided to rename OnlyCaseReducers to SliceDefinedCaseReducers. (There are entirely too many uses of the words "Slice", "Case", and "Reducers" in this file already, but oh well!)

How's this look now?

phryneas · 2019-10-10T06:29:16Z

src/createSlice.ts

@@ -106,8 +111,21 @@ type PrepareActionForReducer<R> = R extends { prepare: infer Prepare }
  ? Prepare
  : never

-type CaseReducerActions<CaseReducers extends SliceCaseReducers<any, any>> = {
-  [Type in keyof CaseReducers]: IfIsEnhancedReducer<
+type ActionForReducer<R, S> = R extends (


As you don't really care for the State type here, I believe you can just leave it out.

type ActionForReducer<R> = R extends ( state: any, action: PayloadAction<infer P> ) => any ? PayloadAction<P> : R extends { reducer(state: any, action: PayloadAction<infer P>): any } ? PayloadAction<P> : unknown

Yeah, that's basically what I originally had. @nickmccurdy suggested I might as well use State instead of any for consistency, and it doesn't seem to actually hurt anything.

It doesn't hurt - it's just one more thing to carry around :)

Explanation why I think it's not necessary here:
What you are doing here is shape-matching. You're either looking for an object with a reducer method or for a method itself, both methods with at least two arguments. From that moment on you just care about the type of the second argument.
Sure, you can choose to only match if the first argument is State, but that means a second parameter to the generic, also caring about that second parameter when calling. So, essentially: more code, without any direct benefit.
So I'd just let it out. But if you choose to let it in there, don't forget to do the same check in line 120, too.

phryneas · 2019-10-10T06:30:17Z

src/createSlice.ts

@@ -122,6 +140,16 @@ type CaseReducerActions<CaseReducers extends SliceCaseReducers<any, any>> = {
  >
 }

+type SliceDefinedCaseReducers<


as a consequence, no need for the State parameter here as well

phryneas · 2019-10-10T06:42:24Z

A little bit - but in the end, you did it, with all the failures that come along on the way.

Just a little secret: for the reducer with payload stuff, I wrote that whole thing at least four times from scratch, completely resetting because something was not behaving as I wanted and I was going routine-blinded with my existing code. Wanted to spare you of that frustration, because it is not a learning one ;)

The code looks good now, I've added two more small comments, but I believe you did it :)

It's early in the morning here and I haven't had my coffee yet, so I'll do a second round of review in the evening, just in case I missed something.

phryneas

Yeah, gave this another round, I believe it looks good now.

All open comments from me are just nitpicks, so do them if you feel like it, but it's fine either way 👍

phryneas · 2019-10-10T20:44:14Z

src/createSlice.ts

-  State = any,
-  ActionCreators extends { [key: string]: any } = { [key: string]: any }
+  CaseReducers extends SliceCaseReducerDefinitions<State, PayloadActions>,
+  State = any


You could bring this back into the original order by giving the CaseReducers the same default value as ActionCreators had before (that should be working). That way, things might still break for ppl who used this type directly, but maybe not as significantly.

Sure, that works.

phryneas · 2019-10-10T20:46:22Z

type-tests/files/createSlice.typetest.ts

+
+  expectType<(state: number, action: PayloadAction<number>) => number | void>(
+    counter.caseReducers.increment
+  )


something like this one.

// Should match positively for reducers with prepare callback expectType<(state: number, action: PayloadAction<number>) => number | void>( counter.caseReducers.decrement )

markerikson · 2019-10-11T02:16:03Z

Awright, let's do this!

* 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 added 2 commits October 6, 2019 21:59

Fix missing name field in type test

67d96f7

phryneas reviewed Oct 7, 2019

View reviewed changes

nickserv requested changes Oct 7, 2019

View reviewed changes

markerikson added 2 commits October 9, 2019 00:42

Add type tests for correct case reducer export inference

36e966f

Rewrite caseReducers types for correct inference of state + action

dd7f19b

markerikson added 3 commits October 9, 2019 21:46

Add type test for reducers with prepare callback

d9cf216

Fix type inference for actions from reducers w/ prepare callbacks

731c744

Clean up type names and usages

184e5ab

Use a generic State type in ActionForReducer

a34bf93

phryneas reviewed Oct 10, 2019

View reviewed changes

Re-switch Slice generics order

8f6add0

markerikson merged commit 209bf06 into v0.8-integration Oct 11, 2019

markerikson deleted the feature/return-slice-reducers branch October 11, 2019 02:16

markerikson mentioned this pull request Oct 15, 2019

Port to TypeScript reduxjs/redux#3500

Closed

Include case reducers in createSlice result #209

Include case reducers in createSlice result #209

Conversation

markerikson commented Oct 7, 2019

markerikson commented Oct 7, 2019 • edited Loading

netlify bot commented Oct 7, 2019 • edited Loading

phryneas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Jessidhia Oct 8, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nickserv Oct 7, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

markerikson commented Oct 8, 2019

markerikson commented Oct 9, 2019

phryneas commented Oct 9, 2019 • edited Loading

markerikson commented Oct 9, 2019

markerikson commented Oct 10, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

phryneas commented Oct 10, 2019

phryneas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

markerikson commented Oct 11, 2019

markerikson commented Oct 7, 2019 •

edited

Loading

netlify bot commented Oct 7, 2019 •

edited

Loading

Jessidhia Oct 8, 2019 •

edited

Loading

nickserv Oct 7, 2019 •

edited

Loading

phryneas commented Oct 9, 2019 •

edited

Loading