feat: expose internal matchers on extended matchers context

[JVBorges]

Resolves #10590

Summary

The idea is to have access to the internal matchers Jest has when creating custom matchers. The internal matchers are only exposed to those functions created from expect.extend() function.

Also, I moved the CustomMatcher class definition to the asymmetricMatchers file. Since that class is only being used when defining the custom matcher as asymmetric in the expect object, I guess it makes more sense to have it over there.

Test plan

Tests have been added to ensure internal matchers are exposed in the custom matcher context

[SimenB]

Can you add an integration test with some snapshots showing what the errors look like if you use the matchers inside your own?

[JVBorges]

Can you add an integration test with some snapshots showing what the errors look like if you use the matchers inside your own?

Sure!

[mrazauskas]

Few more details regarding types. (I had to look through to remember how it all works.) Take a look at TypeScript tab in expect.extend documentation. A matcher is getting typed using MatcherFunction type. The this is typed through that type and expect.extend is receiving typed through MatcherFunction as well.

I am trying to say that you added typings of new prop directly to expect.extend, but that is not enough for usage.

Another tricky part – MatcherFunction is delivered from MatcherFunctionWithContext which is stricter version of RawMatcherFn. In the future RawMatcherFn should be replaced with MatcherFunctionWithContext. That’s not possible right now, because refactoring would bring in breaking changes.

Just wondering – did you consider simply adding matchers: Readonly<MatchersObject>; to the MatcherUtils interface? That would solve the whole complexity.

[JVBorges]

Few more details regarding types. (I had to look through to remember how it all works.) Take a look at TypeScript tab in expect.extend documentation. A matcher is getting typed using MatcherFunction type. The this is typed through that type and expect.extend is receiving typed through MatcherFunction as well.

I am trying to say that you added typings of new prop directly to expect.extend, but that is not enough for usage.

Another tricky part – MatcherFunction is delivered from MatcherFunctionWithContext which is stricter version of RawMatcherFn. In the future RawMatcherFn should be replaced with MatcherFunctionWithContext. That’s not possible right now, because refactoring would bring in breaking changes.

Just wondering – did you consider simply adding matchers: Readonly; to the MatcherUtils interface? That >would solve the whole complexity.

I did consider just adding to the MatcherUtils, but if I do that the internals matchers would have access as well, for me at least don't make much sense to have that's why I thought creating a different type for the functions created inside extend so they have a different context. I agree that it would be much more straightforward just to add to the MatcherUtils instead, what do you think would be a better solution?

[SimenB]

I don't mind internal matchers having access. Probably easier to have some InternalMatcher type which excludes it if we want anyways (if we wanna prevent it)

[JVBorges]

Sorry for the delay.

I've made the changes you guys asked.

[mrazauskas]

Would be nice to have a line in documentation; and to add a type test to packages/expect/__typetests__/expect.test.ts.

To run the type tests: build the repo and run yarn test-types. The types are imported from expect package, so remember to rebuild if you change types.

[SimenB]

I'm thinking this is fine as is and we can make the types better in a future patch. Unless you're currently working on the types @JVBorges?

[SimenB]

to make this test pass I have to do this.matchers.toEqual.call(this, received, expected); here - i.e. explicitly pass in this. any idea what changed to make this test now fail?

[JVBorges]

no clue but I'll take a look

[JVBorges]

Solved this by binding the context to the matchers when I expose them, but doing this way the unit testing started failing.

Later I will try to fix them

[JVBorges]

I'm thinking this is fine as is and we can make the types better in a future patch. Unless you're currently working on the types @JVBorges?

Not yet, but I was going to check it out this weekend.

[SimenB]

Cool, thanks! 👍

[JVBorges]

So I made this change where I bind the context to every matcher before exposing them but this caused the unit test to fail. Before, the test passed because they were the same object so when compared with Object.is returned true
https://github.com/facebook/jest/blob/bed4891b0e5b614988e5a65fca29c4734cdb3c2e/packages/expect-utils/src/jasmineUtils.ts#L95-L97

Since now the exposed matchers have been mutated, Object.is start failing thus the algorithm starts to check each key and fails at this line: https://github.com/facebook/jest/blob/bed4891b0e5b614988e5a65fca29c4734cdb3c2e/packages/expect-utils/src/jasmineUtils.ts#L129-L131

I'm not sure how I'll do this unit test now. Do you guys have any clue?

[JVBorges]

@mrazauskas @SimenB any suggestions?

[mrazauskas]

Unfortunately I have nothing to suggest at this moment.

[github-actions]

This PR is stale because it has been open 90 days with no activity. Remove stale label or comment or this will be closed in 30 days.

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