Allow mocha callbacks to use any subclass of Context

[joeyparrish]

When creating fixtures in Mocha, they get attached to Context, which includes an index signature for this purpose. However, those fixtures will have no type information.

If a project wants to strongly type its fixtures, it needs to subclass Context. But then, without this patch, the compiler will reject the use of those subclasses in Mocha callbacks.

---

Update: a better explanation of the change, to aid review, since this touches 100+ lines.

Context's index signature prevents strong typing for Mocha fixtures. To address this is a backward-compatible way, Context was split into BaseContext, which contains all the strongly typed fields, and Context, which extends it with the index signature. Projects that add custom Mocha fixtures and want them strongly-typed can now define those fixtures on an interface extending BaseContext.

Callback functions Func and AsyncFunc had this: Context, but now are generics with this: T and T extends BaseContext = Context.

Leaving out the default T = Context breaks compilation, with errors like Generic type 'AsyncFunc' requires 1 type argument(s). But including it also leads to the assumption that we always pass the index-signature-bearing Context instead of any BaseContext subclass.

Changing the default to T = BaseContext breaks compatibility with projects that make use of the index-signature I want to avoid in my own project. So that is not an option.

The solution is for each usage of Func and AsyncFunc in Mocha's API, to add a generic overload of the method. If I pass a callback that uses strongly-typed fixtures in BaseContext subclass, the compiler can infer the generic type and properly check my usage of those fixtures.

For example, where we had this:

            before(fn?: Mocha.Func | Mocha.AsyncFunc): void;

we add this:

            before<T extends Mocha.BaseContext>(fn?: Mocha.Func<T> | Mocha.AsyncFunc<T>): void;

Each instance of this fails the linter check @definitelytyped/no-unnecessary-generics with Type parameter T is used only once. The generic doesn't influence the return type, so this rule would steer us toward using any instead. But this is clearly necessary to use the strongly-typed this we need to pass. So each instance is preceded by // eslint-disable-next-line @definitelytyped/no-unnecessary-generics.

If I delete the default version of those methods and only provide a generic method, existing tests break because the compiler assumes the base class BaseContext by default, instead of the index-signature-bearing Context. So the overload is necessary.

---

Please fill in this template.

• Use a meaningful title for the pull request. Include the name of the package modified.
• Test the change in your own code. (Compile and run.)
• Add or edit tests to reflect the change.
• Follow the advice from the readme.
• Avoid common mistakes.
• Run pnpm test <package to test>.

Select one of these and delete the others:

If changing an existing definition:

• Provide a URL to documentation or source code which provides context for the suggested changes: https://mochajs.org/#global-fixtures
• If this PR brings the type definitions up to date with a new version of the JS library, update the version number in the package.json.

[typescript-bot]

@joeyparrish Thank you for submitting this PR!

This is a live comment which I will keep updated.

1 package in this PR

• mocha — on npm, on unpkg

Code Reviews

Because you edited one package and updated the tests (👏), I can help you merge this PR once someone else signs off on it.

You can test the changes of this PR in the Playground.

Status

• ✅ No merge conflicts
• ✅ Continuous integration tests have passed
• 🕐 Most recent commit is approved by type definition owners or DT maintainers

Once every item on this list is checked, I'll ask you for permission to merge and publish the changes.

Inactive

This PR has been inactive for 43 days — it is still unreviewed!

---

Diagnostic Information: What the bot saw about this PR

{
  "type": "info",
  "now": "-",
  "pr_number": 69255,
  "author": "joeyparrish",
  "headCommitOid": "11442cd7cd7fa572b1f0d16ea09a0d4742827356",
  "mergeBaseOid": "2554f0f86fba017b07b4c5175e05fed6bafdd452",
  "lastPushDate": "2024-04-02T19:56:23.000Z",
  "lastActivityDate": "2024-05-06T17:24:36.000Z",
  "maintainerBlessed": "Waiting for Code Reviews",
  "hasMergeConflict": false,
  "isFirstContribution": false,
  "tooManyFiles": false,
  "hugeChange": false,
  "popularityLevel": "Critical",
  "pkgInfo": [
    {
      "name": "mocha",
      "kind": "edit",
      "files": [
        {
          "path": "types/mocha/index.d.ts",
          "kind": "definition"
        },
        {
          "path": "types/mocha/mocha-tests.ts",
          "kind": "test"
        }
      ],
      "owners": [
        "kazimanzurrashid",
        "otiai10",
        "enlight",
        "cspotcode",
        "1999",
        "strangedev",
        "nicojs"
      ],
      "addedOwners": [],
      "deletedOwners": [],
      "popularityLevel": "Critical"
    }
  ],
  "reviews": [],
  "mainBotCommentID": 2032992096,
  "ciResult": "pass"
}

[typescript-bot]

🔔 @kazimanzurrashid @otiai10 @enlight @cspotcode @1999 @strangedev @nicojs — please review this PR in the next few days. Be sure to explicitly select Approve or Request Changes in the GitHub UI so I know what's going on.

[typescript-bot]

@joeyparrish The CI build failed! Please review the logs for more information.

Once you've pushed the fixes, the build will automatically re-run. Thanks!

Note: builds which are failing do not end up on the list of PRs for the DT maintainers to review.

[joeyparrish]

Everything is passing and I'm looking forward to your feedback!

[typescript-bot]

Re-ping @kazimanzurrashid, @otiai10, @enlight, @cspotcode, @1999, @strangedev, @nicojs:

This PR has been out for over a week, yet I haven't seen any reviews.

Could someone please give it some attention? Thanks!

[typescript-bot]

It has been more than two weeks and this PR still has no reviews.

I'll bump it to the DT maintainer queue. Thank you for your patience, @joeyparrish.

(Ping @kazimanzurrashid, @otiai10, @enlight, @cspotcode, @1999, @strangedev, @nicojs.)

[joeyparrish]

I know this is 100+ lines changed, but there are really only 2-3 things happening, one of them repeated many, many times. To aid review, I updated the PR description with a better explanation of the change. It details the things that changed, why these are necessary changes, and why the simpler changes I tried don't work. I hope this helps!

[rbuckton]

Would <T extends BaseContext = Context>(fn: Func<T>): void; allow you to remove the non-generic version?

[joeyparrish]

No, because then every time I invoke the function, I would have to call it with an explicit subtype, or the compiler would assume Context. This way, the compiler infers the type. It's ugly, I know.

[jakebailey]

Does this class actually exist at runtime? This will be exported so implies it can be new'd.

Also, the comment above is attached to the wrong declaration.