-
-
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
chore: migrate @jest/core to TypeScript #7998
Conversation
This is almost ready, I just have some issues with Currently TS doesn't really complain - however I want to make the underscored properties @rogeliog @cpojer is this something you can provide some context on? A quick caveat is that the base class they extend/implement might be wrong, but I don't think so. Removing unused arguments is simple enough, but I just did the necessary syntax and type changes to the files to make them compile to keep the diff down before feedback EDIT: Well, you worked as a rubberduck - removed 4 of the 8 errors by fixing the base type 😅 |
packages/jest-core/src/watch.ts
Outdated
@@ -51,30 +56,34 @@ const INTERNAL_PLUGINS = [ | |||
QuitPlugin, | |||
]; | |||
|
|||
const RESERVED_KEY_PLUGINS = new Map([ | |||
// TODO: Is it correct with constructor here? |
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.
Hey @tdd 👋 Does this make sense to you? 🙂
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.
CI will pass whenever I get #8011 to work. |
@@ -24,7 +25,7 @@ | |||
"jest-watcher": "^24.0.0", | |||
"micromatch": "^3.1.10", | |||
"p-each-series": "^1.0.0", | |||
"pirates": "^4.0.0", | |||
"pirates": "^4.0.1", |
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.
4.0.1 has typings
@@ -1,29 +0,0 @@ | |||
// Copyright (c) Facebook, Inc. and its affiliates. All Rights Reserved. |
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.
no idea what this file is 🤷♀️
Codecov Report
@@ Coverage Diff @@
## master #7998 +/- ##
==========================================
- Coverage 64.1% 63.26% -0.84%
==========================================
Files 259 259
Lines 10165 10225 +60
Branches 1818 2082 +264
==========================================
- Hits 6516 6469 -47
- Misses 3250 3270 +20
- Partials 399 486 +87
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.
Well that took some time to look through. Thanks for tackling this monster 😅
write(message: string) { | ||
expect(wrap(message)).toMatchSnapshot(); | ||
}, | ||
} as NodeJS.WriteStream); | ||
|
||
it('prints the jest version', () => { |
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.
I don't understand what these tests do or what value the huge snapshots now provide
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.
idk
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.
👍 for the test change
Co-Authored-By: SimenB <[email protected]>
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.
Nice, getting closer 🚀
@thymikee feel free to leave feedback after the fact - I just wanna get this in since it tweaks a bunch of other types as well 🙂 |
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
This wasn't as bad as I thought it'd be...
There's still 34 type errors, but they should be pretty straightforward (I'm sick and it's late, so opening this now).Help very much appreciated!Test plan
Green CI (eventually)