feat(expect): allow extending matcher interfaces by moving expect types to @jest/types package

[mrazauskas]

Summary

The idea is simple – move expect types to @jest/types.

Fixed:

• import Expect types instead of copy (jest-jasmine2, jest-snapshot);
• add expect type to TestFrameworkGlobals;
• add snapshotState type to MatcherState through interface merging (jest-snapshot);
• separate matcher interfaces: Matchers and SnapshotMatchers;
• separate types: Expect and JestExpect (only the latter one includes addSnapshotSerializer and SnapshotMatchers).

All this makes Expect type more precise for expect and jest users.

Feature:

Also it becomes possible to extend AsymmetricMatchers and Matchers interfaces:

declare module '@jest/types' {
  namespace Expect {
    interface AsymmetricMatchers {
      toBeWithinRange(floor: number, ceiling: number): AsymmetricMatcher;
    }
    interface Matchers<R> {
      toBeWithinRange(floor: number, ceiling: number): R;
    }
  }
}

Alternatively it might be possible to extend the interfaces from @jest/globals. I was trying it out. Additional exports has to be added to @jest/globals. The same interfaces would be exported from @jest/globals and @jest/types. It means that the same result could be achieved in two different ways. That’s not the best. Also matchers are not a globals, they do not belong there.

Keeping interfaces at @jest/types simplifies usage. The same pattern works equally good for jest and for expect users: import expect from expect, extend matchers from expect.extend, extend types from @jest/types. The @jest/types package is anyway a dependency of expect. Easy to explain, easy to use. For me this looks like an optimal solution.

Why types should be moved at all?

I was trying to expose types from expect package (breaking for now, but might be a solution for the future). Unfortunately this approach creates circular dependencies if we import expect types to @jest/types. The expect typings are needed for TestFrameworkGlobals and from here Circus and Jasmin are loading them. So moving types seemed to be necessary.

Actually the idea of moving expect types came to my mind while looking through code of jest-circus. The typings of jest-circus live in @jest/types. This fact made me to think that moving expect types could solve many issues.

Problem (and possible Solution)

Unfortunately, there is one problem. this.utils, which is part of MatcherState interface, depends on jest-matcher-utils. Importing jest-matcher-utils to @jest/types creates circular dependencies. Here is what I tried:

One. Write manual typings for this.utils. Why not? Some of the utils depend on another Jest packages and these create circular dependencies again or enormous amount of manual typings. That would be hard to maintain.

Two. Use interface merging to patch MatcherState. This is implemented right now. And this is the dirt this PR is trying to bring into code base. Could be acceptable, but only for short term.

Long term solution: to move all expect utils (including print helpers, iterableEquality, subsetEquality, and also isEqual) from this.utils to jest-matcher-utils package and to import them from there instead of this.utils. Remove this.utils and the interface merging patch in Jest 28.

This solution would help to get rid of "./build/utils.js" export from expect. Would resolve #11867 and #11816. By the way, this.utils example in Jest’s documentation is suggesting that toBe matcher is calling this.utils.matcherHint(). In reality matcherHint() helper is imported from jest-matcher-utils. Seems like this is better pattern also for the userland.

If this long term change seem acceptable, then the temporary interface merging patch is fine. (Just to add, I would be glad to help moving the utils.)

This PR might look huge, but it is not. I had to make adjustments in every place where Expect types are consumed and to remove previous workarounds. Actual change is rather minimal. Will add few comments.

Test plan

An example which demonstrates how interface merging for AsymmetricMatchers and Matchers works is added to examples folder.

I extended typechecks (or type tests) too. Had to move them from root into packages so that types of expect package could be checked in isolation. The test makes sure that types added by Jest (snapshotState, addSnapshotSerializer and SnapshotMatchers) do not leak to expect.

Moving type test files into the packages also allowed to simply the setup of typechecks.

[mrazauskas]

Is it breaking to remove this line? Could also be left in place with // @ts-expect-error.

[SimenB]

breaking, yes

[mrazauskas]

