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

Type action and next as unknown #4519

Merged
merged 3 commits into from
Apr 16, 2023

Conversation

EskiMojo14
Copy link
Contributor

@EskiMojo14 EskiMojo14 commented Apr 11, 2023


name: 🐛 Bug fix or new feature
about: Fixing a problem with Redux

PR Type

Does this PR add a new feature, or fix a bug?

It makes types safer for middleware.

Why should this PR be included?

Currently, the next parameter is typed as the D type parameter passed, and action is typed as theAction extracted from the dispatch type. Neither of these are a safe assumption:

  • next would be typed to have all of the dispatch extensions, including the ones earlier in the chain that would no longer apply.
    • Technically it would be mostly safe to type next as the default Dispatch implemented by the base redux store, however this would cause next(action) to error (as we cannot promise action is actually an Action) - and it wouldn't account for any following middlewares that return anything other than the action they're given when they see a specific action.
  • action is not necessarily a known action, it can be literally anything - for example a thunk would be a function with no .type property (so AnyAction would be inaccurate)

This PR changes next to be (action: unknown) => unknown (which is accurate, we have no idea what next expects or will return), and changes the action parameter to be unknown (which as above, is accurate)

Checklist

  • Have you added an explanation of what your changes do and why you'd like us to include them?
  • Is there an existing issue for this PR?
  • Have the files been linted and formatted?
  • Have the docs been updated to match the changes in the PR? (TODO)
  • Have the tests been updated to match the changes in the PR?
  • Have you run the tests locally to confirm they pass?

Bug Fixes

What is the current behavior, and the steps to reproduce the issue?

As stated above, the types used for next and action are inaccurate at best, and dangerous at worst.

What is the expected behavior?

Middleware should be forced to check what the action is, before using it.

How does this PR fix the problem?

Types next and action in a way that would force the middleware to type check before using them (return next(action) still works without issue)

Note: this is a breaking change for middleware typed to expect a certain action or dispatch for next. For example:

// @ts-expect-error wrong next
const customMiddleware: Middleware = (api) => (next: Dispatch) => (action) => next(action);

// @ts-expect-error wrong action
const promiseMiddleware: Middleware<{
  (promise: Promise<any>): Promise<void>
}> = (api) => (next) => (action: Promise) => {
  if (action instanceof Promise) {
    return promise.then(console.log);
  }
  return next(action);
}

However, I would argue this is a good change - as explained above, this is inherently a lie to the compiler, and is thus unsafe.

Similar to caught errors, any middleware that (unsafely) wishes to opt out and treat action as any can still do so:

const customMiddleware: Middleware = (api) => (next) => (action: any) => next(action);

@codesandbox-ci
Copy link

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 0693e5e:

Sandbox Source
Vanilla Configuration
Vanilla Typescript Configuration

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.

Middleware action is typed as any
2 participants