createSlice reducer types

[ceuk]

Hi,

I've been using a utility type like this for a while:

import { CaseReducer, PayloadAction } from '@reduxjs/toolkit'

export type MapReducerPayloads<State, ReducerPayloadMap> = {
  [K in keyof ReducerPayloadMap]: CaseReducer<State, PayloadAction<ReducerPayloadMap[K]>>
}

Which allows you to succinctly create a CaseReducers type like so:

interface RoomState {
  id: string | null;
  peer?: Peer;
  owner?: boolean;
  connections: string[];
}

type Reducers = MapReducerPayloads<RoomState, {
  createRoom: void;
  joinRoom: {
    roomId: string;
    owner: boolean;
  };
}>

const initialState: RoomState = {
  id: null,
  connections: []
}

export const roomSlice = createSlice<RoomState, Reducers>({
  name: 'room',
  initialState,
  reducers: {
    createRoom: (state) => {},
    joinRoom: (state, { payload: { roomId, owner } }) => {
      state.id = roomId
      state.peer = new Peer(roomId)
      state.owner = owner
    }
  }
})

I really like this approach as you get the types flowing through into your payload objects inside your reducers functions.

Just wanted to ask:

A) Is there a similar, built-in way of accomplishing the same? (without defining the reducers outside of the createSlice fn and letting the types get inferred - I find this less readable too)

B) Is this a pattern the maintainers would be interested in incorporating into the lib? Or is it too subjective/niche/complex etc?

Cheers

[markerikson]

Hmm. I'm not sure I see much of a point in this, tbh.

If you really don't want to define your case reducers inline inside of createSlice, you'd write them as:

function todoToggled(state: TodosState, action: PayloadAction<string>) {
  // etc
}

but this really just ends up with you writing the same code you would have written inside of createSlice, but with a couple more keywords (function and : TodosState). So to me, there's no actual benefit to writing them outside.

I'm honestly kind of confused what the intended improvement is in the snippet you're showing. It doesn't look like it's saving any lines of code or characters vs how we normally recommend things.

(Also, as a side note: state.peer = new Peer(roomId) looks like you're putting a class instance into Redux state, which is specifically something you should avoid: https://redux.js.org/style-guide/#do-not-put-non-serializable-values-in-state-or-actions )

[ceuk]

I just think separate/out-of-band type annotations make everything much more readable, but I guess it's a really subjective thing ¯\_(ツ)_/¯

Thanks for the response (and thanks for the catch with the anti-pattern :) I was in the middle of converting a plain old object into a slice when I had the thought about this issue)

[phryneas]

If you really want to, you can call it with reducers: { ... } as Reducers, but you should never call createSlice (or configureStore or most other RTK functions unless explicitly stated in the docs) with hand-written generic arguments.

Not only does it probably already erase important type info at this point (like in your example, the slice name which will get more use in #2250), it also disables inference for any additional generic arguments we might add in the future - and since we expect nobody to use those, there might be all kinds of breakage.

But I have to agree with Mark, I don't think that separating definition so far from usage here does you any good - it just makes it more difficult to see what is actually happening.

[ceuk]

you should never call createSlice (or configureStore or most other RTK functions unless explicitly stated in the docs) with hand-written generic arguments.

I think it's fairer to say you shouldn't call the functions with incorrect or incomplete arguments right? (which is a bit of a truism).

IMO generic args are part of the public API that you the library exposes and just like the rest of the public interface (e.g. the runtime args passed to the functions themselves) incomplete, incompatible or incorrect inputs will obviously cause problems.

However, in this case, the args are more-or-less correct as far as I can tell (at least for now), in which case I guess the point is that future upgrades could introduce breaking changes, which again I would argue is no different to anything else and it's fairly normal to expect that upgrading a dependency could require refactoring your code slightly.

From your POV I can definitely see how it might be worrying to have people overriding default args that you didn't anticipate they would. But I doubt I'm the only one, unfortunately. It might even be worth having an explicit note in the docs themselves saying that certain generic values are considered internal and by manually setting them you're more tightly coupling your implementation to the current version, making it more liable to break for even minor future updates.

I don't think that separating definition so far from usage here does you any good - it just makes it more difficult to see what is actually happening.

I don't feel like there's a huge difference between seeing something like:

(state: State, action: PayloadAction<number>) => {}

And getting the same info in an IDE popup without the persistent visual noise:

It's fascinating because it just feels so "right" to me that it never even occurred to me that (seemingly most) people would prefer the inverse. I suspect I'm probably the abnormal one in this case :)

[phryneas]

I think it's fairer to say you shouldn't call the functions with incorrect or incomplete arguments right? (which is a bit of a truism).

IMO generic args are part of the public API that you the library exposes and just like the rest of the public interface (e.g. the runtime args passed to the functions themselves) incomplete, incompatible or incorrect inputs will obviously cause problems.

No, you should never call it with generic arguments. I mean it very much like that. None of the TypeScript examples in the documentation ever show it. It is not documented and thus not guaranteed that we won't completely change the internals in the next minor version and break everything if you use it in an undocumented way.

You are interpreting something into the public api that is not documented, and most of these generic arguments would require to be called with a 1:1 copy of the runtime code typified to even remotely stand a chance of being correct. They are designed for inference, not for manual specification.

This is not about "defaults", it is about "inferred value". TypeScript has two different modes when it comes to generics:

• infer all of them
• specify at least one, use the "default values" (which usually are very useless) and infer nothing.

A mix of inferred and specified usage in TS is not possible - specifying something here will have everything that is added in the future (and might rely on inference) fall back to the default and thus potentially break it.

As I said, you can use your way by casting the reducers argument (as that would still help with inference on the method call itself), but manually specifying generics on an api that is meant for inferred consumption is not the way it is meant to be used and we cannot support it in any way.

[ceuk]

I understand all that, my point was there's little way to know that you consider them "for inference only" from the docs.

I don't think expecting people to infer this must be the case due to their omission from the docs is a very reliable strategy as - whilst the RTK docs are superb - omission in other docs is usually just due to poor/incomplete/out of date docs rather than something intentional.

I was saying if I find something I can configure/provide etc, I expect that this is okay unless I'm explicitly told I shouldn't. It's explicitly documented on this issue now I guess, I was just suggesting it might be worth calling it out more explicitly in the docs too

P.S I like your suggestion with the casting of the casereducers object instead, hope I'm not coming across as disagreeable :)

[phryneas]

Well, to be fair we do say so in the docs ;)

Here for example: https://redux-toolkit.js.org/usage/usage-with-typescript#defining-the-initial-state-type

[ceuk]

Ah, I probably should have checked that first, I humbly withdraw my suggestion 😄