Interface merging or casting? My thinking was: if it is possible without merging, then better to avoid another declare module. Merging is less explicit. Usually hiding in another file. Felt like code is more readable with type casting.

[SimenB]

agreed, if this can also work out for consumers outside of this module 🙂

[codecov-commenter]

Codecov Report

Merging #12059 (a391c8f) into main (3093c18) will decrease coverage by 0.22%.
The diff coverage is 35.65%.

@@            Coverage Diff             @@
##             main   #12059      +/-   ##
==========================================
- Coverage   67.71%   67.49%   -0.23%     
==========================================
  Files         328      329       +1     
  Lines       16990    17043      +53     
  Branches     4817     4818       +1     
==========================================
- Hits        11505    11503       -2     
- Misses       5452     5507      +55     
  Partials       33       33              

Impacted Files	Coverage Δ
packages/expect/__typechecks__/expect.test.ts	0.00% <0.00%> (ø)
...ages/expect/src/extractExpectedAssertionsErrors.ts	10.52% <0.00%> (ø)
packages/expect/src/jasmineUtils.ts	96.03% <ø> (ø)
...us/src/legacy-code-todo-rewrite/jestAdapterInit.ts	0.00% <0.00%> (ø)
...-circus/src/legacy-code-todo-rewrite/jestExpect.ts	0.00% <0.00%> (ø)
packages/jest-circus/src/types.ts	100.00% <ø> (ø)
packages/jest-globals/src/index.ts	100.00% <ø> (ø)
packages/jest-jasmine2/src/jestExpect.ts	0.00% <0.00%> (ø)
packages/jest-jasmine2/src/setup_jest_globals.ts	0.00% <ø> (ø)
packages/jest-types/__typechecks__/expect.test.ts	0.00% <0.00%> (ø)
... and 7 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3093c18...a391c8f. Read the comment docs.

[mrazauskas]

Should not be optional, because this prop will be added to each matcher. At the same time, in code it works only as optional prop. Hm.. Feels like it does not belong to this type.

[SimenB]

Thanks for taking the time to put this together! My main concern is - ideally people who install just expect would be able to add extra matchers to it without worrying about @jest/types - can we keep the types in expect instead of moving? I don't fully follow why it has to move

[mrazauskas]

@SimenB Thanks for taking a look. I was spending some time to dig deeper into types of all packages.

My aim was to find a solution for this problem: How Jest’s own types could become a single source of truth and could be used instead of @types/jest in the future?

A draft branch with full solution is almost done (only one final touch is missing). I will open an issue to show the whole picture with link to that branch. It is hardly possible to land it as a single PR, but this could become a milestone for Jest 28.

This PR is part of that draft. The proposed changes make much more sense looking at larger picture. I think it has to wait for Jest 28 and to land without adding any workarounds.

[SimenB]

Would you mind rebasing this?

Also, I think possibly moving matchers to a separate module from expect might make sense, not sure?

[mrazauskas]

Managed to rebase it. Something goes wrong with module augmentation. Works on IDE, but fails to build. Most probably will fail here too. Thinking about it.

[mrazauskas]

Not happy about this fix. Type tests will fail, because of the same module augmentation problem. Have to dig deep into this.

@SimenB What you think about removing equals and utils from MatcherState interface and the this inside matchers? I don’t like these two extra imports inside @jest/globals. If this sounds acceptable, perhaps equals and utils could be deprecated gradually? For Jest 28 they could be typed as any with @deprecated, but the actual implementation could be removed only later in Jest 29. Hope it is clear what I try to say here (;

[SimenB]

We need to keep this working in matchers. And this should include utils and equals. Whether that's augmented at some later point or added from the get go shouldn't matter. But we want custom matchers to not depend on e.g. jest-matchers-utils as those always gets outdated, and rather get stuff from this.

[mrazauskas]

Ah.. Got it. That makes sense.

[mrazauskas]

Closing in favour of #12404

[github-actions]

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.
Please note this issue tracker is not a help forum. We recommend using StackOverflow or our discord channel for questions.