Skip to content
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-worker to TypeScript #7853

Merged
merged 2 commits into from
Feb 10, 2019
Merged

Conversation

SimenB
Copy link
Member

@SimenB SimenB commented Feb 10, 2019

Summary

I left the tests alone as there were way too many typer errors for the effort I wanted to put into it :P

At some point we can probably make the types smarter (having it infer exports from the file coming in, filtering by exposedMethods etc). My hope is that someone will contribute that as a PR at some point 😀

built diff:

diff --git i/packages/jest-worker/build/Farm.js w/packages/jest-worker/build/Farm.js
index ed652e867..f16fe5df8 100644
--- i/packages/jest-worker/build/Farm.js
+++ w/packages/jest-worker/build/Farm.js
@@ -1,11 +1,3 @@
-/**
- * Copyright (c) Facebook, Inc. and its affiliates. All Rights Reserved.
- *
- * This source code is licensed under the MIT license found in the
- * LICENSE file in the root directory of this source tree.
- *
- *
- */
 'use strict';
 
 Object.defineProperty(exports, '__esModule', {
@@ -15,6 +7,12 @@ exports.default = void 0;
 
 var _types = require('./types');
 
+/**
+ * Copyright (c) Facebook, Inc. and its affiliates. All Rights Reserved.
+ *
+ * This source code is licensed under the MIT license found in the
+ * LICENSE file in the root directory of this source tree.
+ */
 class Farm {
   constructor(numOfWorkers, callback, computeWorkerKey) {
     this._callback = callback;
@@ -38,7 +36,7 @@ class Farm {
       let hash = null;
 
       if (computeWorkerKey) {
-        hash = computeWorkerKey.apply(this, [method].concat(args));
+        hash = computeWorkerKey.call(this, method, ...args);
         worker = hash == null ? null : this._cacheKeys[hash];
       }
 
@@ -74,7 +72,7 @@ class Farm {
     let queueHead = this._queue[workerId];
 
     while (queueHead && queueHead.request[1]) {
-      queueHead = queueHead.next;
+      queueHead = queueHead.next || null;
     }
 
     this._queue[workerId] = queueHead;
diff --git i/packages/jest-worker/build/WorkerPool.js w/packages/jest-worker/build/WorkerPool.js
index 5ab4dada6..16ed8908b 100644
--- i/packages/jest-worker/build/WorkerPool.js
+++ w/packages/jest-worker/build/WorkerPool.js
@@ -1,11 +1,3 @@
-/**
- * Copyright (c) Facebook, Inc. and its affiliates. All Rights Reserved.
- *
- * This source code is licensed under the MIT license found in the
- * LICENSE file in the root directory of this source tree.
- *
- *
- */
 'use strict';
 
 Object.defineProperty(exports, '__esModule', {
@@ -19,6 +11,12 @@ function _interopRequireDefault(obj) {
   return obj && obj.__esModule ? obj : {default: obj};
 }
 
+/**
+ * Copyright (c) Facebook, Inc. and its affiliates. All Rights Reserved.
+ *
+ * This source code is licensed under the MIT license found in the
+ * LICENSE file in the root directory of this source tree.
+ */
 const canUseWorkerThreads = () => {
   try {
     // $FlowFixMe: Flow doesn't know about experimental APIs
diff --git i/packages/jest-worker/build/base/BaseWorkerPool.js w/packages/jest-worker/build/base/BaseWorkerPool.js
index 8f83a8808..eefd02c84 100644
--- i/packages/jest-worker/build/base/BaseWorkerPool.js
+++ w/packages/jest-worker/build/base/BaseWorkerPool.js
@@ -1,11 +1,3 @@
-/**
- * Copyright (c) Facebook, Inc. and its affiliates. All Rights Reserved.
- *
- * This source code is licensed under the MIT license found in the
- * LICENSE file in the root directory of this source tree.
- *
- *
- */
 'use strict';
 
 Object.defineProperty(exports, '__esModule', {
@@ -13,20 +5,20 @@ Object.defineProperty(exports, '__esModule', {
 });
 exports.default = void 0;
 
-function _mergeStream() {
-  const data = _interopRequireDefault(require('merge-stream'));
+function _path() {
+  const data = _interopRequireDefault(require('path'));
 
-  _mergeStream = function _mergeStream() {
+  _path = function _path() {
     return data;
   };
 
   return data;
 }
 
-function _path() {
-  const data = _interopRequireDefault(require('path'));
+function _mergeStream() {
+  const data = _interopRequireDefault(require('merge-stream'));
 
-  _path = function _path() {
+  _mergeStream = function _mergeStream() {
     return data;
   };
 
@@ -39,6 +31,13 @@ function _interopRequireDefault(obj) {
   return obj && obj.__esModule ? obj : {default: obj};
 }
 
+/**
+ * Copyright (c) Facebook, Inc. and its affiliates. All Rights Reserved.
+ *
+ * This source code is licensed under the MIT license found in the
+ * LICENSE file in the root directory of this source tree.
+ */
+
 /* istanbul ignore next */
 const emptyMethod = () => {};
 
@@ -100,7 +99,7 @@ class BaseWorkerPool {
     return this._workers[workerId];
   }
 
-  createWorker(workerOptions) {
+  createWorker(_workerOptions) {
     throw Error('Missing method createWorker in WorkerPool');
   }
 
diff --git i/packages/jest-worker/build/index.js w/packages/jest-worker/build/index.js
index 45fc7702a..73ba44544 100644
--- i/packages/jest-worker/build/index.js
+++ w/packages/jest-worker/build/index.js
@@ -1,11 +1,3 @@
-/**
- * Copyright (c) Facebook, Inc. and its affiliates. All Rights Reserved.
- *
- * This source code is licensed under the MIT license found in the
- * LICENSE file in the root directory of this source tree.
- *
- *
- */
 'use strict';
 
 Object.defineProperty(exports, '__esModule', {
@@ -67,15 +59,15 @@ function getExposedMethods(workerPath, options) {
   let exposedMethods = options.exposedMethods; // If no methods list is given, try getting it by auto-requiring the module.
 
   if (!exposedMethods) {
-    // $FlowFixMe: This has to be a dynamic require.
     const module = require(workerPath);
 
     exposedMethods = Object.keys(module).filter(
+      // @ts-ignore: no index
       name => typeof module[name] === 'function'
     );
 
     if (typeof module === 'function') {
-      exposedMethods.push('default');
+      exposedMethods = [...exposedMethods, 'default'];
     }
   }
 
@@ -110,6 +102,7 @@ function getExposedMethods(workerPath, options) {
 class JestWorker {
   constructor(workerPath, options) {
     this._options = _objectSpread({}, options);
+    this._ending = false;
     const workerPoolOptions = {
       enableWorkerThreads: this._options.enableWorkerThreads || false,
       forkOptions: this._options.forkOptions || {},
@@ -119,9 +112,17 @@ class JestWorker {
         Math.max(_os().default.cpus().length - 1, 1),
       setupArgs: this._options.setupArgs || []
     };
-    this._workerPool = this._options.WorkerPool
-      ? new this._options.WorkerPool(workerPath, workerPoolOptions)
-      : new _WorkerPool.default(workerPath, workerPoolOptions);
+
+    if (this._options.WorkerPool) {
+      // @ts-ignore: constructor target any?
+      this._workerPool = new this._options.WorkerPool(
+        workerPath,
+        workerPoolOptions
+      );
+    } else {
+      this._workerPool = new _WorkerPool.default(workerPath, workerPoolOptions);
+    }
+
     this._farm = new _Farm.default(
       workerPoolOptions.numWorkers,
       this._workerPool.send.bind(this._workerPool),
@@ -139,7 +140,7 @@ class JestWorker {
 
       if (this.constructor.prototype.hasOwnProperty(name)) {
         throw new TypeError('Cannot define a method called ' + name);
-      } // $FlowFixMe: dynamic extension of the class instance is expected.
+      } // @ts-ignore: dynamic extension of the class instance is expected.
 
       this[name] = this._callFunctionWithArgs.bind(this, name);
     });
diff --git i/packages/jest-worker/build/types.js w/packages/jest-worker/build/types.js
index 9ac9874cf..eb8beb09d 100644
--- i/packages/jest-worker/build/types.js
+++ w/packages/jest-worker/build/types.js
@@ -1,19 +1,18 @@
+'use strict';
+
+Object.defineProperty(exports, '__esModule', {
+  value: true
+});
+exports.PARENT_MESSAGE_SETUP_ERROR = exports.PARENT_MESSAGE_CLIENT_ERROR = exports.PARENT_MESSAGE_OK = exports.CHILD_MESSAGE_END = exports.CHILD_MESSAGE_CALL = exports.CHILD_MESSAGE_INITIALIZE = void 0;
+
 /**
  * Copyright (c) Facebook, Inc. and its affiliates. All Rights Reserved.
  *
  * This source code is licensed under the MIT license found in the
  * LICENSE file in the root directory of this source tree.
- *
- *
  */
-'use strict'; // Because of the dynamic nature of a worker communication process, all messages
+// Because of the dynamic nature of a worker communication process, all messages
 // coming from any of the other processes cannot be typed. Thus, many types
-// include "any" as a flow type, which is (unfortunately) correct here.
-
-Object.defineProperty(exports, '__esModule', {
-  value: true
-});
-exports.PARENT_MESSAGE_SETUP_ERROR = exports.PARENT_MESSAGE_CLIENT_ERROR = exports.PARENT_MESSAGE_OK = exports.CHILD_MESSAGE_END = exports.CHILD_MESSAGE_CALL = exports.CHILD_MESSAGE_INITIALIZE = void 0;
 const CHILD_MESSAGE_INITIALIZE = 0;
 exports.CHILD_MESSAGE_INITIALIZE = CHILD_MESSAGE_INITIALIZE;
 const CHILD_MESSAGE_CALL = 1;
@@ -27,4 +26,5 @@ exports.PARENT_MESSAGE_CLIENT_ERROR = PARENT_MESSAGE_CLIENT_ERROR;
 const PARENT_MESSAGE_SETUP_ERROR = 2;
 exports.PARENT_MESSAGE_SETUP_ERROR = PARENT_MESSAGE_SETUP_ERROR;
 
+// Option objects.
 const EventEmitter = require('events');
diff --git i/packages/jest-worker/build/workers/ChildProcessWorker.js w/packages/jest-worker/build/workers/ChildProcessWorker.js
index 3252e6faa..6abdf5721 100644
--- i/packages/jest-worker/build/workers/ChildProcessWorker.js
+++ w/packages/jest-worker/build/workers/ChildProcessWorker.js
@@ -1,11 +1,3 @@
-/**
- * Copyright (c) Facebook, Inc. and its affiliates. All Rights Reserved.
- *
- * This source code is licensed under the MIT license found in the
- * LICENSE file in the root directory of this source tree.
- *
- *
- */
 'use strict';
 
 Object.defineProperty(exports, '__esModule', {
@@ -23,8 +15,6 @@ function _child_process() {
   return data;
 }
 
-var _types = require('../types');
-
 function _supportsColor() {
   const data = _interopRequireDefault(require('supports-color'));
 
@@ -35,6 +25,8 @@ function _supportsColor() {
   return data;
 }
 
+var _types = require('../types');
+
 function _interopRequireDefault(obj) {
   return obj && obj.__esModule ? obj : {default: obj};
 }
@@ -104,6 +96,7 @@ class ChildProcessWorker {
 
     const child = _child_process().default.fork(
       require.resolve('./processChild'),
+      [],
       _objectSpread(
         {
           cwd: process.cwd(),
@@ -111,7 +104,7 @@ class ChildProcessWorker {
             {},
             process.env,
             {
-              JEST_WORKER_ID: this._options.workerId
+              JEST_WORKER_ID: String(this._options.workerId)
             },
             forceColor
           ),
@@ -150,10 +143,7 @@ class ChildProcessWorker {
     }
   }
 
-  onMessage(
-    response
-    /* Should be ParentMessage */
-  ) {
+  onMessage(response) {
     let error;
 
     switch (response[0]) {
@@ -166,16 +156,16 @@ class ChildProcessWorker {
         error = response[4];
 
         if (error != null && typeof error === 'object') {
-          const extra = error;
+          const extra = error; // @ts-ignore: no index
+
           const NativeCtor = global[response[1]];
           const Ctor = typeof NativeCtor === 'function' ? NativeCtor : Error;
-          error = new Ctor(response[2]); // $FlowFixMe: adding custom properties to errors.
-
+          error = new Ctor(response[2]);
           error.type = response[1];
           error.stack = response[3];
 
           for (const key in extra) {
-            // $FlowFixMe: adding custom properties to errors.
+            // @ts-ignore: adding custom properties to errors.
             error[key] = extra[key];
           }
         }
@@ -185,7 +175,7 @@ class ChildProcessWorker {
         break;
 
       case _types.PARENT_MESSAGE_SETUP_ERROR:
-        error = new Error('Error when calling setup: ' + response[2]); // $FlowFixMe: adding custom properties to errors.
+        error = new Error('Error when calling setup: ' + response[2]); // @ts-ignore: adding custom properties to errors.
 
         error.type = response[1];
         error.stack = response[3];
diff --git i/packages/jest-worker/build/workers/NodeThreadsWorker.js w/packages/jest-worker/build/workers/NodeThreadsWorker.js
index 5a08a8972..d91ed0aa8 100644
--- i/packages/jest-worker/build/workers/NodeThreadsWorker.js
+++ w/packages/jest-worker/build/workers/NodeThreadsWorker.js
@@ -1,11 +1,3 @@
-/**
- * Copyright (c) Facebook, Inc. and its affiliates. All Rights Reserved.
- *
- * This source code is licensed under the MIT license found in the
- * LICENSE file in the root directory of this source tree.
- *
- *
- */
 'use strict';
 
 Object.defineProperty(exports, '__esModule', {
@@ -13,8 +5,6 @@ Object.defineProperty(exports, '__esModule', {
 });
 exports.default = void 0;
 
-var _types = require('../types');
-
 function _path() {
   const data = _interopRequireDefault(require('path'));
 
@@ -25,6 +15,18 @@ function _path() {
   return data;
 }
 
+function _worker_threads() {
+  const data = require('worker_threads');
+
+  _worker_threads = function _worker_threads() {
+    return data;
+  };
+
+  return data;
+}
+
+var _types = require('../types');
+
 function _interopRequireDefault(obj) {
   return obj && obj.__esModule ? obj : {default: obj};
 }
@@ -61,10 +63,6 @@ function _defineProperty(obj, key, value) {
   return obj;
 }
 
-// $FlowFixMe: Flow doesn't know about experimental features of Node
-const _require = require('worker_threads'),
-  Worker = _require.Worker;
-
 class ExperimentalWorker {
   constructor(options) {
     this._options = options;
@@ -72,7 +70,7 @@ class ExperimentalWorker {
   }
 
   initialize() {
-    this._worker = new Worker(
+    this._worker = new (_worker_threads()).Worker(
       _path().default.resolve(__dirname, './threadChild.js'),
       {
         eval: false,
@@ -82,7 +80,7 @@ class ExperimentalWorker {
           {
             cwd: process.cwd(),
             env: _objectSpread({}, process.env, {
-              JEST_WORKER_ID: this._options.workerId
+              JEST_WORKER_ID: String(this._options.workerId)
             }),
             // Suppress --debug / --inspect flags while preserving others (like --harmony).
             execArgv: process.execArgv.filter(
@@ -124,10 +122,7 @@ class ExperimentalWorker {
     }
   }
 
-  onMessage(
-    response
-    /* Should be ParentMessage */
-  ) {
+  onMessage(response) {
     let error;
 
     switch (response[0]) {
@@ -140,16 +135,16 @@ class ExperimentalWorker {
         error = response[4];
 
         if (error != null && typeof error === 'object') {
-          const extra = error;
+          const extra = error; // @ts-ignore: no index
+
           const NativeCtor = global[response[1]];
           const Ctor = typeof NativeCtor === 'function' ? NativeCtor : Error;
-          error = new Ctor(response[2]); // $FlowFixMe: adding custom properties to errors.
-
+          error = new Ctor(response[2]);
           error.type = response[1];
           error.stack = response[3];
 
           for (const key in extra) {
-            // $FlowFixMe: adding custom properties to errors.
+            // @ts-ignore: no index
             error[key] = extra[key];
           }
         }
@@ -159,7 +154,7 @@ class ExperimentalWorker {
         break;
 
       case _types.PARENT_MESSAGE_SETUP_ERROR:
-        error = new Error('Error when calling setup: ' + response[2]); // $FlowFixMe: adding custom properties to errors.
+        error = new Error('Error when calling setup: ' + response[2]); // @ts-ignore: adding custom properties to errors.
 
         error.type = response[1];
         error.stack = response[3];
diff --git i/packages/jest-worker/build/workers/processChild.js w/packages/jest-worker/build/workers/processChild.js
index 409e0c673..222242c86 100644
--- i/packages/jest-worker/build/workers/processChild.js
+++ w/packages/jest-worker/build/workers/processChild.js
@@ -1,11 +1,3 @@
-/**
- * Copyright (c) Facebook, Inc. and its affiliates. All Rights Reserved.
- *
- * This source code is licensed under the MIT license found in the
- * LICENSE file in the root directory of this source tree.
- *
- *
- */
 'use strict';
 
 var _types = require('../types');
@@ -118,7 +110,6 @@ function reportError(error, type) {
 }
 
 function end() {
-  // $FlowFixMe: This has to be a dynamic require.
   const main = require(file);
 
   if (!main.teardown) {
@@ -134,7 +125,6 @@ function exitProcess() {
 }
 
 function execMethod(method, args) {
-  // $FlowFixMe: This has to be a dynamic require.
   const main = require(file);
 
   let fn;
diff --git i/packages/jest-worker/build/workers/threadChild.js w/packages/jest-worker/build/workers/threadChild.js
index 346510ed0..941c69992 100644
--- i/packages/jest-worker/build/workers/threadChild.js
+++ w/packages/jest-worker/build/workers/threadChild.js
@@ -1,15 +1,5 @@
-/**
- * Copyright (c) Facebook, Inc. and its affiliates. All Rights Reserved.
- *
- * This source code is licensed under the MIT license found in the
- * LICENSE file in the root directory of this source tree.
- *
- *
- */
 'use strict';
 
-var _types = require('../types');
-
 function _worker_threads() {
   const data = require('worker_threads');
 
@@ -20,6 +10,8 @@ function _worker_threads() {
   return data;
 }
 
+var _types = require('../types');
+
 function _objectSpread(target) {
   for (var i = 1; i < arguments.length; i++) {
     var source = arguments[i] != null ? arguments[i] : {};
@@ -55,11 +47,6 @@ function _defineProperty(obj, key, value) {
 let file = null;
 let setupArgs = [];
 let initialized = false;
-/* eslint-disable import/no-unresolved */
-// $FlowFixMe: Flow doesn't support experimental node modules
-
-/* eslint-enable import/no-unresolved */
-
 /**
  * This file is a small bootstrapper for workers. It sets up the communication
  * between the worker and the parent process, interpreting parent messages and
@@ -73,6 +60,7 @@ let initialized = false;
  * If an invalid message is detected, the child will exit (by throwing) with a
  * non-zero exit code.
  */
+
 _worker_threads().parentPort.on('message', request => {
   switch (request[0]) {
     case _types.CHILD_MESSAGE_INITIALIZE:
@@ -126,13 +114,12 @@ function reportError(error, type) {
     type,
     error.constructor && error.constructor.name,
     error.message,
-    error.stack, // $FlowFixMe: this is safe to just inherit from Object.
+    error.stack,
     typeof error === 'object' ? _objectSpread({}, error) : error
   ]);
 }
 
 function end() {
-  // $FlowFixMe: This has to be a dynamic require.
   const main = require(file);
 
   if (!main.teardown) {
@@ -148,7 +135,6 @@ function exitProcess() {
 }
 
 function execMethod(method, args) {
-  // $FlowFixMe: This has to be a dynamic require.
   const main = require(file);
 
   let fn;

Test plan

Green CI

Copy link
Contributor

@jeysal jeysal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tried really hard to stay focused despite the huge diff, and found at least a few nits / questions 😅

// Because of the dynamic nature of a worker communication process, all messages
// coming from any of the other processes cannot be typed. Thus, many types
// include "any" as a flow type, which is (unfortunately) correct here.
// include "unknown" as a TS type, which is (unfortunately) correct here.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Eagle eye 😄

};

export type ChildMessageInitialize = [
typeof CHILD_MESSAGE_INITIALIZE, // type
boolean, // processed
string, // file
?Array<mixed>, // setupArgs
?MessagePort, // MessagePort
Array<unknown> | undefined, // setupArgs
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Intentionally left out null? I see ChildMessageInitialize being assigned from any and usually we have nulls in this package

Edit: I guess for an array leaving out the element makes more sense though. On a side note, wondering if TS differentiates between [string | undefined] and [string] | never[]. Would matter for inferring the parameter type of forEach 😄

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yea, mostly because it didn't complain. the less allowed values the better, IMO. Easy enough to add more allowed types at some point if there's value in it

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tried out the side note thing in the TS playground btw, looks like it does differentiate 😎 not relevant for our use case though

packages/jest-worker/src/types.ts Outdated Show resolved Hide resolved
@@ -63,7 +61,7 @@ it('passes fork options down to child_process.fork, adding the defaults', () =>
});

expect(childProcess.fork.mock.calls[0][0]).toBe(child);
expect(childProcess.fork.mock.calls[0][1]).toEqual({
expect(childProcess.fork.mock.calls[0][2]).toEqual({
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wait what :D

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See https://github.com/facebook/jest/pull/7853/files#diff-f9414f0ef9e6230a02ef6a47be731f55R55

The typings doesn't allow skipping the args array. It's not mentioned in the docs, so probably fine

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, totally missed the added [] ^^

Co-Authored-By: SimenB <sbekkhus91@gmail.com>
@github-actions
Copy link

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.
Please note this issue tracker is not a help forum. We recommend using StackOverflow or our discord channel for questions.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 12, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants