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

fix(env-jsdom): remove setImmediate and clearImmediate #11222

Merged
merged 7 commits into from Mar 25, 2021
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
5 changes: 3 additions & 2 deletions CHANGELOG.md
Expand Up @@ -38,7 +38,7 @@
- `[babel-plugin-jest-hoist]` Add `__dirname` and `__filename` to whitelisted globals ([#10903](https://github.com/facebook/jest/pull/10903))
- `[expect]` [**BREAKING**] Revise `expect.not.objectContaining()` to be the inverse of `expect.objectContaining()`, as documented. ([#10708](https://github.com/facebook/jest/pull/10708))
- `[expect]` [**BREAKING**] Make `toContain` more strict with the received type ([#10119](https://github.com/facebook/jest/pull/10119) & [#10929](https://github.com/facebook/jest/pull/10929))
- `[expect]` [**BREAKING**] `matcherResult` on `JestAssertionError` are now strings rather than functions ([#10989] (https://github.com/facebook/jest/pull/10989))
- `[expect]` [**BREAKING**] `matcherResult` on `JestAssertionError` are now strings rather than functions ([#10989](https://github.com/facebook/jest/pull/10989))
- `[jest-circus]` Fixed the issue of beforeAll & afterAll hooks getting executed even if it is inside a skipped `describe` block [#10451](https://github.com/facebook/jest/issues/10451)
- `[jest-circus]` Fix `testLocation` on Windows when using `test.each` ([#10871](https://github.com/facebook/jest/pull/10871))
- `[jest-cli]` Use testFailureExitCode when bailing from a failed test ([#10958](https://github.com/facebook/jest/pull/10958))
Expand All @@ -49,10 +49,11 @@
- `[jest-each]` [**BREAKING**] Ignore excess words in headings ([#8766](https://github.com/facebook/jest/pull/8766))
- `[jest-environment]` [**BREAKING**] Drop support for `runScript` for test environments ([#11155](https://github.com/facebook/jest/pull/11155))
- `[jest-environment-jsdom]` Use inner realm’s `ArrayBuffer` constructor ([#10885](https://github.com/facebook/jest/pull/10885))
- `[jest-environment-jsdom]` [**BREAKING**] Remove Node globals `setImmediate`, `clearImmediate` and `Buffer` [#11222](https://github.com/facebook/jest/pull/11222)
- `[jest-globals]` [**BREAKING**] Disallow return values other than a `Promise` from hooks and tests ([#10512](https://github.com/facebook/jest/pull/10512))
- `[jest-globals]` [**BREAKING**] Disallow mixing a done callback and returning a `Promise` from hooks and tests ([#10512](https://github.com/facebook/jest/pull/10512))
- `[jest-haste-map]` Vendor `NodeWatcher` from `sane` ([#10919](https://github.com/facebook/jest/pull/10919))
- `[jest-jasmine2]` Fixed the issue of beforeAll & afterAll hooks getting executed even if it is inside a skipped `describe` block when it has child `tests` marked as either `only` or `todo` [#10451](https://github.com/facebook/jest/issues/10451)
- `[jest-jasmine2]` Fixed the issue of `beforeAll` & `afterAll` hooks getting executed even if it is inside a skipped `describe` block when it has child `tests` marked as either `only` or `todo` [#10451](https://github.com/facebook/jest/issues/10451)
- `[jest-jasmine2]` Fixed the issues of child `tests` marked with `only` or `todo` getting executed even if it is inside a skipped parent `describe` block [#10451](https://github.com/facebook/jest/issues/10451)
- `[jest-reporter]` Handle empty files when reporting code coverage with V8 ([#10819](https://github.com/facebook/jest/pull/10819))
- `[jest-resolve]` Replace read-pkg-up with escalade package ([#10781](https://github.com/facebook/jest/pull/10781))
Expand Down
4 changes: 4 additions & 0 deletions packages/jest-environment-node/src/index.ts
Expand Up @@ -36,6 +36,9 @@ class NodeEnvironment implements JestEnvironment {
global.clearTimeout = clearTimeout;
global.setInterval = setInterval;
global.setTimeout = setTimeout;
global.Buffer = Buffer;
global.setImmediate = setImmediate;
global.clearImmediate = clearImmediate;
global.ArrayBuffer = ArrayBuffer;
// TextEncoder (global or via 'util') references a Uint8Array constructor
// different than the global one used by users in tests. This makes sure the
Expand Down Expand Up @@ -64,6 +67,7 @@ class NodeEnvironment implements JestEnvironment {
global.AbortController = AbortController;
}
installCommonGlobals(global, config.globals);

this.moduleMocker = new ModuleMocker(global);

const timerIdToRef = (id: number) => ({
Expand Down
7 changes: 5 additions & 2 deletions packages/jest-matcher-utils/src/deepCyclicCopyReplaceable.ts
Expand Up @@ -7,9 +7,8 @@

import {plugins} from 'pretty-format';

const builtInObject = [
const builtInObject: Array<unknown> = [
Array,
Buffer,
Date,
Float32Array,
Float64Array,
Expand All @@ -25,6 +24,10 @@ const builtInObject = [
Uint8ClampedArray,
];

if (typeof Buffer !== 'undefined') {
builtInObject.push(Buffer);
}

const isBuiltInObject = (object: any) =>
builtInObject.includes(object.constructor);

Expand Down
23 changes: 17 additions & 6 deletions packages/jest-runner/src/runTest.ts
Expand Up @@ -206,13 +206,24 @@ async function runTestInternal(
},
};

const isBrowserEnv = typeof environment.global?.document !== 'undefined';

// For tests
runtime
.requireInternalModule<typeof import('source-map-support')>(
require.resolve('source-map-support'),
'source-map-support',
)
.install(sourcemapOptions);
if (isBrowserEnv) {
runtime.requireInternalModule(
Copy link
Member Author

@SimenB SimenB Mar 20, 2021

Choose a reason for hiding this comment

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

for some reason this is horribly slow:

image

If I comment out the install call (but keeping the require):

image

Note that the install itself is fast, it's whatever happens when something throws later that that takes 10+ seconds...

Copy link
Member Author

Choose a reason for hiding this comment

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

@LinusU any ideas about what's happening here? I'm having some issues reproducing this using plain JSDOM, so I'm not really sure where to begin debugging this...

require.resolve('source-map-support/browser-source-map-support'),
);
const sourceMapSupport = environment.global
.sourceMapSupport as typeof sourcemapSupport;

sourceMapSupport.install({...sourcemapOptions, environment: 'browser'});
} else {
runtime
.requireInternalModule<typeof sourcemapSupport>(
require.resolve('source-map-support'),
)
.install(sourcemapOptions);
}

// For runtime errors
sourcemapSupport.install(sourcemapOptions);
Expand Down
13 changes: 8 additions & 5 deletions packages/jest-runtime/src/index.ts
Expand Up @@ -652,12 +652,10 @@ export default class Runtime {

if (options?.isInternalModule) {
moduleRegistry = this._internalModuleRegistry;
} else if (this._isolatedModuleRegistry) {
moduleRegistry = this._isolatedModuleRegistry;
} else {
if (this._isolatedModuleRegistry) {
moduleRegistry = this._isolatedModuleRegistry;
} else {
moduleRegistry = this._moduleRegistry;
}
moduleRegistry = this._moduleRegistry;
}

const module = moduleRegistry.get(modulePath);
Expand Down Expand Up @@ -1823,6 +1821,11 @@ export default class Runtime {
throw moduleNotFoundError;
}

e.message = `Jest: Got error evaluating ${path.relative(
Copy link
Member Author

Choose a reason for hiding this comment

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

It was impossible to see where errors originated without this

Before: image

After: image

Copy link
Member Author

Choose a reason for hiding this comment

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

let's not have that now though to reduce the surface of this PR

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh wow I ran into that reference error yesterday for some reasons and it was gone out of the blue

this._config.rootDir,
module.filename,
)}.\n${e.message}`;

throw e;
}

Expand Down
5 changes: 0 additions & 5 deletions packages/jest-util/src/installCommonGlobals.ts
Expand Up @@ -62,10 +62,5 @@ export default function (
};
});

// Forward some others (this breaks the sandbox but for now it's OK).
globalObject.Buffer = global.Buffer;
globalObject.setImmediate = global.setImmediate;
globalObject.clearImmediate = global.clearImmediate;

return Object.assign(globalObject, deepCyclicCopy(globals));
}