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-jsdom to TypeScript #8003

Merged
merged 31 commits into from
Feb 28, 2019
Merged
Show file tree
Hide file tree
Changes from 25 commits
Commits
Show all changes
31 commits
Select commit Hold shift + click to select a range
c857e4c
Rename files
Feb 25, 2019
0cfe10b
Migrate jest-environment-node
Feb 25, 2019
57a3567
Minor
Feb 25, 2019
9b279a6
Merge branch 'master' into jest-environment
Feb 26, 2019
2c3afe4
Use new fake timers package
Feb 26, 2019
ae3c45c
Implement JestEnvironment
Feb 26, 2019
bd4e070
before migration
Feb 26, 2019
71048f6
Update CHANGELOG.md
Feb 26, 2019
cc4eacb
add to tsconfigs
SimenB Feb 26, 2019
b3effe8
Merge remote-tracking branch 'upstream/master'
Feb 27, 2019
9cf040b
Merge branch 'master' into jest-environment
Feb 27, 2019
a40ea8d
Revert "before migration"
Feb 27, 2019
b215884
Merge branch 'master' into jest-environment
Feb 27, 2019
0a9e4c4
Rename files
Feb 27, 2019
4e8e638
Migrate to TS (WIP)
Feb 27, 2019
3c4486f
Update package.json
Feb 27, 2019
d50560b
Revert "Update package.json"
Feb 27, 2019
3968bc4
Update package.json
Feb 27, 2019
6105073
runScript should always return `null` (never `void`)
Feb 27, 2019
5ffc083
Migrate to TS (WIP)
Feb 27, 2019
f45cc88
Migrate mocks
Feb 27, 2019
1f9081a
Change runScript return type to unknown
Feb 27, 2019
f1ed385
Migrate jest-environment-jsdom
Feb 27, 2019
e162e48
Comments
Feb 27, 2019
eebb6f8
Update changelog and jest-config tsconfig.json
Feb 27, 2019
a743367
Merge remote-tracking branch 'upstream/master'
Feb 28, 2019
3beb007
Merge branch 'master' into jest-environment
Feb 28, 2019
5fb6bfd
Update return type of runScript()
Feb 28, 2019
58fc28b
Update global assignment of Window
Feb 28, 2019
c7c84f7
Merge branch 'master' into jest-environment
SimenB Feb 28, 2019
06539b1
tweaks to reduce diff
SimenB Feb 28, 2019
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@

### Chore & Maintenance

