-
-
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
Migrate jest-runner to typescript #7968
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.
Leaving some comments on the parts I need help. The test suite is failing for me locally. Can't verify if its just my machine or not. Well on the flipside, nearly there!
packages/jest-runner/src/runTest.ts
Outdated
|
||
environment.global.process.exit = function exit(...args) { | ||
(environment.global as NodeJS.Global).process.exit = function exit( | ||
...args: Array<any> |
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.
unknown doesnt work here
Regarding your |
Just pushed some more changes. Let me know what else needs resolving |
You need to add a couple of |
I've tweaked this a bit, I think it's pretty close now. Will review properly tomorrow 🙂 |
Hey thank you so much for the small fixes that I missed! They’re much appreciated. I’ll add in the diff and update the change log in a bit. |
Codecov Report
@@ Coverage Diff @@
## master #7968 +/- ##
========================================
Coverage ? 64.7%
========================================
Files ? 256
Lines ? 10051
Branches ? 1497
========================================
Hits ? 6504
Misses ? 3223
Partials ? 324
Continue to review full report at Codecov.
|
uid?: number; | ||
gid?: number; | ||
}; | ||
export {ForkOptions}; |
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 love these changes 😀
@@ -48,7 +48,7 @@ export interface Describe extends DescribeBase { | |||
skip: ItBase; | |||
} | |||
|
|||
export interface Global { | |||
export interface Global extends NodeJS.Global { |
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 didn't know this was the fix. I tried extending the global inside the declare
. Cool beans.
packages/jest-worker/src/types.ts
Outdated
@@ -5,6 +5,9 @@ | |||
* LICENSE file in the root directory of this source tree. | |||
*/ | |||
|
|||
import EventEmitter from 'events'; |
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.
Should this be import * as EventEmitter from 'events'
?
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.
It's the same object
$ node -p "require('events') === require('events').EventEmitter"
true
And the node docs does not destructure: https://nodejs.org/api/events.html#events_events
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.
My concern is that events
may not has a default export just like path
.
import path from 'path'
won't work on ts.
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.
Fair point! Changed. Should we add "allowSyntheticDefaultImports": false
to out tsconfig?
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.
That would conflict with esModuleInterop: true
. I think we need it for now, but in an ideal world in the future where everything is written in ESM we will be able to drop esModuleInterop
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.
Anyways, this PR is not the right place to discuss it. Might be worth a new issue, though, so we do it correctly
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.
esModuleInterop
doc: "Emit __importStar and __importDefault helpers for runtime babel ecosystem compatibility and enable --allowSyntheticDefaultImports for typesystem compatibility"
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.
Yeah, but that doesn't mean I can't disable it again, does it?
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.
Might be possible to, but it seems like that would mean to forbid default-importing module.exports
for type checking, but including the interop helper in output code. Because we use Babel, so our tsc does not emit anything, that would equal not enabling either of them at all, I think. And I believe we need it at least for default-importing some util libraries from npm that use module.exports =
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.
Anyways, this PR is not the right place to discuss it. Might be worth a new issue, though, so we do it correctly
😀
0f7b761
to
01485d4
Compare
This reverts commit 266a8c9.
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. |
This PR migrates jest-runner. I need help on some things hence opening an early PR
Heres the diff