fix(types): getState is not bound to MiddlewareAPI

[janeklb]

PR Type

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

Fixes a type bug

Why should this PR be included?

The getState function provided by MiddlewareAPI is not bound to any object, and as such it can be used freely as in the examples.

The MiddlewareAPI type does not make this explicit, and typescript linters can report this as problematic usage.

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?
  • link issue here
• Have the files been linted and formatted?
  • there's a file that needs formatting, but is outside the scope of my change: docs/usage/deriving-data-selectors.md
• Have the docs been updated to match the changes in the PR?
  • No updates needed IMO
• Have the tests been updated to match the changes in the PR?
  • No new tests added - the linting rules required to
• Have you run the tests locally to confirm they pass?

Bug Fixes

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

• Enable the @typescript-eslint/unbound-method linting rule
• Load up one of the examples

  const middleware: Middleware<unknown, unknown> =
    ({ getState }) =>
    (next) =>
    (action) => {
      // ...
    };

• The linter will report that the rule is being violated

What is the expected behavior?

getState can be used with the linting rule enabled and not produce errors

How does this PR fix the problem?

Fixes the signature of getState

[codesandbox-ci]

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.

[timdorr]

This should have some sort of test to go along with it. That will prevent a regression in the future.

[janeklb]

This should have some sort of test to go along with it. That will prevent a regression in the future.

I'm able to create a test if we revert to

export interface MiddlewareAPI<D extends Dispatch = Dispatch, S = any> {
  dispatch: D
  getState(this: void): S
}

however, with the current signature (getState: () => S), I can't find a way to do it. @EskiMojo14 can you advise?

[janeklb]

This should have some sort of test to go along with it. That will prevent a regression in the future.

I'm able to create a test if we revert to

export interface MiddlewareAPI<D extends Dispatch = Dispatch, S = any> {
  dispatch: D
  getState(this: void): S
}

however, with the current signature (getState: () => S), I can't find a way to do it. @EskiMojo14 can you advise?

Never mind -- found something that covers the bases, I think.

[EskiMojo14]

this doesn't actually fail if the signature is changed back

if we really need a test, then we can switch back to the this: void method, but i'm not convinced that we should worry too much about testing a change solely to satisfy a lint rule, and I think this: void is unnecessary noise for a codebase that doesn't actually use the lint rule it's meant to satisfy

[janeklb]

I swear I tested this.. thanks for double checking it.

[janeklb]

So, to proceed: remove the test and keep the current signature?

[EskiMojo14]

that would be my preference - @timdorr?

[timdorr]

I would add the test as one commit, which should fail, and then add the fix in the next commit to show that this satisfies the condition we're looking to correct.

[timdorr]

Oh, I see what you're saying. Sorry, I was thinking about the actual test passing.

As long as the test essentially functions as a lint check, I think that's sufficient. Can we get a spot check by importing this version into a project and seeing if that does pass the lint (vs the current release failing)?

[janeklb]

I've tested it out in my project and it fixes the lint violation.

Should I remove the test?

[timdorr]

That seems good enough to me. Leaving the test seems fine, since it at least validates the behavior and will trip if someone breaks this again later on.

[EskiMojo14]

the test doesn't actually verify anything, and will not break if anything is changed.

[phryneas]

It's an eslint rule fix. Even if this breaks, it will not cause any bugs in any production environment but only give someone red squigglies. I don't think it needs a test honestly.

[timdorr]

Thanks!