- `[jest-environment-jsdom]`: Migrate to TypeScript ([#7985](https://github.com/facebook/jest/pull/8003))
- `[jest-environment-node]`: Migrate to TypeScript ([#7985](https://github.com/facebook/jest/pull/7985))
- `[*]`: Setup building, linting and testing of TypeScript ([#7808](https://github.com/facebook/jest/pull/7808), [#7855](https://github.com/facebook/jest/pull/7855), [#7951](https://github.com/facebook/jest/pull/7951))
- `[pretty-format]`: Migrate to TypeScript ([#7809](https://github.com/facebook/jest/pull/7809), [#7809](https://github.com/facebook/jest/pull/7972))
Expand Down
6 changes: 3 additions & 3 deletions packages/jest-config/tsconfig.json
Original file line number Diff line number Diff line change
Expand Up @@ -4,10 +4,10 @@
"rootDir": "src",
"outDir": "build"
},
// TODO: This is missing `jest-validate`, in addition to
// `jest-environment-jsdom` and `jest-jasmine2`, but those are just
// `require.resolve`d, so no real use for their types
// TODO: This is missing `jest-validate`, in addition to and `jest-jasmine2`,
// but those are just `require.resolve`d, so no real use for their types
"references": [
{"path": "../jest-environment-jsdom"},
{"path": "../jest-environment-node"},
{"path": "../jest-get-type"},
{"path": "../jest-regex-util"},
Expand Down
5 changes: 4 additions & 1 deletion packages/jest-environment-jsdom/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -8,13 +8,16 @@
},
"license": "MIT",
"main": "build/index.js",
"types": "build/index.d.ts",
"dependencies": {
"@jest/environment": "^24.1.0",
"@jest/fake-timers": "^24.1.0",
"@jest/types": "^24.1.0",
"jest-mock": "^24.0.0",
"jest-util": "^24.0.0",
"jsdom": "^11.5.1"
SimenB marked this conversation as resolved.
Show resolved Hide resolved
},
"devDependencies": {
"devDependencies": {
"@types/jsdom": "^11.12.0"
},
"engines": {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,24 +8,27 @@

const vm = jest.requireActual('vm');

const JSDOMEnvironment = jest.genMockFromModule('../index');
const JSDOMEnvironment = jest.genMockFromModule('../index') as jest.Mock;

JSDOMEnvironment.mockImplementation(function(config) {
// @ts-ignore
this.global = {
JSON,
console: {},
};

const globalValues = {...config.globals};
for (const customGlobalKey in globalValues) {
// @ts-ignore
this.global[customGlobalKey] = globalValues[customGlobalKey];
}
});

JSDOMEnvironment.prototype.runSourceText.mockImplementation(function(
sourceText,
filename,
sourceText: string,
filename: string,
) {
// @ts-ignore
return vm.runInNewContext(sourceText, this.global, {
displayErrors: false,
filename,
Expand Down
113 changes: 0 additions & 113 deletions packages/jest-environment-jsdom/src/index.js

This file was deleted.

146 changes: 146 additions & 0 deletions packages/jest-environment-jsdom/src/index.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,146 @@
/**
* 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.
*/

import {Script} from 'vm';
import {Global, Config} from '@jest/types';
import mock, {ModuleMocker} from 'jest-mock';
import {installCommonGlobals} from 'jest-util';
import {JestFakeTimers as FakeTimers} from '@jest/fake-timers';
import {JestEnvironment, EnvironmentContext} from '@jest/environment';
import {JSDOM, VirtualConsole} from 'jsdom';

// The `Window` interface does not have an `Error.stackTraceLimit` property, but
// `JSDOMEnvironment` assumes it is there.
interface Win extends Window {
Error: {
stackTraceLimit: number;
};
}

function isWin(globals: Win | Global.Global): globals is Win {
return (globals as Win).document !== undefined;
}

class JSDOMEnvironment implements JestEnvironment {
dom: JSDOM | null;
fakeTimers: FakeTimers<number> | null;
// @ts-ignore
global: Global.Global | Win | null;
errorEventListener: ((event: Event & {error: unknown}) => void) | null;
moduleMocker: ModuleMocker | null;

constructor(config: Config.ProjectConfig, options: EnvironmentContext = {}) {
this.dom = new JSDOM('<!DOCTYPE html>', {
pretendToBeVisual: true,
runScripts: 'dangerously',
url: config.testURL,
virtualConsole: new VirtualConsole().sendTo(options.console || console),
...config.testEnvironmentOptions,
});

// `defaultView` returns a `Window` type, which is missing the
// `Error.stackTraceLimit` property. See the `Win` interface above.
const global = (this.global = this.dom.window.document
.defaultView as Win | null);
SimenB marked this conversation as resolved.
Show resolved Hide resolved

if (!global || !this.global) {
SimenB marked this conversation as resolved.
Show resolved Hide resolved
this.fakeTimers = null;
this.errorEventListener = null;
this.moduleMocker = null;
return;
}

// 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;
// `global` is of type `Win`, but `installCommonGlobals` expects
// `NodeJS.Global`, so using `any` for now
installCommonGlobals(global as any, config.globals);

// Report uncaught errors.
this.errorEventListener = event => {
if (userErrorListenerCount === 0 && event.error) {
(process as NodeJS.EventEmitter).emit('uncaughtException', event.error);
}
};
global.addEventListener('error', this.errorEventListener);

// However, don't report them as uncaught if the user listens to 'error' event.
// In that case, we assume the might have custom error handling logic.
const originalAddListener = global.addEventListener;
const originalRemoveListener = global.removeEventListener;
let userErrorListenerCount = 0;
global.addEventListener = function(name: string) {
if (name === 'error') {
userErrorListenerCount++;
}
// TODO: remove `any` type assertion
return originalAddListener.apply(this, arguments as any);
};
global.removeEventListener = function(name: string) {
if (name === 'error') {
userErrorListenerCount--;
}
// TODO: remove `any` type assertion
return originalRemoveListener.apply(this, arguments as any);
};

// `global` is of type `Win`, but `ModuleMocker` expects `NodeJS.Global`, so
// using `any` for now
this.moduleMocker = new mock.ModuleMocker(global as any);

const timerConfig = {
idToRef: (id: number) => id,
refToId: (ref: number) => ref,
};

this.fakeTimers = new FakeTimers({
config,
// `global` is of type `Win`, but `FakeTimers` expects `NodeJS.Global`, so
// using `any` for now
global: global as any,
moduleMocker: this.moduleMocker,
timerConfig,
});
}

setup() {
return Promise.resolve();
}

teardown() {
if (this.fakeTimers) {
this.fakeTimers.dispose();
}
if (this.global) {
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});
if (isWin(this.global)) {
this.global.close();
}
}
this.errorEventListener = null;
this.global = null;
this.dom = null;
this.fakeTimers = null;
return Promise.resolve();
}

runScript(script: Script) {
if (this.dom) {
// Explicitly returning `unknown` since `runVMScript` currently returns
// `void`, which is wrong
return this.dom.runVMScript(script) as unknown;
Copy link
Member

Choose a reason for hiding this comment

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

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)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@lirbank lirbank Feb 28, 2019

Choose a reason for hiding this comment

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

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... :)

Copy link
Member

@SimenB SimenB Feb 28, 2019

Choose a reason for hiding this comment

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

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

}
return null;
}
}

export = JSDOMEnvironment;
14 changes: 14 additions & 0 deletions packages/jest-environment-jsdom/tsconfig.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
{
"extends": "../../tsconfig.json",
"compilerOptions": {
"outDir": "build",
"rootDir": "src"
},
"references": [
{"path": "../jest-environment"},
{"path": "../jest-fake-timers"},
{"path": "../jest-mock"},
{"path": "../jest-types"},
{"path": "../jest-util"}
]
}
14 changes: 11 additions & 3 deletions packages/jest-environment/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -22,15 +22,23 @@ export type EnvironmentContext = {
// TODO: type this better: https://nodejs.org/api/modules.html#modules_the_module_wrapper
type ModuleWrapper = (...args: Array<unknown>) => unknown;

export function hasModuleWrapper(
obj: unknown,
): obj is {[ScriptTransformer.EVAL_RESULT_VARIABLE]: ModuleWrapper} {
return (
typeof obj === 'object' &&
!!obj &&
obj.hasOwnProperty(ScriptTransformer.EVAL_RESULT_VARIABLE)
);
}

export declare class JestEnvironment {
constructor(config: Config.ProjectConfig);
constructor(config: Config.ProjectConfig, context: EnvironmentContext);
global: Global.Global;
fakeTimers: FakeTimers<unknown> | null;
moduleMocker: ModuleMocker | null;
runScript(
SimenB marked this conversation as resolved.
Show resolved Hide resolved
script: Script,
): {[ScriptTransformer.EVAL_RESULT_VARIABLE]: ModuleWrapper} | null;
runScript(script: Script): unknown;
setup(): Promise<void>;
teardown(): Promise<void>;
}
Expand Down
3 changes: 2 additions & 1 deletion packages/jest-runtime/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import {
JestEnvironment,
LocalModuleRequire,
Module,
hasModuleWrapper
} from '@jest/environment';
import jestMock, {MockFunctionMetadata} from 'jest-mock';
import HasteMap, {ModuleMap} from 'jest-haste-map';
Expand Down Expand Up @@ -679,7 +680,7 @@ class Runtime {

const runScript = this._environment.runScript(transformedFile.script);

if (runScript === null) {
if (!hasModuleWrapper(runScript)) {
SimenB marked this conversation as resolved.
Show resolved Hide resolved
this._logFormattedReferenceError(
'You are trying to `import` a file after the Jest environment has been torn down.',
);
Expand Down