-
-
Notifications
You must be signed in to change notification settings - Fork 6.5k
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
jest snapshot - TS migration #7899
jest snapshot - TS migration #7899
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR, this is great! Left som inline comments.
The big one is that it seems we have to stick some more types into @jest/types
in order to avoid changing the exported interface of the modules we have that are stuck on CJS
packages/jest-snapshot/src/index.ts
Outdated
import diff from 'jest-diff'; | ||
import {EXPECTED_COLOR, matcherHint, RECEIVED_COLOR} from 'jest-matcher-utils'; | ||
import {MatcherState} from 'expect'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
an you put this in @jest/types
instead? We'll move it back into expect
when it is migrated to TS (or probably keep it in @jest/types
until the next major since expect
doesn't use ESM)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/cc @natealcedo relevant for your expect
migration 🙂
Co-Authored-By: doniyor2109 <doniyor2109@gmail.com>
Co-Authored-By: doniyor2109 <doniyor2109@gmail.com>
Co-Authored-By: doniyor2109 <doniyor2109@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@doniyor2109 all |
@SimenB That is ok. Thank you for great feedbacks. Just keep going giving great feedbacks 😄 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Getting closer! 😀
Co-Authored-By: doniyor2109 <doniyor2109@gmail.com>
I merged in master and fixed the type error, so now we should be able to see if we've broken any tests 🙂 |
Codecov Report
@@ Coverage Diff @@
## master #7899 +/- ##
==========================================
- Coverage 57.98% 57.91% -0.07%
==========================================
Files 171 164 -7
Lines 6422 6093 -329
Branches 6 5 -1
==========================================
- Hits 3724 3529 -195
+ Misses 2696 2562 -134
Partials 2 2 Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you so much! Took a bit to get it over the line, but this is really awesome work
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. |
Summary
TS migration for
jest-snapshot
Built diff:
Test plan
yarn test