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

fix(expect): expose AsymmetricMatchers and RawMatcherFn interfaces #12363

Merged
merged 9 commits into from Feb 12, 2022

Conversation

mrazauskas
Copy link
Contributor

@mrazauskas mrazauskas commented Feb 10, 2022

Summary

Just wanted to check if now it is possible to extend interfaces of matchers for expect.extend(). Couple of details missing:

  • AsymmetricMatchers interface is not yet public, but it should be. And then this is possible:
declare module 'expect' {
  interface AsymmetricMatchers {
    toBeWithinRange(floor: number, ceiling: number): void;
  }
  interface Matchers<R> {
    toBeWithinRange(floor: number, ceiling: number): void;
  }
}
  • exposed RawMatcherFn is useful internally and provides type safety for this pattern:
import {expect} from '@jest/globals';
import type {RawMatcherFn} from 'expect';

const someCustomMatcher: RawMatcherFn = (x, y, z) => {
  return {
    message: () => '',
    pass: true,
  };
};

expect.extend({
  someCustomMatcher,
});

Nice! Shall I add it to docs as well?

An example is added to examples folder. Regarding the docs update, I think it would be better idea to move expect.extend() to a separate guide chapter similar to "Code Transformation". The expect.extend() section is already too long inside Expect chapter. So perhaps I will make another PR with this change.

Test plan

Type tests are added.

@SimenB
Copy link
Collaborator

SimenB commented Feb 10, 2022

Nice! Shall I add it to docs as well?

Yes please! 😀

SimenB
SimenB approved these changes Feb 10, 2022
Copy link
Collaborator

@SimenB SimenB left a comment

yay!

@mrazauskas mrazauskas changed the title fix(expect): export AsymmetricMatchers interface fix(expect): expose AsymmetricMatchers and RawMatcherFn interfaces Feb 11, 2022
packages/expect/tsconfig.json Outdated Show resolved Hide resolved
"babel-jest": "*",
"expect": "*",
"jest": "*"
Copy link
Collaborator

@SimenB SimenB Feb 11, 2022

Choose a reason for hiding this comment

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

do we need workspace:* here so we resolve to the versions we want?

Copy link
Collaborator

@SimenB SimenB Feb 11, 2022

Choose a reason for hiding this comment

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

potentially just resolutions in root:

jest/package.json

Lines 157 to 159 in 60eb416

"babel-jest": "workspace:*",
"jest": "workspace:*",
"jest-environment-node": "workspace:*",


// extend

type MatcherUtils = typeof jestMatcherUtils & {
Copy link
Collaborator

@SimenB SimenB Feb 11, 2022

Choose a reason for hiding this comment

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

we should export this type probably

Copy link
Contributor Author

@mrazauskas mrazauskas Feb 11, 2022

Choose a reason for hiding this comment

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

Hm.. But this is just a helper for testing. Perhaps that’s fine?

Copy link
Collaborator

@SimenB SimenB Feb 11, 2022

Choose a reason for hiding this comment

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

it's a copy of

utils: typeof jestMatcherUtils & {
iterableEquality: Tester;
subsetEquality: Tester;
};
, no?

Copy link
Contributor Author

@mrazauskas mrazauskas Feb 11, 2022

Choose a reason for hiding this comment

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

Got it. I though you speak about exporting typeof jestMatcherUtil somehow.

Copy link
Contributor Author

@mrazauskas mrazauskas Feb 11, 2022

Choose a reason for hiding this comment

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

Do you think MatcherUtils type should be public? Type test is importing from build, not from source.

Copy link
Collaborator

@SimenB SimenB Feb 11, 2022

Choose a reason for hiding this comment

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

it was just some duplication that felt unnecessary, I don't feel strongly here 🙂

declare module 'expect' {
interface AsymmetricMatchers {
toBeWithinRange(a: number, b: number): void;
}
interface Matchers<R> {
toBeWithinRange(a: number, b: number): R;
}
}
Copy link
Collaborator

@SimenB SimenB Feb 11, 2022

Choose a reason for hiding this comment

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

there's no way we can avoid the duplication here, right?

Copy link
Contributor Author

@mrazauskas mrazauskas Feb 11, 2022

Choose a reason for hiding this comment

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

Wrestled with this. No luck. Alternative idea: what if expect.extend() would return Expect extended with types of custom matchers? Right now expect.extend() returns void. The proposed return value would be only useful for typings. No need to extend, augment, etc.

import {expect as jestExpect} from '@jest/globals';
import customMatcher from 'customMatcher';

const expect = jestExpect.extend({
  customMatcher,
});

// here `expect.customMatcher()` is fully typed

Copy link
Contributor Author

@mrazauskas mrazauskas Feb 11, 2022

Choose a reason for hiding this comment

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

Drafty, but working prototype (in this case, toBeWithinRange takes two arguments and both are inferred):

Screenshot 2022-02-11 at 20 50 11

Copy link
Collaborator

@SimenB SimenB Feb 11, 2022

Choose a reason for hiding this comment

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

I like it! 😃 We need "old" way to continue working of course, but doing it this way seems smoother

SimenB
SimenB approved these changes Feb 12, 2022
Copy link
Collaborator

@SimenB SimenB left a comment

awesome!!

@SimenB SimenB merged commit faef0b4 into facebook:main Feb 12, 2022
20 of 26 checks passed
@@ -13,15 +13,16 @@ import {INTERNAL_MATCHER_FLAG} from './jestMatchersObject';

export type SyncExpectationResult = {
pass: boolean;
message: () => string;
message(): string;
};

export type AsyncExpectationResult = Promise<SyncExpectationResult>;

export type ExpectationResult = SyncExpectationResult | AsyncExpectationResult;

export type RawMatcherFn<T extends MatcherState = MatcherState> = {
Copy link
Collaborator

@SimenB SimenB Feb 12, 2022

Choose a reason for hiding this comment

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

Do we need Raw in the name? Doesn't seem like something one should use when exported 😅

@github-actions
Copy link

github-actions bot commented Mar 19, 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 19, 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

3 participants