chore: migrate jest-environment-jsdom to TypeScript #8003

lirbank · 2019-02-27T18:36:36Z

@SimenB I have a bunch of questions. I'll make a few at the time to see if it clears to me as we go (see inline review comments in a sec).

Built diff:

diff --git c/packages/jest-environment-jsdom/build/index.js w/packages/jest-environment-jsdom/build/index.js
index 8cd500cf4..718039095 100644
--- c/packages/jest-environment-jsdom/build/index.js
+++ w/packages/jest-environment-jsdom/build/index.js
@@ -1,29 +1,29 @@
 'use strict';
 
-function _fakeTimers() {
-  const data = require('@jest/fake-timers');
+function _jestUtil() {
+  const data = require('jest-util');
 
-  _fakeTimers = function _fakeTimers() {
+  _jestUtil = function _jestUtil() {
     return data;
   };
 
   return data;
 }
 
-function _jestUtil() {
-  const data = require('jest-util');
+function _jestMock() {
+  const data = _interopRequireDefault(require('jest-mock'));
 
-  _jestUtil = function _jestUtil() {
+  _jestMock = function _jestMock() {
     return data;
   };
 
   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;
   };
 
@@ -76,7 +76,13 @@ function _defineProperty(obj, key, value) {
   return obj;
 }
 
+function isWin(globals) {
+  return globals.document !== undefined;
+} // A lot of the globals expected by other APIs are `NodeJS.Global` and not
+// `Window`, so we need to cast here and there
+
 class JSDOMEnvironment {
+  // @ts-ignore
   constructor(config, options = {}) {
     this.dom = new (_jsdom()).JSDOM(
       '<!DOCTYPE html>',
@@ -92,7 +98,11 @@ class JSDOMEnvironment {
         config.testEnvironmentOptions
       )
     );
-    const global = (this.global = this.dom.window.document.defaultView); // Node's error-message stack size is limited at 10, but it's pretty useful
+    const global = (this.global = this.dom.window.document.defaultView);
+
+    if (!global) {
+      throw new Error('JSDOM did not return a Window object');
+    } // Node's error-message stack size is limited at 10, but it's pretty useful
     // to see more than that when a test fails.
 
     this.global.Error.stackTraceLimit = 100;
@@ -111,20 +121,20 @@ class JSDOMEnvironment {
     const originalRemoveListener = global.removeEventListener;
     let userErrorListenerCount = 0;
 
-    global.addEventListener = function(name) {
-      if (name === 'error') {
+    global.addEventListener = function(...args) {
+      if (args[0] === 'error') {
         userErrorListenerCount++;
       }
 
-      return originalAddListener.apply(this, arguments);
+      return originalAddListener.apply(this, args);
     };
 
-    global.removeEventListener = function(name) {
-      if (name === 'error') {
+    global.removeEventListener = function(...args) {
+      if (args[0] === 'error') {
         userErrorListenerCount--;
       }
 
-      return originalRemoveListener.apply(this, arguments);
+      return originalRemoveListener.apply(this, args);
     };
 
     this.moduleMocker = new (_jestMock()).default.ModuleMocker(global);
@@ -134,7 +144,7 @@ class JSDOMEnvironment {
     };
     this.fakeTimers = new (_fakeTimers()).JestFakeTimers({
       config,
-      global,
+      global: global,
       moduleMocker: this.moduleMocker,
       timerConfig
     });
@@ -150,14 +160,17 @@ class JSDOMEnvironment {
     }
 
     if (this.global) {
-      if (this.errorEventListener) {
+      if (this.errorEventListener && isWin(this.global)) {
         this.global.removeEventListener('error', this.errorEventListener);
       } // Dispose "document" to prevent "load" event from triggering.
 
       Object.defineProperty(this.global, 'document', {
         value: null
       });
-      this.global.close();
+
+      if (isWin(this.global)) {
+        this.global.close();
+      }
     }
 
     this.errorEventListener = null;

This reverts commit bd4e070.

SimenB · 2019-02-27T18:37:15Z

(see inline review comments in a sec).

👀

I'll hold off on commenting just yet then 😀

lirbank · 2019-02-27T18:40:07Z

(see inline review comments in a sec).

👀

I'll hold off on commenting just yet then 😀

Yes, please, the code is FULL or errors. At this point I just need some guidance.

lirbank · 2019-02-27T18:40:59Z

Q1: defaultView returns Window | null which is not compatible with Global.Global. It looks like we're overwriting this.global with a window object, which seems odd to me?

https://github.com/facebook/jest/blob/4e8e6380372676f3381ad589046dba4ef53e45fa/packages/jest-environment-jsdom/src/index.ts#L31

lirbank · 2019-02-27T18:43:47Z

Q2: To be able to reset this.global, we need to change it's type to accept null. This is currently not compatible with JestEnvironment. Should I update JestEnvironment?

Q2b: Also, should we also set this.global to null on teardown in jest-environment-node?

https://github.com/facebook/jest/blob/4e8e6380372676f3381ad589046dba4ef53e45fa/packages/jest-environment-jsdom/src/index.ts#L95

packages/jest-environment-jsdom/package.json

SimenB · 2019-02-27T18:47:05Z

Q1: defaultView returns Window | null which is not compatible with Global.Global. It looks like we're overwriting this.global with a window object, which seems odd to me?

Yeah, we have a TODO it it should accept Window there. It probably should, but that will break other stuff. Just add a @ts-ignore above the assignment for now

Q2: To be able to reset this.global, we need to change it's type to accept null. This is currently not compatible with JestEnvironment. Should I update JestEnvironment?

Yeah, feel free to make it nullable

Q2b: Also, should we also set this.global to null on teardown in jest-environment-node?

Yeah, I think that makes sense - easier for GC to pick it up then

This reverts commit 3c4486f.

lirbank · 2019-02-27T18:52:07Z

Q2b: Also, should we also set this.global to null on teardown in jest-environment-node?

Yeah, I think that makes sense - easier for GC to pick it up then

Do you prefer if this goes into a separate PR, since it's another package? Or doesn't matter?

SimenB · 2019-02-27T18:53:07Z

Do you prefer if this goes into a separate PR, since it's another package? Or doesn't matter?

Doesn't matter

packages/jest-environment-jsdom/src/index.ts

lirbank · 2019-02-27T23:28:26Z

@SimenB please take a look when you have the time, and let me know what you think.

There are quite a few discrepancies between types that technically should match. I have solved these with any types and @ts-ignore. Not a big fan of hacking the type system, but wanted to get a working version quickly. Happy to update as you see fit.

I've changed the return type of runScript to unknown. While I like the idea of using unknown over any I am not sure this is in the right direction, LMK what you think.

packages/jest-environment-jsdom/src/index.ts

SimenB · 2019-02-28T00:17:12Z

packages/jest-environment-jsdom/src/index.ts

+    if (this.dom) {
+      // Explicitly returning `unknown` since `runVMScript` currently returns
+      // `void`, which is wrong
+      return this.dom.runVMScript(script) as unknown;


Suggested change

return this.dom.runVMScript(script) as unknown;

return this.dom.runVMScript(script) as any;

We know what the type here will be, no need to assert it.

(it'll have the EVAL_RESULT_VARIABLE object thing due to https://github.com/facebook/jest/blob/438a178c8addf2806bebf44726d080921ad9984f/packages/jest-transform/src/ScriptTransformer.ts#L559-L565)

We do need to assert it since runVMScript() returns void, which is not true, and runScript() is also expected to receive {[ScriptTransformer.EVAL_RESULT_VARIABLE]: ModuleWrapper} | null (via the JestEnvironment interface). I have a new solution for this, committing in a sec.

We know what the type here will be

This contrasts with #8003 (comment) - we can't both set the return type to the EVAL_RESULT_VARIABLE thing, AND the return value of the provided script... Sorry, I'm a bit confused here.

If we want the return type of runScript to be the the same as the return type of the provided script, we could use a generic to let the user of jest-environment-jsdom set the return type? Just an idea. But I feel there is something I am missing here... :)

The return value of the provided script is, 100%, the EVAL_RESULT_VARIABLE thing (unless the environment has been torn down, in which cse it will be null). We construct the script ourselves. The function assigned to EVAL_RESULT_VARIABLE, we do not know what will return when invoked (that's the unknown part), but we don't care about that result

If we want the return type of runScript to be the the same as the return type of the provided script, we could use a generic to let the user of jest-environment-jsdom set the return type? Just an idea. But I feel there is something I am missing here... :)

I don't think that's needed - the script we get is constructed from a string source we've read (and potentially transformed) from disk. So we cannot statically know what it will return

packages/jest-environment/src/index.ts

packages/jest-runtime/src/index.ts

SimenB · 2019-02-28T18:27:39Z

@lirbank I pushed a commit that removes some of the comments to reduce the diff - this allows git to see it's a file rename and retain blame history

SimenB

This was a tough one! I think it's good to go now, though 🙂 I added a diff to the OP which looks good. The generated typings file also looks good. Will merge if CI is happy.

Thank you so much for sticking with me through this review!

SimenB · 2019-02-28T18:44:13Z

Also, I've opened up a PR to definitelytyped to fix the JSDOM type: https://github.com/DefinitelyTyped/DefinitelyTyped/pull/33476/files

CI is passing! :🎉@lirbank could you mark this PR ready for review? I'm not allowed to merge when it's a draft 🙂

github-actions · 2021-05-12T00:06:40Z

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.

Mikael Lirbank and others added 15 commits February 25, 2019 09:38

Rename files

c857e4c

Migrate jest-environment-node

0cfe10b

Minor

57a3567

Merge branch 'master' into jest-environment

9b279a6

Use new fake timers package

2c3afe4

Implement JestEnvironment

ae3c45c

before migration

bd4e070

Update CHANGELOG.md

71048f6

add to tsconfigs

cc4eacb

Merge remote-tracking branch 'upstream/master'

b3effe8

Merge branch 'master' into jest-environment

9cf040b

Revert "before migration"

a40ea8d

This reverts commit bd4e070.

Merge branch 'master' into jest-environment

b215884

Rename files

0a9e4c4

Migrate to TS (WIP)

4e8e638

facebook-github-bot added the cla signed label Feb 27, 2019

SimenB added this to the TypeScript Migration milestone Feb 27, 2019

SimenB mentioned this pull request Feb 27, 2019

☂️TypeScript migration tracking issue #7807

Closed

SimenB reviewed Feb 27, 2019

View reviewed changes

packages/jest-environment-jsdom/package.json Outdated Show resolved Hide resolved

Mikael Lirbank added 3 commits February 27, 2019 10:48

Update package.json

3c4486f

Revert "Update package.json"

d50560b

This reverts commit 3c4486f.

Update package.json

3968bc4

runScript should always return null (never void)

6105073

SimenB reviewed Feb 27, 2019

View reviewed changes

packages/jest-environment-jsdom/src/index.ts Outdated Show resolved Hide resolved

Mikael Lirbank added 5 commits February 27, 2019 13:40

Migrate to TS (WIP)

5ffc083

Migrate mocks

f45cc88

Change runScript return type to unknown

1f9081a

Migrate jest-environment-jsdom

f1ed385

Comments

e162e48

Update changelog and jest-config tsconfig.json

eebb6f8

SimenB requested changes Feb 28, 2019

View reviewed changes

Mikael Lirbank and others added 6 commits February 28, 2019 09:09

Merge remote-tracking branch 'upstream/master'

a743367

Merge branch 'master' into jest-environment

3beb007

Update return type of runScript()

5fb6bfd

Update global assignment of Window

58fc28b

Merge branch 'master' into jest-environment

c7c84f7

tweaks to reduce diff

06539b1

SimenB approved these changes Feb 28, 2019

View reviewed changes

lirbank marked this pull request as ready for review February 28, 2019 19:18

SimenB merged commit de52b48 into jestjs:master Feb 28, 2019

lirbank deleted the jest-environment branch February 28, 2019 19:24

snyk-bot mentioned this pull request Dec 15, 2019

[Snyk] Upgrade babel-jest from 24.0.0 to 24.9.0 jakeherp/burger-builder#14

Open

This was referenced Mar 21, 2020

[Snyk] Upgrade: jest, jest-cli lwojcik/sc2pte-panel-frontend#88

Closed

[Snyk] Upgrade jest from 24.0.0 to 24.9.0 DavidKindler/burger-builder#47

Merged

github-actions bot locked as resolved and limited conversation to collaborators May 12, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: migrate jest-environment-jsdom to TypeScript #8003

chore: migrate jest-environment-jsdom to TypeScript #8003

lirbank commented Feb 27, 2019 •

edited by SimenB

Loading

SimenB commented Feb 27, 2019 •

edited

Loading

lirbank commented Feb 27, 2019

lirbank commented Feb 27, 2019

lirbank commented Feb 27, 2019

SimenB commented Feb 27, 2019

lirbank commented Feb 27, 2019

SimenB commented Feb 27, 2019

lirbank commented Feb 27, 2019

SimenB Feb 28, 2019

lirbank Feb 28, 2019

lirbank Feb 28, 2019 •

edited

Loading

SimenB Feb 28, 2019 •

edited

Loading

SimenB commented Feb 28, 2019

SimenB left a comment

SimenB commented Feb 28, 2019

github-actions bot commented May 12, 2021

	return this.dom.runVMScript(script) as unknown;
	return this.dom.runVMScript(script) as any;

chore: migrate jest-environment-jsdom to TypeScript #8003

chore: migrate jest-environment-jsdom to TypeScript #8003

Conversation

lirbank commented Feb 27, 2019 • edited by SimenB Loading

SimenB commented Feb 27, 2019 • edited Loading

lirbank commented Feb 27, 2019

lirbank commented Feb 27, 2019

lirbank commented Feb 27, 2019

SimenB commented Feb 27, 2019

lirbank commented Feb 27, 2019

SimenB commented Feb 27, 2019

lirbank commented Feb 27, 2019

SimenB Feb 28, 2019

Choose a reason for hiding this comment

lirbank Feb 28, 2019

Choose a reason for hiding this comment

lirbank Feb 28, 2019 • edited Loading

Choose a reason for hiding this comment

SimenB Feb 28, 2019 • edited Loading

Choose a reason for hiding this comment

SimenB commented Feb 28, 2019

SimenB left a comment

Choose a reason for hiding this comment

SimenB commented Feb 28, 2019

github-actions bot commented May 12, 2021

lirbank commented Feb 27, 2019 •

edited by SimenB

Loading

SimenB commented Feb 27, 2019 •

edited

Loading

lirbank Feb 28, 2019 •

edited

Loading

SimenB Feb 28, 2019 •

edited

Loading