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

Add inferred action creator types #975

Closed
wants to merge 1 commit into from

Conversation

gtkatakura
Copy link

@gtkatakura gtkatakura commented Apr 2, 2021

Hey, guys. I want to share some ideas about safety infer the action types. This is just a draft of the idea because I am using the template literal types of TS 4.1 and I know have such a version dependency would be problematic for people using old versions of TS. I didn't work into caseReducers and other things, because like I said, I just want to share the idea and see if this will make sense to be part of redux-toolkit in the future or someone know some way to use template literal types and keep support for old versions of TS.

The idea is to infer the slice.actions and slice.reducer to make totally safety the usage of the type property of the action.

Screenshot from 2021-04-01 23-59-51

const slice = createSlice({
  name: 'counter',
  initialState: 0,
  reducers: {
    increment: (state: number, action) => state + action.payload,
    decrement: (state: number, action) => state - action.payload
  },
  extraReducers: {
    [firstAction.type]: (state: number, action) =>
      state + action.payload.count
  }
})

/* Reducer */

type InferredTypeActions = 'counter/increment' | 'counter/decrement'

const reducer: Reducer<number, PayloadAction<void, InferredTypeActions>> = slice.reducer

@markerikson
Copy link
Collaborator

markerikson commented Apr 2, 2021

Ooooooo.

This looks fascinating.

I know one of the weaknesses in our TS support since the beginning has been that the type of the action constants is only a string. And yeah, with TS 4.1's string manipulation, I can see how this would be doable.

@phryneas , @msutkowski , thoughts?

Per the TS 4.1 support: the plan for the upcoming RTK 1.6 release is to drop support for TS < 3.8. However, we can definitely have TS typedefs for < 4.1, and 4.1+.

Comment on lines +32 to +42
O extends GetDefaultMiddlewareOptions = {},
A extends AnyAction = AnyAction
> = O extends {
thunk: false
}
? never
: O extends { thunk: { extraArgument: infer E } }
? ThunkMiddleware<S, AnyAction, E>
? ThunkMiddleware<S, A, E>
:
| ThunkMiddleware<S, AnyAction, null> //The ThunkMiddleware with a `null` ExtraArgument is here to provide backwards-compatibility.
| ThunkMiddleware<S, AnyAction>
| ThunkMiddleware<S, A, null> //The ThunkMiddleware with a `null` ExtraArgument is here to provide backwards-compatibility.
| ThunkMiddleware<S, A>
Copy link
Member

Choose a reason for hiding this comment

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

Is there a particular reason for this change?

Copy link
Author

Choose a reason for hiding this comment

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

Yes, because the reducer has an intersection type with ThunkMiddlewareFor, so without it, the reducer will merge the action argument to AnyAction.

Copy link
Author

@gtkatakura gtkatakura Apr 2, 2021

Choose a reason for hiding this comment

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

This ThunkMiddlewareFor is passed to configureStore and is merged into reducer here:

export function configureStore<
  S = any,
  A extends Action = AnyAction,
  M extends Middlewares<S> = [ThunkMiddlewareFor<S, {}, A>]
>(options: ConfigureStoreOptions<S, A, M>): EnhancedStore<S, A, M> {


export interface EnhancedStore<
  S = any,
  A extends Action = AnyAction,
  M extends Middlewares<S> = Middlewares<S>
> extends Store<S, A> {
  /**
   * The `dispatch` method of your store, enhanced by all its middlewares.
   *
   * @inheritdoc
   */
  dispatch: DispatchForMiddlewares<M> & Dispatch<A>
}

@msutkowski
Copy link
Member

msutkowski commented Apr 2, 2021

Ooooooo.

This looks fascinating.

I know one of the weaknesses in our TS support since the beginning has been that the type of the action constants is only a string. And yeah, with TS 4.1's string manipulation, I can see how this would be doable.

@phryneas , @msutkowski , thoughts?

Per the TS 4.1 support: the plan for the upcoming RTK 1.6 release is to drop support for TS < 3.8. However, we can definitely have TS typedefs for < 4.1, and 4.1+.

Seems like a good idea to me if it can be done - I'm not sure how this can possibly work without restricting extraReducers, but there is a good chance I'm missing something. I'm sure Lenz will have feedback on the types.

@phryneas
Copy link
Member

phryneas commented Apr 2, 2021

I've feared this would come up eventually. 🤣

There are some things in here we can do (but I'm not sure if we should do them) and some thing we really shouldn't.

Let's get the "shouldn't" part out of the way:

  • The only correct use of Reducer is Reducer<StateType, AnyAction>. At runtime, any reducer will be called with every action that is ever dispatched in your application. Anything else is a lie to the compiler (and yourself, if you write tests).
    Creating a reducer that restricts it's input type is a "fake TS pattern" to enable distributed unions in reducers. We do not rely on this pattern (really, nobody should nowadays) and of course our reducers (like every other reducer) are getting called with any action. Using any other type would be wrong.

  • The same goes for action mentions in middlewares etc. The dispatch type there can take even more actions than reducers. We really shouldn't restrict that beyond AnyAction.

  • The same goes for the resulting State signature.

  • Or restricting what can be dispatched into the application. Everthing can be dispatched there. In the worst case, no reducer or middleware will react to it, but it's still perfectly valid to dispatch other stuff, maybe for logging reasons in the devtools or for future usage 🤷

And having all these out of the way, there isn't too much left to actually use these types for.

I can think of three usages:

  • Having it in Tooltips. I would love this.

  • Restricting the Dispatch type. We won't do that, but some people might choose to do that by hand. I'm not a fan of it, but if people want to do the extra work to impose a restriction that doesn't exist in the first place, that's their own fight.

  • Writing old-style switch-case reducers/middlewares/saga/observable pipes/whatever. I would hate to see this happening.
    We offer better tools for that that are actually suited for that job, namely the action.match type guard and the matching utilities

I've actually written a blog post on this whole debacle: https://phryneas.de/redux-typescript-no-discriminating-union

There also has been a lot of discussion on this topic over in the TS repo since Redux has been translated to TS: reduxjs/redux#3580

And @markerikson has recently changed the "Usage with TypeScript" section of Redux to remove mentions of the "Action Union Type" pattern: reduxjs/redux#4042

All that said: of course we could - without creating any action union types or adding tools for people to create those (they can still do it by hand if they really want to) - add this to the action creators created by createSlice.

We would need an extra typing build for TS>=4.1 but we'll have to introduce that for rtk-query anyways.

So my suggestion would be to create a new Scoped type like this: (pseudocode right now)

type ScopedActionType<Scope extends string, Type extends string> = `${Scope}/${Type}`

in a TS<4.1 build, we could just exchange that transparently with

type ScopedActionType<Scope extends string, Type extends string> = string

and use that in the CaseReducerActions type, but not change anything outside of the scope of createSlice

@gtkatakura
Copy link
Author

gtkatakura commented Apr 2, 2021

@phryneas good points. What do you think about the relay approach of generating enums? It generates a '%future added value' option into the enum to force people to handle future potential values of enum be introduced into graphql APIs. Here could be %other potential actions% or something like that. It is to force to use of switch-default/return-default/etc, isn't to use direct. Relay has eslint rule to ensure that.

@phryneas
Copy link
Member

phryneas commented Apr 2, 2021

@gtkatakura that will still give you a reducer that has an outer interface that can only be called with certain actions, but will in reality be called by any number of other actions. (See the screenshot in your initial post - that reducer has to be callable with { type: 'INIT-FOOBAR' } as well as any other valid action.

There is any number of ways to handle that, for example reduxjs/redux#4025 (comment) suggested a type like

type OtherActions<T extends AnyAction> = { type: T extends T['type'] ? T : { type: unknown } }

But all that leaves one thing untouched: That just isn't necessary with RTK. Every RTK action has a .match method that can be used as a type guard like

if (firstAction.match(action)) {
  // ...
} else if (secondAction.match(action)) {
  // ...
}

without the need of declaring any Action Union types.

Going further, that pattern can be used to use matchers that do not only use at the type, but other metadata, like

const isActionIAmLookingFor = isRejectedWithValue(asyncThunk1, asyncThunk2)

if (isActionIAmLookingFor(action)) {
}

This plays nicely with Array.prototype.filter, with the filter from rxjs/operators, with redux-saga's take and many more, without ever having to create a single Union Action Type.

So in the end, we have something that covers more use cases with less work. I really do not want to encourage people using the old Union Action Type pattern instead of this.
Union Action Types are the "TypeScript Boilerplate of Redux" and RTK's goal is to get rid of Boilerplate, not to encourage it.

With the suggested changes above anyone can still create their own boilerplate by creating a helper type like

type ActionsFromSlice<T> = T extends { actions: infer Actions } ? { [K in keyof Actions]: Actions[K] extends (...args: any[]) => infer R ? R : never }[keyof Actions] : never;

so it would still be possible - but I would not want to ship actual code to encourage that.

@pbambusek
Copy link

@phryneas I see all the arguments and I definitely agree with them, there should be no union action types needed and definitely no reducer scoping to just selected actions, however I think there is one opportunity still to consider on this proposal - it would be beneficial to have properly typed action types on actions on their own, without scoping reducers to work only with them.

For example, for slice defined like this

const slice = createSlice({
  name: 'counter',
  initialState: 0,
  reducers: {
    increment: (state: number, action: PayloadAction<number>) => state + action.payload,
    decrement: (state: number, action: PayloadAction<number>) => state - action.payload
  },
});

it would be nice to be able to have action types defined like this:

// { type: 'counter/increment', payload: number }
type IncrementAction = typeof slice.actions.increment;

// { type: 'counter/decrement', payload: number }
type DecrementAction = typeof slice.actions.decrement;

// Still not scoped
// Reducer<number, AnyAction>
type SliceReducer = typeof slice.reducer;

What do you think?

@phryneas
Copy link
Member

phryneas commented Jun 5, 2021

@pbambusek We will consider this for the next release, but for this release, we just have too much stuff already happening with RTK-Query - and this is actually more work than it seems since we would have to adjust the build again to support both TS>4.1 and below.

Even though, honestly, I still do not see any reason to ever reference one of those types in code. Hovering them in the IDE for more information, sure. But what do you actually want to do with that?

@phryneas phryneas added this to the 1.7 milestone Jun 5, 2021
@phryneas phryneas modified the milestones: 1.7, 1.8 Oct 1, 2021
@phryneas
Copy link
Member

phryneas commented Jul 3, 2022

Closing in favor of #2250 that adds the literal types for actions, but does not do any more than that.

@phryneas phryneas closed this Jul 3, 2022
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.

5 participants