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

Include case reducers in createSlice result #209

Merged
merged 9 commits into from Oct 11, 2019

Conversation

@markerikson
Copy link
Collaborator

markerikson commented Oct 7, 2019

Per discussion at #82 (comment)

markerikson added 2 commits Oct 7, 2019
- 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

This comment has been minimized.

Copy link
Collaborator Author

markerikson commented Oct 7, 2019

@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

This comment has been minimized.

Copy link

netlify bot commented Oct 7, 2019

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

Built with commit 8f6add0

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

Copy link
Collaborator

phryneas left a comment

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? :)

@@ -35,14 +35,19 @@ export interface Slice<
* reducer.
*/
actions: ActionCreators

caseReducers: SliceCaseReducers<State, any>

This comment has been minimized.

Copy link
@phryneas

phryneas Oct 7, 2019

Collaborator

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.

This comment has been minimized.

Copy link
@markerikson

markerikson Oct 8, 2019

Author Collaborator

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

This comment has been minimized.

Copy link
@Jessidhia

Jessidhia Oct 8, 2019

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

This comment has been minimized.

Copy link
@markerikson

markerikson Oct 8, 2019

Author Collaborator

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?

This comment has been minimized.

Copy link
@phryneas

phryneas Oct 8, 2019

Collaborator

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.

This comment has been minimized.

Copy link
@phryneas

phryneas Oct 8, 2019

Collaborator

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)

This comment has been minimized.

Copy link
@markerikson

markerikson Oct 9, 2019

Author Collaborator

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.

This comment has been minimized.

Copy link
@markerikson

markerikson Oct 9, 2019

Author Collaborator

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.

This comment has been minimized.

Copy link
@markerikson

markerikson Oct 9, 2019

Author Collaborator

And I think I have something!


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

This comment has been minimized.

Copy link
@phryneas

phryneas Oct 7, 2019

Collaborator

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
  )

This comment has been minimized.

Copy link
@markerikson

markerikson Oct 9, 2019

Author Collaborator

Thanks! These were a huge help!

This comment has been minimized.

Copy link
@phryneas

phryneas Oct 10, 2019

Collaborator

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

This comment has been minimized.

Copy link
@phryneas

phryneas Oct 10, 2019

Collaborator

something like this one.

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

This comment has been minimized.

Copy link
@nickmccurdy

nickmccurdy Oct 7, 2019

Collaborator

Use unknown instead of any (requires TypeScript 3)

This comment has been minimized.

Copy link
@markerikson

markerikson Oct 8, 2019

Author Collaborator

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

This comment has been minimized.

Copy link
@markerikson

markerikson Oct 8, 2019

Author Collaborator

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

This comment has been minimized.

Copy link
@nickmccurdy

nickmccurdy Oct 8, 2019

Collaborator

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

This comment has been minimized.

Copy link
@markerikson

markerikson Oct 8, 2019

Author Collaborator

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.

This comment has been minimized.

Copy link
@nickmccurdy

nickmccurdy Oct 8, 2019

Collaborator

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

This comment has been minimized.

Copy link
@phryneas

phryneas Oct 8, 2019

Collaborator

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 Show resolved Hide resolved
src/createSlice.ts Show resolved Hide resolved
src/createSlice.ts Show resolved Hide resolved
src/createSlice.ts Show resolved Hide resolved

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

This comment has been minimized.

Copy link
@nickmccurdy

nickmccurdy Oct 7, 2019

Collaborator

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.

This comment has been minimized.

Copy link
@markerikson

markerikson Oct 8, 2019

Author Collaborator

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.

This comment has been minimized.

Copy link
@phryneas

phryneas Oct 8, 2019

Collaborator

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.

const type = getType(name, reducerName)

let caseReducer: CaseReducer<State, any>
let prepareCallback: PrepareAction<any> | undefined

This comment has been minimized.

Copy link
@nickmccurdy

nickmccurdy Oct 7, 2019

Collaborator

See previous comment

@markerikson

This comment has been minimized.

Copy link
Collaborator Author

markerikson commented Oct 8, 2019

@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

This comment has been minimized.

Copy link
Collaborator Author

markerikson commented Oct 9, 2019

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

This comment has been minimized.

Copy link
Collaborator

phryneas commented Oct 9, 2019

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

This comment has been minimized.

Copy link
Collaborator Author

markerikson commented Oct 9, 2019

@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

This comment has been minimized.

Copy link
Collaborator Author

markerikson commented Oct 10, 2019

@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?

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

This comment has been minimized.

Copy link
@phryneas

phryneas Oct 10, 2019

Collaborator

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

This comment has been minimized.

Copy link
@markerikson

markerikson Oct 10, 2019

Author Collaborator

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.

This comment has been minimized.

Copy link
@phryneas

phryneas Oct 10, 2019

Collaborator

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.

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

type SliceDefinedCaseReducers<

This comment has been minimized.

Copy link
@phryneas

phryneas Oct 10, 2019

Collaborator

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

@phryneas

This comment has been minimized.

Copy link
Collaborator

phryneas commented Oct 10, 2019

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.

Copy link
Collaborator

phryneas left a comment

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 👍

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

This comment has been minimized.

Copy link
@phryneas

phryneas Oct 10, 2019

Collaborator

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.

This comment has been minimized.

Copy link
@markerikson

markerikson Oct 11, 2019

Author Collaborator

Sure, that works.


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

This comment has been minimized.

Copy link
@phryneas

phryneas Oct 10, 2019

Collaborator

something like this one.

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

This comment has been minimized.

Copy link
Collaborator Author

markerikson commented Oct 11, 2019

Awright, let's do this!

@markerikson markerikson merged commit 209bf06 into v0.8-integration Oct 11, 2019
7 checks passed
7 checks passed
Header rules - redux-starter-kit-docs No header rules processed
Details
Pages changed - redux-starter-kit-docs All files already uploaded
Details
Redirect rules - redux-starter-kit-docs No redirect rules processed
Details
Mixed content - redux-starter-kit-docs No mixed content detected
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
netlify/redux-starter-kit-docs/deploy-preview Deploy preview ready!
Details
@markerikson markerikson deleted the feature/return-slice-reducers branch Oct 11, 2019
markerikson added a commit that referenced this pull request Oct 15, 2019
* 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 markerikson mentioned this pull request Oct 15, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.