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

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

Merged
merged 13 commits into from Feb 15, 2022

Conversation

mrazauskas
Copy link
Contributor

@mrazauskas mrazauskas commented Feb 12, 2022

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:

export type RawMatcherFn<T extends MatcherState = MatcherState> = {
(this: T, actual: any, expected: any, options?: any): ExpectationResult;

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.

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

@mrazauskas mrazauskas Feb 12, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

compare(...args: Array<unknown>): ExpectationResult;
negativeCompare(...args: Array<unknown>): ExpectationResult;
Copy link
Contributor Author

@mrazauskas mrazauskas Feb 12, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Puzzle. To be precise this type should be () => (...args: Array<unknown>) => ExpectationResult, but my version eliminates these @ts-expect-error:

return this.isNot
? negativeCompare.apply(
null,
// @ts-expect-error
args,
)
: result.compare.apply(
null,
// @ts-expect-error
args,
);

@codecov-commenter
Copy link

codecov-commenter commented Feb 12, 2022

Codecov Report

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

Impacted file tree graph

@@           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.

CHANGELOG.md Outdated Show resolved Hide resolved
examples/expect-extend/toBeWithinRange.ts Show resolved Hide resolved
@mrazauskas mrazauskas marked this pull request as draft Feb 14, 2022
packages/jest-snapshot/src/index.ts Outdated Show resolved Hide resolved
@SimenB
Copy link
Collaborator

SimenB commented Feb 15, 2022

@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
Copy link
Contributor Author

mrazauskas commented Feb 15, 2022

@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
Copy link
Collaborator

SimenB commented Feb 15, 2022

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 😀

@mrazauskas 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 mrazauskas marked this pull request as ready for review Feb 15, 2022
CHANGELOG.md Show resolved Hide resolved
SimenB
SimenB approved these changes Feb 15, 2022
Copy link
Collaborator

@SimenB SimenB left a comment

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

@mrazauskas
Copy link
Contributor Author

mrazauskas commented Feb 15, 2022

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
Copy link
Contributor Author

mrazauskas commented Feb 15, 2022

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
Copy link
Collaborator

SimenB commented Feb 15, 2022

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

@SimenB SimenB merged commit 4318575 into facebook:main Feb 15, 2022
25 of 27 checks passed
@mrazauskas mrazauskas deleted the fix-expose-ExpectationResult branch Feb 15, 2022
@github-actions
Copy link

github-actions bot commented Mar 18, 2022

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.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 18, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants