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

Delete expect and jest-matcher-utils dependencies #332

Closed
wants to merge 1 commit into from
Closed

Delete expect and jest-matcher-utils dependencies #332

wants to merge 1 commit into from

Conversation

keeganwitt
Copy link
Collaborator

@keeganwitt keeganwitt commented Jul 1, 2021

What

Don't depend on expect and jest-matcher-utils directly.

Why

utils are already injected into all the matchers.

Notes

Housekeeping

  • Unit tests
  • Documentation is up to date
  • No additional lint warnings
  • Typescript definitions are added/updated where relevant

Copy link
Member

@SimenB SimenB left a comment

Choose a reason for hiding this comment

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

looks great! I have a couple of suggestions to reduce the diff, but the migration itself LGTM. I only commented in the first file, but the comments apply to them all

src/matchers/toBeAfter/index.js Outdated Show resolved Hide resolved
src/matchers/toBeAfter/index.js Outdated Show resolved Hide resolved

const failMessage = (received, after) => () =>
matcherHint('.toBeAfter', 'received', '') +
const failMessage = (received, after, context) => () =>
Copy link
Member

@SimenB SimenB Jul 1, 2021

Choose a reason for hiding this comment

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

Suggested change
const failMessage = (received, after, context) => () =>
const failMessage = (received, after, { matcherHint, printReceived }) => () =>

etc? again, to reduce diff

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It would reduce diff, but I thought it was kinda nice that the matchers all followed the same pattern of all using context (although will now be utils when I make that change).

@SimenB
Copy link
Member

SimenB commented Jul 1, 2021

import { equals } from 'expect/build/jasmineUtils';
remove this?

@keeganwitt
Copy link
Collaborator Author

import { equals } from 'expect/build/jasmineUtils';

remove this?

Where does the equals come from instead? This is outside a matcher, so nothing is injected, right?

@SimenB
Copy link
Member

SimenB commented Jul 2, 2021

yeah, it cannot be imported like this, the matcher must inject it into the helper functions

@keeganwitt
Copy link
Collaborator Author

yeah, it cannot be imported like this, the matcher must inject it into the helper functions

Got it. Made that change.

Copy link
Member

@SimenB SimenB left a comment

Choose a reason for hiding this comment

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

I'll rebase this since I've introduced a conflict in pretty much all files 🙈

src/matchers/index.test.js Outdated Show resolved Hide resolved
CONTRIBUTING.md Outdated Show resolved Hide resolved
CONTRIBUTING.md Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Jul 9, 2021

Codecov Report

Merging #332 (08b9497) into main (cc53ef3) will not change coverage.
The diff coverage is 100.00%.

❗ Current head 08b9497 differs from pull request most recent head 5491f54. Consider uploading reports for the commit 5491f54 to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##              main      #332    +/-   ##
==========================================
  Coverage   100.00%   100.00%            
==========================================
  Files          137       109    -28     
  Lines          843       643   -200     
  Branches       143       100    -43     
==========================================
- Hits           843       643   -200     
Impacted Files Coverage Δ
src/matchers/toContainEntries/predicate.js 100.00% <ø> (ø)
src/matchers/toContainEntry/predicate.js 100.00% <ø> (ø)
src/matchers/toBeAfter/index.js 100.00% <100.00%> (ø)
src/matchers/toBeArray/index.js 100.00% <100.00%> (ø)
src/matchers/toBeArrayOfSize/index.js 100.00% <100.00%> (ø)
src/matchers/toBeBefore/index.js 100.00% <100.00%> (ø)
src/matchers/toBeBoolean/index.js 100.00% <100.00%> (ø)
src/matchers/toBeDate/index.js 100.00% <100.00%> (ø)
src/matchers/toBeEmpty/index.js 100.00% <100.00%> (ø)
src/matchers/toBeEmpty/predicate.js 100.00% <100.00%> (ø)
... and 148 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update cc53ef3...5491f54. Read the comment docs.

@SimenB
Copy link
Member

SimenB commented Aug 13, 2021

Seems this failed CI, could you rebase to retrigger?

@keeganwitt
Copy link
Collaborator Author

Seems this failed CI, could you rebase to retrigger?

Rebased, but the asymmetric matchers still aren't working.

@SimenB
Copy link
Member

SimenB commented Oct 4, 2021

Right, so the failure is (imo) a bug in Jest. I've opened up a PR over there (jestjs/jest#11926), but unfortunately that means we can't really land this PR without breaking everybody using the matchers as asymmetric ones.

@SimenB
Copy link
Member

SimenB commented Oct 6, 2021

We could tho, and just say that to use the asymmetric matchers you need to use Jest 27.x (whichever version it is when I make a release)?

/cc @rickhanlonii thoughts?

@rickhanlonii
Copy link
Contributor

Could we release a new major of jest-extended instead?

@SimenB
Copy link
Member

SimenB commented Nov 29, 2021

That's easiest, yeah 🙂

@keeganwitt
Copy link
Collaborator Author

Jest versions older than v27.2.5 fail because they lack the asymmetric matchers fix.

@SimenB
Copy link
Member

SimenB commented Jan 25, 2022

Jest versions older than v27.2.5 fail because they lack the asymmetric matchers fix.

Yep, so we need to drop those versions from CI and update the peer dependency range before landing

@keeganwitt
Copy link
Collaborator Author

Replaced by #405 (because I'm no longer able to push to my fork after having deleted it).

@keeganwitt keeganwitt closed this Jan 25, 2022
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.

None yet

4 participants