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-environment-node to TypeScript #7985

Merged
merged 8 commits into from
Feb 26, 2019

Conversation

lirbank
Copy link
Contributor

@lirbank lirbank commented Feb 25, 2019

@SimenB here is a first migration of jest-environment-node.

I can't seem to import FakeTimers via the default module export. If I do this:

import {installCommonGlobals, FakeTimers} from 'jest-util';

I get Cannot find name 'FakeTimers'.ts(2304).

However, this works (albeit not ideal):

import {installCommonGlobals} from 'jest-util';
import FakeTimers from 'jest-util/build/FakeTimers';

I think it has to do with the use of NodeJS-style export in jest-utils (instead of ES6 style export {} (no equals sign)), but not sure:

https://github.com/facebook/jest/blob/51817fd79a3744078a82861f3763dc346ca0cbd6/packages/jest-util/src/index.ts#L30-L52

How should I deal with this you think?

diff --git a/packages/jest-environment-node/build/index.js b/packages/jest-environment-node/build/index.js
index 36c28a2bc..e107def56 100644
--- a/packages/jest-environment-node/build/index.js
+++ b/packages/jest-environment-node/build/index.js
@@ -10,6 +10,16 @@ function _vm() {
   return data;
 }
 
+function _jestMock() {
+  const data = require('jest-mock');
+
+  _jestMock = function _jestMock() {
+    return data;
+  };
+
+  return data;
+}
+
 function _jestUtil() {
   const data = require('jest-util');
 
@@ -20,10 +30,10 @@ function _jestUtil() {
   return data;
 }
 
-function _jestMock() {
-  const data = _interopRequireDefault(require('jest-mock'));
+function _FakeTimers() {
+  const data = _interopRequireDefault(require('jest-util/build/FakeTimers'));
 
-  _jestMock = function _jestMock() {
+  _FakeTimers = function _FakeTimers() {
     return data;
   };
 
@@ -39,8 +49,6 @@ function _interopRequireDefault(obj) {
  *
  * This source code is licensed under the MIT license found in the
  * LICENSE file in the root directory of this source tree.
- *
- *
  */
 class NodeEnvironment {
   constructor(config) {
@@ -65,7 +73,7 @@ class NodeEnvironment {
     }
 
     (0, _jestUtil().installCommonGlobals)(global, config.globals);
-    this.moduleMocker = new (_jestMock()).default.ModuleMocker(global);
+    this.moduleMocker = new (_jestMock()).ModuleMocker(global);
 
     const timerIdToRef = id => ({
       id,
@@ -79,13 +87,13 @@ class NodeEnvironment {
       }
     });
 
-    const timerRefToId = timer => (timer && timer.id) || null;
+    const timerRefToId = timer => (timer && timer.id) || undefined;
 
     const timerConfig = {
       idToRef: timerIdToRef,
       refToId: timerRefToId
     };
-    this.fakeTimers = new (_jestUtil()).FakeTimers({
+    this.fakeTimers = new (_FakeTimers()).default({
       config,
       global,
       moduleMocker: this.moduleMocker,

@facebook-github-bot
Copy link
Contributor

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need the corporate CLA signed.

If you have received this in error or have any questions, please contact us at [email protected]. Thanks!

@facebook-github-bot
Copy link
Contributor

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

@lirbank lirbank changed the title Jest environment jest-environment-node Feb 25, 2019
@SimenB
Copy link
Member

SimenB commented Feb 25, 2019

Thanks @lirbank! That's the same issue I had in @jest/environment. I'll extract the fake timer into its own package, which should fix its export

EDIT: #7987

@lirbank
Copy link
Contributor Author

lirbank commented Feb 26, 2019

Ok, got it! That's what you were referring to here #7964 (review) I take?

I'll wait for #7987 to be merged and then I'll finish up this PR.

@SimenB
Copy link
Member

SimenB commented Feb 26, 2019

Yeah!

@@ -88,7 +84,7 @@ class NodeEnvironment {
}

// Disabling rule as return type depends on script's return type.
runScript(script: Script): ?any {
runScript(script: Script): any | null | undefined {
Copy link

Choose a reason for hiding this comment

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

null | undefined is redundant since this is already any.

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

null | undefined is redundant since this is already any.

I agree, I was just trying to refactor without changing to much. I'll try to change it into a proper type instead of any.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@SimenB WRT the any type here. I've just logged what is returned from runScript() and it looks like this (in a separate app/jest setup):

{ 'Object.<anonymous>': [Function: Object.<anonymous>] }

JSON stringified it is an empty object:

'{}'

I'm not sure what to put as the return type here. I just don't have enough understanding of what is being done in the method. Just keep it as any? Or an unsealed object {} | undefined | null? Open to suggestions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Never mind, I'll follow your diff below.

@SimenB
Copy link
Member

SimenB commented Feb 26, 2019

@lirbank merged

@codecov-io
Copy link

Codecov Report

Merging #7985 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #7985      +/-   ##
==========================================
+ Coverage   64.54%   64.54%   +<.01%     
==========================================
  Files         257      257              
  Lines       10074    10076       +2     
  Branches     1587     1586       -1     
==========================================
+ Hits         6502     6504       +2     
- Misses       3229     3230       +1     
+ Partials      343      342       -1
Impacted Files Coverage Δ
packages/jest-environment-jsdom/src/index.js 39.02% <ø> (ø) ⬆️
packages/jest-environment-node/src/index.js 62.06% <ø> (ø) ⬆️
packages/expect/src/jasmineUtils.ts 76.03% <100%> (+0.82%) ⬆️
packages/jest-fake-timers/src/jestFakeTimers.ts 88.17% <100%> (ø)
packages/jest-util/src/setGlobal.ts 0% <0%> (-100%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 51817fd...57a3567. Read the comment docs.

@SimenB
Copy link
Member

SimenB commented Feb 26, 2019

Opened up #7988 as well, which is needed here. If I rebase this branch on top of master, I can apply this diff:

diff --git i/packages/jest-environment-node/src/index.ts w/packages/jest-environment-node/src/index.ts
index 28473c6c5..3a9b5672e 100644
--- i/packages/jest-environment-node/src/index.ts
+++ w/packages/jest-environment-node/src/index.ts
@@ -9,6 +9,7 @@ import vm, {Script, Context} from 'vm';
 import {Global, Config} from '@jest/types';
 import {ModuleMocker} from 'jest-mock';
 import {installCommonGlobals} from 'jest-util';
+import {JestEnvironment} from '@jest/environment';
 import {JestFakeTimers as FakeTimers} from '@jest/fake-timers';
 
 type Timer = {
@@ -17,11 +18,11 @@ type Timer = {
   unref: () => Timer;
 };
 
-class NodeEnvironment {
-  context?: Context | null;
-  fakeTimers?: FakeTimers<Timer> | null;
-  global?: Global.Global | null;
-  moduleMocker?: ModuleMocker | null;
+class NodeEnvironment implements JestEnvironment {
+  context: Context | null;
+  fakeTimers: FakeTimers<Timer> | null;
+  global: Global.Global;
+  moduleMocker: ModuleMocker | null;
 
   constructor(config: Config.ProjectConfig) {
     this.context = vm.createContext();
@@ -70,11 +71,11 @@ class NodeEnvironment {
     });
   }
 
-  setup(): Promise<void> {
+  setup() {
     return Promise.resolve();
   }
 
-  teardown(): Promise<void> {
+  teardown() {
     if (this.fakeTimers) {
       this.fakeTimers.dispose();
     }
@@ -83,8 +84,8 @@ class NodeEnvironment {
     return Promise.resolve();
   }
 
-  // Disabling rule as return type depends on script's return type.
-  runScript(script: Script): any | null | undefined {
+  // TS infers the return type to be `any`, since that's what `runInContext` returns.
+  runScript(script: Script): ReturnType<JestEnvironment['runScript']> {
     if (this.context) {
       return script.runInContext(this.context);
     }
@@ -92,4 +93,4 @@ class NodeEnvironment {
   }
 }
 
-module.exports = NodeEnvironment;
+export = NodeEnvironment;

and everything seems to work 🙂

@lirbank
Copy link
Contributor Author

lirbank commented Feb 26, 2019

@SimenB do you want NodeEnvironment to implement the JestEnvironment? I've tried it and would need to update the typings for the fakeTimers, global and moduleMocker members for it to work.

@@ -60,7 +54,8 @@ class NodeEnvironment {
},
});

const timerRefToId = (timer: Timer): ?number => (timer && timer.id) || null;
const timerRefToId = (timer: Timer): number | undefined =>
(timer && timer.id) || undefined;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Please note that I've changed the possible return type from null to undefined here. FakeTimers does not like the timerConfig if timerRefToId returns null (but undefined is OK).

Copy link
Member

Choose a reason for hiding this comment

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

That's fine. Happy to get rid of the null!

@SimenB
Copy link
Member

SimenB commented Feb 26, 2019

@SimenB do you want NodeEnvironment to implement the JestEnvironment? I've tried it and would need to update the typings for the fakeTimers, global and moduleMocker members for it to work.

Yes 🙂

@lirbank
Copy link
Contributor Author

lirbank commented Feb 26, 2019

@SimenB

@SimenB do you want NodeEnvironment to implement the JestEnvironment? I've tried it and would need to update the typings for the fakeTimers, global and moduleMocker members for it to work.

Never mind, I see your diff above now. I'll update this PR. Just a sec.

@SimenB
Copy link
Member

SimenB commented Feb 26, 2019

Thanks! Could you update the changelog and mark this PR ready for review (I'm not allowed to merge while it's a draft 🙂)?

@lirbank
Copy link
Contributor Author

lirbank commented Feb 26, 2019

Updated git diff:

diff --git a/packages/jest-environment-node/build/index.js b/packages/jest-environment-node/build/index.js
index 1c59cb7b7..25d56a387 100644
--- a/packages/jest-environment-node/build/index.js
+++ b/packages/jest-environment-node/build/index.js
@@ -10,10 +10,10 @@ function _vm() {
   return data;
 }
 
-function _fakeTimers() {
-  const data = require('@jest/fake-timers');
+function _jestMock() {
+  const data = require('jest-mock');
 
-  _fakeTimers = function _fakeTimers() {
+  _jestMock = function _jestMock() {
     return data;
   };
 
@@ -30,10 +30,10 @@ function _jestUtil() {
   return data;
 }
 
-function _jestMock() {
-  const data = _interopRequireDefault(require('jest-mock'));
+function _fakeTimers() {
+  const data = require('@jest/fake-timers');
 
-  _jestMock = function _jestMock() {
+  _fakeTimers = function _fakeTimers() {
     return data;
   };
 
@@ -49,8 +49,6 @@ function _interopRequireDefault(obj) {
  *
  * This source code is licensed under the MIT license found in the
  * LICENSE file in the root directory of this source tree.
- *
- *
  */
 class NodeEnvironment {
   constructor(config) {
@@ -75,7 +73,7 @@ class NodeEnvironment {
     }
 
     (0, _jestUtil().installCommonGlobals)(global, config.globals);
-    this.moduleMocker = new (_jestMock()).default.ModuleMocker(global);
+    this.moduleMocker = new (_jestMock()).ModuleMocker(global);
 
     const timerIdToRef = id => ({
       id,
@@ -89,7 +87,7 @@ class NodeEnvironment {
       }
     });
 
-    const timerRefToId = timer => (timer && timer.id) || null;
+    const timerRefToId = timer => (timer && timer.id) || undefined;
 
     const timerConfig = {
       idToRef: timerIdToRef,
@@ -115,7 +113,8 @@ class NodeEnvironment {
     this.context = null;
     this.fakeTimers = null;
     return Promise.resolve();
-  } // Disabling rule as return type depends on script's return type.
+  } // TS infers the return type to be `any`, since that's what `runInContext`
+  // returns.
 
   runScript(script) {
     if (this.context) {

@lirbank lirbank marked this pull request as ready for review February 26, 2019 18:55
@lirbank lirbank changed the title jest-environment-node chore: migrate jest-environment-node to TypeScript Feb 26, 2019
@SimenB SimenB merged commit 3bd384b into jestjs:master Feb 26, 2019
@SimenB
Copy link
Member

SimenB commented Feb 26, 2019

Thanks!

@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.

5 participants