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

Middleware action is typed as any #4518

Closed
ethanneff opened this issue Apr 11, 2023 · 12 comments · Fixed by #4519
Closed

Middleware action is typed as any #4518

ethanneff opened this issue Apr 11, 2023 · 12 comments · Fixed by #4519
Milestone

Comments

@ethanneff
Copy link

ethanneff commented Apr 11, 2023

The documentation is missing typing on action. https://redux.js.org/usage/usage-with-typescript#type-checking-middleware

export type AppStore = typeof store;
export type AppState = ReturnType<typeof store.getState>;
export type AppDispatch = typeof store.dispatch;
export type AppMiddleware = Middleware<void, AppState, AppDispatch>;

export const loggerMiddleware: AppMiddleware =
  (storeApi) => (next) => (action) => {
    const state = storeApi.getState();

    console.log(action.type, state); // Unsafe member access .type on an `any` value.

    return next(action); // Unsafe return of an `any` typed value.
  };
@markerikson
Copy link
Contributor

That's because as far as the middleware is concerned, that action literally could be anything. It could be an action object, it could be a thunk function, it could be a promise... anything at all:

export interface Middleware<
  DispatchExt = {},
  S = any,
  D extends Dispatch = Dispatch
> {
  (api: MiddlewareAPI<D, S>): (
    next: Dispatch<AnyAction>
  ) => (action: any) => any
}

It's up to you to do appropriate type guard checks if you need to narrow it down.

@EskiMojo14
Copy link
Contributor

if you want this to be more explicit, you can force action to be unknown:

export const loggerMiddleware: AppMiddleware =
  (storeApi) => (next) => (action: unknown) => {

this would force you to check what the action is before you can do anything with it.

@ethanneff
Copy link
Author

Is there a way to do this without using any or unknown for action:?

For example, is there a known way to pull the action type for all actions?

const actions = {
  ...deviceSlice.actions,
  ...authSlice.actions,
};

type AppAction = typeof actions;

@EskiMojo14
Copy link
Contributor

by doing so, you would be lying to Typescript: Do not create union types with Redux Action Types. It's most likely an antipattern.

if you want to check whether an action is a known one, you can use the action creator's match method and/or RTK's matching utilities.

the simple fact is that you do not know what is being dispatched - in a middleware, it could be anything (thunks for example are functions and would not have a .type property at all)

@markerikson markerikson transferred this issue from reduxjs/redux-toolkit Apr 11, 2023
@markerikson markerikson added this to the 5.0 milestone Apr 11, 2023
@markerikson markerikson changed the title How do you type middleware? Middleware action is typed as any Apr 11, 2023
@ethanneff
Copy link
Author

For context, I was trying to migrate an existing redux library to toolkit. In my existing codebase, I had full type-safety around actions, dispatches, and middleware.

Screen Shot 2023-04-11 at 9 07 34 AM

Screen Shot 2023-04-11 at 9 02 48 AM

But this relied on a RootAction, which from the article is an anti-pattern.

I guess my next best bet is to type it as unknown and guard check it.

@markerikson
Copy link
Contributor

Yep. Type action as unknown here in the middleware.

Per that article, we really don't find it useful to limit what can be passed into dispatch at the TS level.

@EskiMojo14
Copy link
Contributor

EskiMojo14 commented Apr 11, 2023

yeah, so for example your reduxWhitelist check could instead look like:

export type AppMiddleware<DispatchExt = {}> = Middleware<DispatchExt, AppState, AppDispatch>;

const isWhitelistedAction = isAnyOf(chatCreated, chatDeleted);

export const syncMiddleware: AppMiddleware = (mwApi) => (next) => (action: unknown) => {
  if (isWhitelistedAction(action)) {
     // inside this block, action is now a union of all the actions whitelisted
     syncQueue.set([action.type]);
  }
  return next(action);
}

@guillaumebrunerie
Copy link

I would also very much like it to default to unknown, which is (in my opinion) the correct way to say that it can be anything at all.
Generally speaking, I would love that all instances of any be replaced by unknown. Having bugs that would have been prevented by Typescript just because Redux silently introduced some any type somewhere is one of the few places where I find the typing suboptimal. For instance the Reducer type defaults with any, and there are various other places where the AnyAction type is used.

@Methuselah96
Copy link
Member

Methuselah96 commented Apr 15, 2023

I am also interested in defaulting those to unknown for v5 and have poked at that a little bit. It should be possible to make state default to unknown, but actions are a bit more tricky since they need to be covariant in some places and contravariant in other places. Using AnyAction as the default action sweeps those issues under the rug (for better or worse).

@Methuselah96
Copy link
Member

Although after discussing it some more, I think they're always contravariant, but it will still take more work to avoid AnyAction for the default type.

@guillaumebrunerie
Copy link

I experimented a bit with replacing any with unknown in AnyAction and middlewares, and managed to do it with only fairly minimal changes (I only tested with the test:types script, not on any real codebase), see here: guillaumebrunerie@435fd41
The two other changes I needed to do are

  • ReducersMapObjects, ActionFromReducersMapObjects and the third overload of combineReducers take an additional S type argument. I am not quite sure why this is needed.
  • Action has been changed from an interface to a type. I don’t know if that would be a breaking change, and I’m also not sure why this is needed, but at least it works.

When it comes to the tests, it broke two tests:

  • one was doing state + action.count without even checking that action.count is a number. It used to work because action.count was typed as any, but it doesn’t work anymore when it is unknown (as expected)
  • another one also required changing an interface to a type, otherwise some type guard behaves differently somehow.

@EskiMojo14
Copy link
Contributor

EskiMojo14 commented Apr 16, 2023

yeah, you're right - i was running into a bunch of headaches with Action not being assignable to UnknownAction, but changing Action to a type fixes it

very odd

(technically this would be a breaking change in that you couldn't use declaration merging any more, but I don't think we'd want anyone merging the basic Action type anyway)

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 a pull request may close this issue.

5 participants