Add support for workers to send custom messages to parent in jest-worker #9662
Conversation
rogeliog
changed the title
rogeliog
changed the title
rogeliog
requested review from
SimenB,
jeysal and
rickhanlonii
and removed request for
SimenB and
jeysal
|
🙀 I missed this PR @rogeliog, sorry! |
| PARENT_MESSAGE_OK, | ||
| } from '../../types'; | ||
| import expectExport from 'expect'; |
There was a problem hiding this comment.
| import expectExport from 'expect'; |
| try { | ||
| const {isMainThread, parentPort} = require('worker_threads'); | ||
| return !isMainThread && parentPort; | ||
| } catch (_) { |
There was a problem hiding this comment.
| } catch (_) { | |
| } catch { |
but what's this guard for? The fact the module is missing in node 8? If so, would this code path ever be called - wouldn't earlier guards catch it?
| @@ -228,6 +234,8 @@ export default class ChildProcessWorker implements WorkerInterface { | |||
| return onProcessEnd(...args); | |||
| }; | |||
|
|
|||
| this._onCustomMessage = (...arg) => onCustomMessage(...arg); | |||
There was a problem hiding this comment.
| this._onCustomMessage = (...arg) => onCustomMessage(...arg); | |
| this._onCustomMessage = onCustomMessage; |
or is it somehow different?
| @@ -56,6 +59,10 @@ export type PoolExitResult = { | |||
| forceExited: boolean; | |||
| }; | |||
|
|
|||
| export interface PromiseWithCustomMessage<T> extends Promise<T> { | |||
| onCustomMessage?: (listener: OnCustomMessage) => () => void; | |||
There was a problem hiding this comment.
this feels slightly wonky, but if it works I'm all for it. maybe
| onCustomMessage?: (listener: OnCustomMessage) => () => void; | |
| unstable_onCustomMessage?: (listener: OnCustomMessage) => () => void; |
just so people know not to use it if they find it? Leaves us with some wiggle-room
| @@ -19,6 +19,7 @@ export const CHILD_MESSAGE_END: 2 = 2; | |||
| export const PARENT_MESSAGE_OK: 0 = 0; | |||
| export const PARENT_MESSAGE_CLIENT_ERROR: 1 = 1; | |||
| export const PARENT_MESSAGE_SETUP_ERROR: 2 = 2; | |||
| export const PARENT_MESSAGE_CUSTOM: -1 = -1; | |||
There was a problem hiding this comment.
is it cleaner with as const? maybe not
| }; | ||
| }; | ||
|
|
||
| const onCustomMessage: OnCustomMessage = (message: any) => { |
There was a problem hiding this comment.
| const onCustomMessage: OnCustomMessage = (message: any) => { | |
| const onCustomMessage: OnCustomMessage = message => { |
OnCustomMessage should make sure this can be inferred, no?
|
I can't give large PRs like this a proper review atm, only just managed to get back below ~30 Jest notifications after months. But if there are parts around how workers exit, which I've dealt with somewhat recently, happy to take a look at specifics. |
|
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 PR part of an effort to break down PR #9001, so that we can eventually have #6616
It adds jest-worker the ability for workers to send "custom messages" to their parent while they are still running. This will eventually allow us to report per test case updates to the reporter.
Test plan
This should not break the API, and I added multiple tests for it. I should also add an e2e test for it, but that is still pending.
I intentionally kept it undocumented given that I'm still not 100% sure that this is the right interface. Currently it uses a
PromiseWithCustomMessageinterface, that allows to callp.onCustomMessage(listener)to a thenable.I would love feedback on that interface or potential new ideas for it.