feat(jest-worker)!: replace Worker class export with createWorkerFarm() factory function
#12680
Conversation
| * processed by the same worker. This is specially useful if your workers | ||
| * are caching results. | ||
| */ | ||
| export default class WorkerFarm { |
There was a problem hiding this comment.
Simply moved from index.ts to this file. Nothing much changed.
|
|
||
| export type JestWorkerFarm<T> = WorkerFarm & WorkerModule<T>; | ||
|
|
||
| const reservedKeys = ['end', 'getStderr', 'getStdout', 'setup', 'teardown']; |
There was a problem hiding this comment.
Currently setup() and teardown() are not excluded. Felt like these should not be present in the returned API.
| const reservedKeys = ['end', 'getStderr', 'getStdout', 'setup', 'teardown']; | ||
|
|
||
| const isReserved = (methodName: string) => reservedKeys.includes(methodName); | ||
| const isPrivate = (methodName: string) => methodName.startsWith('_'); |
There was a problem hiding this comment.
If a module is exporting a method, it is hardly private. Methods prefix with _ are currently excluded, I just left this in place.
| * processed by the same worker. This is specially useful if your workers | ||
| * are caching results. | ||
| */ | ||
| export class Worker { |
There was a problem hiding this comment.
Moved it to separate WorkerFarm.ts file to keep index.ts cleaner.
packages/jest-worker/src/types.ts
Outdated
| [K in keyof T as Extract< | ||
| ExcludeReservedKeys<K>, | ||
| MethodLikeKeys<T> | ||
| >]: T[K] extends FunctionLike ? Promisify<T[K]> : never; |
There was a problem hiding this comment.
Methods in the resulting API always return a Promise.
| if (!this._worker) { | ||
| if ((options && options.forceInBand) || this._options.maxWorkers <= 1) { | ||
| this._worker = {getSha1, worker}; | ||
| } else { | ||
| // @ts-expect-error: assignment of a worker with custom properties. | ||
| this._worker = new Worker(require.resolve('./worker'), { | ||
| this._worker = new WorkerFarm(require.resolve('./worker'), { |
There was a problem hiding this comment.
Yep. Still struggling with this one.
There was a problem hiding this comment.
@SimenB I was trying again to make this work, but cannot figure out how to make async function work in sync context. Probably some .then should be used to unwrap the promise. Might work, but this function already returns worker() that returns a Promise:
I was trying to play with .thens and to refactor this code to use async / await. Unfortunately it is too complex for me. Can’t make it work. Someone more experienced should take a take over this PR.
There was a problem hiding this comment.
cannot figure out how to make async function work in sync context.
Why/where does it have to work in sync context? If it's just https://github.com/facebook/jest/blob/8ef4d86f14954277394c7e4fb708697dfa92e80c/packages/jest-haste-map/src/index.ts#L743, making that return a Promise seems fine?
There was a problem hiding this comment.
Right, _getWorker can return a Promise. It is called and then is used to handle a Promise return by the worker:
The return above is is the return of _processFile method, that has a return type of Promise<void> | null:
This null part is where sync fun begins. It just must be Promise<void> | null, not something like Promise<void | null>, because of these lines:
I was playing with the filter above for some time with no luck. Might be some deeper refactor is needed.
|
@mrazauskas I'm planning to release 28 stable in the next few days (possibly this weekend). Just a heads up 🙂 I plan to have v29 come out when node 17 is EOL in June, so not too far into the future if you wanna wait. No rush either way 👍 |
|
Sure. I moved forward with refactoring |
|
Oh.. That’s to complicated to rebase and I can’t integrate it with haste module anyways. Leaving this for smarter than me (; Closing to have less noise around. |
|
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. |
Part of #12274
Summary
Here is an attempt to replace
Workerclass exported fromjest-workerwith an asynccreateWorkerFarm()factory function. This is needed to allow worker modules in ESM syntax.The
createWorkerFarm()function is implemented – it simply does some validation and returns and instance ofWorkerFarmclass. I have spend some time to make TypeScript usage smoother.I managed to reshape
jest-worker's implementation injest-reportersandjest-runner, but got stuck withjest-haste-map. Will try again, not giving up yet (;Test plan
Unit tests and type tests are added. All must pass.