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(extend): custom asymmetric matchers #32740

Closed

Conversation

muhqu
Copy link
Contributor

@muhqu muhqu commented Sep 20, 2024

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

@dgozman
Copy link
Contributor

dgozman commented Sep 26, 2024

@muhqu Thank you for the PR!

We have actually fixed the regression introduced by #32366 in #32795. I've added a test for asymmetric matches to make sure it won't regress in the future in #32829.

That said, we are not really sure this is the API we would love to support long-term, therefore we don't want to add types support for it right now. We'll think about the API as a part of #32562.

I think we can close this PR right now, unless I am missing something and there are changes we should land. Let me know if that's the case. Thank you for contributing!

@muhqu
Copy link
Contributor Author

muhqu commented Sep 26, 2024

@dgozman okay, sounds fair. I'm glad you fixed it for backward compatibility.

However, I think you should really consider retaining the type information of asymmetric matchers. It just feels broken, if there is something supported under the hood, but not exposed via TypeScript. Are you questioning extensibility of expect() in general? … we can also take the discussion to #32562 if that makes sense.

I've updated this PR to only include the type definition fix.

Copy link
Contributor

Test results for "tests 1"

2 flaky ⚠️ [playwright-test] › ui-mode-test-setup.spec.ts:22:5 › should run global setup and teardown
⚠️ [webkit-page] › page/workers.spec.ts:245:3 › should support offline

36375 passed, 759 skipped
✔️✔️✔️

Merge workflow run.

@dgozman
Copy link
Contributor

dgozman commented Sep 27, 2024

@muhqu As I said, we are not sure we'd like to keep this API for asymmetric matchers going forward. It's better to keep discussion to the issue instead of the PR for future discoverability though.

@pavelfeldman
Copy link
Member

Closing this so that we could discuss it in the issue instead.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants