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 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 ninevra:any-asymmetric-matcher-bugfix branch from 6f01978 to cd23c45 Jul 2, 2020
@thymikee thymikee requested a review from pedrottimark Jul 2, 2020
Copy link
Collaborator

thymikee left a comment

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;

This comment has been minimized.

@thymikee

thymikee Jul 2, 2020 Collaborator

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 Jul 2, 2020
@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
Collaborator

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 facebook:master Jul 2, 2020
22 checks passed
22 checks passed
cleanup-runs
Details
Running TypeScript compiler & ESLint
Details
Node v10.x on ubuntu-latest
Details
Node v10.x on macOS-latest
Details
Node v10.x on windows-latest
Details
Node v12.x on ubuntu-latest
Details
Node v12.x on macOS-latest
Details
Node v12.x on windows-latest
Details
Node v13.x on ubuntu-latest
Details
Node v13.x on macOS-latest
Details
Node v13.x on windows-latest
Details
Node v14.x on ubuntu-latest
Details
Node v14.x on macOS-latest
Details
Node v14.x on windows-latest
Details
ci/circleci: test-jest-circus Your tests passed on CircleCI!
Details
ci/circleci: test-node-10 Your tests passed on CircleCI!
Details
ci/circleci: test-node-12 Your tests passed on CircleCI!
Details
ci/circleci: test-node-13 Your tests passed on CircleCI!
Details
ci/circleci: test-node-14 Your tests passed on CircleCI!
Details
ci/circleci: test-or-deploy-website Your tests passed on CircleCI!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
facebook.jest #20200702.4 succeeded
Details
@ninevra ninevra deleted the ninevra:any-asymmetric-matcher-bugfix branch Jul 2, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

4 participants
You can’t perform that action at this time.