feat(expect): replace `RawMatcherFn` with `MatcherFunction` and `MatcherFunctionWithState` types #12376

mrazauskas · 2022-02-12T14:07:19Z

Summary

It may be not the best idea to expose RawMatcherFn from expect. That was my initiative (see #12363), so fixing it. Easier to explain with example:

https://github.com/facebook/jest/blob/faef0b4b7082df574a0e4423b86d468847360f17/packages/expect/src/types.ts#L23-L24

const toBeWithinRange: RawMatcherFn = (actual: number, floor: number, ceiling: number) => {
  // implementation
};

In the example above RawMatcherFn will simply turn all three number types into any. Ups.. Found it while working on #12363 (comment). Everything what I can infer with expect.extend({toBeWithinRange}) is floor: any, ceiling: any.

It felt like RawMatcherFn should give some type safety here. For example, the number of arguments? Let’s check. Hardly possible to imagine any implementation without actual (or received). The rest of arguments are optional (for instance .toBeNull()). No use of actual: any, expected?: any, options?: any and it is not good idea to any types of all args.

What about return type? All we need is ExpectationResult, right? Like this:

const toBeWithinRange = (actual: number, floor: number, ceiling: number): ExpectationResult => {
  // implementation
};

This way one can ensure that { pass: boolean; message(): string; } will be returned as required. That’s good. Also arguments are not any. That’s also good. And it seems that ExpectationResult is all we need internally too. Or?

Edit: One more detail. RawMatcherFn also has this type. That’s important, of course. Just checked, it all works:

function toHaveContext(this: MatcherState, received: string): ExpectationResult {
  // implementation
};

Test plan

Green CI.

mrazauskas · 2022-02-12T14:09:03Z

packages/jest-jasmine2/src/jestExpect.ts

@@ -41,23 +41,15 @@ export default function jestExpect(config: {expand: boolean}): void {
      jestMatchersObject[name] = function (
        this: MatcherState,
        ...args: Array<unknown>
-      ): RawMatcherFn {


This function has inferred return type of ExpectationResult without this extra type.

mrazauskas · 2022-02-12T14:13:00Z

packages/jest-jasmine2/src/types.ts

+  compare(...args: Array<unknown>): ExpectationResult;
+  negativeCompare(...args: Array<unknown>): ExpectationResult;


Puzzle. To be precise this type should be () => (...args: Array<unknown>) => ExpectationResult, but my version eliminates these @ts-expect-error: https://github.com/facebook/jest/blob/faef0b4b7082df574a0e4423b86d468847360f17/packages/jest-jasmine2/src/jestExpect.ts#L50-L60

codecov-commenter · 2022-02-12T14:35:06Z

Codecov Report

Merging #12376 (b16b3a4) into main (faef0b4) will increase coverage by 0.00%.
The diff coverage is 73.68%.

@@           Coverage Diff           @@
##             main   #12376   +/-   ##
=======================================
  Coverage   68.47%   68.47%           
=======================================
  Files         324      324           
  Lines       16967    16968    +1     
  Branches     5060     5060           
=======================================
+ Hits        11618    11619    +1     
  Misses       5317     5317           
  Partials       32       32

Impacted Files	Coverage Δ
packages/expect/src/index.ts	`91.27% <ø> (ø)`
packages/jest-jasmine2/src/jestExpect.ts	`0.00% <0.00%> (ø)`
packages/jest-snapshot/src/index.ts	`78.75% <87.50%> (+0.13%)`	⬆️

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 faef0b4...b16b3a4. Read the comment docs.

packages/expect/__typetests__/expect.test.ts

CHANGELOG.md

examples/expect-extend/toBeWithinRange.ts

packages/expect/src/types.ts

examples/expect-extend/toBeWithinRange.ts

packages/jest-snapshot/src/index.ts

SimenB · 2022-02-15T17:13:55Z

@mrazauskas can we land this now, you think? Just so the types are closer to what we want than #12363. I wanna make a new alpha release without that in it 🙂 We can still revisit of course, but not publishing stuff we know will be yanked makes sense

mrazauskas · 2022-02-15T17:47:54Z

@SimenB Sure. Just let me take one last look.

The thing is that I got stuck trying to use the new MatcherFunction internally. Some changes are needed and I was preparing few PRs to make that possible. But we can land this now with todo comment and iterate to fix it all.

By the way, should ExpectationResult be exposed or not? Hm.. I am 50/50 about it. What you think?

SimenB · 2022-02-15T17:49:40Z

The thing is that I got stuck trying to use the new MatcherFunction internally. Some changes are needed and I was preparing few PRs to make that possible. But we can land this now with todo comment and iterate to fix it all.

Exciting stuff! II really hope these awesome changes you're working on will let us use @jest/globals directly in @types/jest, that way custom matchers can all depend on @jest/globals instead.

By the way, should ExpectationResult be exposed or not? Hm.. I am 50/50 about it. What you think?

Hmm, let's keep it out for now, and if there's a use case we can expose it? Always easier to add than remove stuff 😀

Semi-related - thoughts about a @jest/expect which adds the snapshot matchers? That way people can use expect as a "generic" assertion module, @jest/expect if it's only for Jest. Instead of @jest/globals which also pulls in describe test etc.. Feels slightly more natural for libraries only doing custom assertions (like jest-extended).

@jest/expect would then do the interface merging for the snapshot matchers and the extend call itself.

Not sure, might not make sense 😀

packages/expect/src/types.ts

packages/jest-jasmine2/src/types.ts

packages/expect/__typetests__/expect.test.ts

CHANGELOG.md

SimenB

this is great, I'm super excited about where this is going!

mrazauskas · 2022-02-15T19:46:46Z

Same here. Thanks for good questions. It is interesting exercise to think about and to shape these types.

Semi-related - thoughts about a @jest/expect which adds the snapshot matchers? That way people can use expect as a "generic" assertion module, @jest/expect if it's only for Jest.

Sounds interesting. And then @jest/globals could pull types from @jest/expect too?

mrazauskas · 2022-02-15T20:03:43Z

Wait.. So, you think @jest/expect would be simply this one + right types: https://github.com/facebook/jest/blob/main/packages/jest-circus/src/legacy-code-todo-rewrite/jestExpect.ts. That might be very good.

SimenB · 2022-02-15T20:09:27Z

Semi-related - thoughts about a @jest/expect which adds the snapshot matchers? That way people can use expect as a "generic" assertion module, @jest/expect if it's only for Jest.

Sounds interesting. And then @jest/globals could pull types from @jest/expect too?

Yep

Wait.. So, you think @jest/expect would be simply this one + right types: https://github.com/facebook/jest/blob/main/packages/jest-circus/src/legacy-code-todo-rewrite/jestExpect.ts. That might be very good.

Yeah, exactly

github-actions · 2022-03-18T00:07:21Z

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.

fix: expose ExpectationResult

3d31e39

facebook-github-bot added the cla signed label Feb 12, 2022

mrazauskas commented Feb 12, 2022

View reviewed changes

mrazauskas added 2 commits February 12, 2022 17:01

upgrade tsd-lite

2273adf

add test for ExpectationResult

47bca7f

mrazauskas commented Feb 12, 2022

View reviewed changes

packages/expect/__typetests__/expect.test.ts Outdated Show resolved Hide resolved

mrazauskas added 3 commits February 12, 2022 17:09

one more type test

3064a78

add changelog entry

eafcbc2

add MatcherState test

f97e5a8

SimenB reviewed Feb 13, 2022

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

examples/expect-extend/toBeWithinRange.ts Show resolved Hide resolved

mrazauskas added 2 commits February 13, 2022 11:14

fix changelog

f0407fc

expose MatcherFunction

b16b3a4

mrazauskas marked this pull request as draft February 14, 2022 08:26

mrazauskas commented Feb 14, 2022

View reviewed changes

packages/expect/src/types.ts Show resolved Hide resolved

SimenB reviewed Feb 15, 2022

View reviewed changes

examples/expect-extend/toBeWithinRange.ts Outdated Show resolved Hide resolved

SimenB reviewed Feb 15, 2022

View reviewed changes

packages/jest-snapshot/src/index.ts Outdated Show resolved Hide resolved

mrazauskas added 2 commits February 15, 2022 20:06

fix example

71d8de1

fix last details

ab8c2ea

mrazauskas changed the title ~~fix(expect): expose ExpectationResult instead of RawMatcherFn (partly reverts #12363)~~ feat(expect): replace RawMatcherFn with MatcherFunction and MatcherFunctionWithState types Feb 15, 2022

mrazauskas commented Feb 15, 2022

View reviewed changes

packages/expect/src/types.ts Show resolved Hide resolved

mrazauskas commented Feb 15, 2022

View reviewed changes

packages/jest-jasmine2/src/types.ts Show resolved Hide resolved

mrazauskas commented Feb 15, 2022

View reviewed changes

packages/expect/__typetests__/expect.test.ts Outdated Show resolved Hide resolved

tweak changelog

f0bca5d

mrazauskas marked this pull request as ready for review February 15, 2022 19:30

Merge branch 'main'

3023e88

mrazauskas commented Feb 15, 2022

View reviewed changes

CHANGELOG.md Show resolved Hide resolved

remove demo

4589553

SimenB approved these changes Feb 15, 2022

View reviewed changes

SimenB merged commit 4318575 into jestjs:main Feb 15, 2022

mrazauskas deleted the fix-expose-ExpectationResult branch February 15, 2022 20:23

github-actions bot locked as resolved and limited conversation to collaborators Mar 18, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(expect): replace `RawMatcherFn` with `MatcherFunction` and `MatcherFunctionWithState` types #12376

feat(expect): replace `RawMatcherFn` with `MatcherFunction` and `MatcherFunctionWithState` types #12376

mrazauskas commented Feb 12, 2022 •

edited

mrazauskas Feb 12, 2022

mrazauskas Feb 12, 2022

codecov-commenter commented Feb 12, 2022 •

edited

SimenB commented Feb 15, 2022

mrazauskas commented Feb 15, 2022

SimenB commented Feb 15, 2022 •

edited

SimenB left a comment

mrazauskas commented Feb 15, 2022 •

edited

mrazauskas commented Feb 15, 2022

SimenB commented Feb 15, 2022 •

edited

github-actions bot commented Mar 18, 2022

		compare(...args: Array<unknown>): ExpectationResult;
		negativeCompare(...args: Array<unknown>): ExpectationResult;

feat(expect): replace RawMatcherFn with MatcherFunction and MatcherFunctionWithState types #12376

feat(expect): replace RawMatcherFn with MatcherFunction and MatcherFunctionWithState types #12376

Conversation

mrazauskas commented Feb 12, 2022 • edited

Summary

Test plan

mrazauskas Feb 12, 2022

Choose a reason for hiding this comment

mrazauskas Feb 12, 2022

Choose a reason for hiding this comment

codecov-commenter commented Feb 12, 2022 • edited

Codecov Report

SimenB commented Feb 15, 2022

mrazauskas commented Feb 15, 2022

SimenB commented Feb 15, 2022 • edited

SimenB left a comment

Choose a reason for hiding this comment

mrazauskas commented Feb 15, 2022 • edited

mrazauskas commented Feb 15, 2022

SimenB commented Feb 15, 2022 • edited

github-actions bot commented Mar 18, 2022

feat(expect): replace `RawMatcherFn` with `MatcherFunction` and `MatcherFunctionWithState` types #12376

feat(expect): replace `RawMatcherFn` with `MatcherFunction` and `MatcherFunctionWithState` types #12376

mrazauskas commented Feb 12, 2022 •

edited

codecov-commenter commented Feb 12, 2022 •

edited

SimenB commented Feb 15, 2022 •

edited

mrazauskas commented Feb 15, 2022 •

edited

SimenB commented Feb 15, 2022 •

edited