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: fix any() for symbols and bigints (#10179) #10223

Merged
merged 2 commits into from Jul 2, 2020

Conversation

ninevra
Copy link
Contributor

@ninevra ninevra commented Jul 2, 2020

Summary

Adds clauses to expect's Any.asymmetricMatch() to handle Symbol and BigInt analogously to other primitives, i.e., using typeof instead of instanceof. This makes expect.any()'s behavior more consistent across primitive types.

Adds a clause to Any.asymmetricMatch() to exclude null from expect.any(Object). This makes expect.any()'s behavior more consistent with its documentation; null is not created with the Object constructor, and so should not be matched.

Closes #10179.

Test plan

Added checks to the list in asymmetricMatchers.test.ts test('Any.asymmetricMatch())' to cover the changed behavior.

@ninevra ninevra force-pushed the any-asymmetric-matcher-bugfix branch from 6f01978 to cd23c45 Compare July 2, 2020 04:57
Copy link
Collaborator

@thymikee thymikee left a comment

Choose a reason for hiding this comment

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

LGTM, left a note on the null

@@ -46,13 +46,22 @@ class Any extends AsymmetricMatcher<any> {
}

if (this.sample == Object) {
return typeof other == 'object';
return typeof other == 'object' && other !== null;
Copy link
Collaborator

Choose a reason for hiding this comment

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

this is kinda breaking, null is and Object in JS after all. Please revert

@ninevra
Copy link
Contributor Author

ninevra commented Jul 2, 2020

I've added a commit reverting the change to null handling, as requested.

If any(Object) matching null is intended, would there be interest in documenting that? It seems unlikely to me that someone asserting a value is an Object would intend to allow null. This is also an area where Jest differs from Jasmine's original implementation.

@ninevra ninevra requested a review from thymikee July 2, 2020 08:46
@thymikee
Copy link
Collaborator

thymikee commented Jul 2, 2020

@SimenB @jeysal thoughts? I don't have strong feelings about treating null as Object. It's valid from JS point of view, but it's also often confusing. If we're about to change it, we need to do it in a major version bump because it's technically breaking.

@SimenB
Copy link
Member

SimenB commented Jul 2, 2020

typeof null === 'object' is one of my least favorite parts of the language, but I don't think we should deviate from that - I think that'll just cause more confusion

@thymikee thymikee changed the title fix: fix any() for symbols, bigints, null (#10179) fix: fix any() for symbols and bigints (#10179) Jul 2, 2020
@thymikee thymikee merged commit 08f00e9 into jestjs:master Jul 2, 2020
@ninevra ninevra deleted the any-asymmetric-matcher-bugfix branch July 2, 2020 19:07
@github-actions
Copy link

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 May 11, 2021
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.

expect: any() doesn't match symbols or bigints, does match null
4 participants