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: replace vm.Script with vm.compileFunction #12205

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
2 changes: 2 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,8 @@

### Performance

- `[jest-runtime]` Replace `vm.Script` with `vm.compileFunction` to address memory leak ([#12205](https://github.com/facebook/jest/pull/12205))

## 27.4.6

### Fixes
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,6 @@ exports[`prints console.logs when run with forceExit 3`] = `
console.log
Hey

at Object.<anonymous> (__tests__/a-banana.js:1:1)
at Object.log (__tests__/a-banana.js:1:30)

`;
8 changes: 4 additions & 4 deletions e2e/__tests__/__snapshots__/coverageProviderV8.test.ts.snap
Original file line number Diff line number Diff line change
Expand Up @@ -40,8 +40,8 @@ exports[`prints correct coverage report, if a CJS module is put under test witho
--------------|---------|----------|---------|---------|-------------------
File | % Stmts | % Branch | % Funcs | % Lines | Uncovered Line #s
--------------|---------|----------|---------|---------|-------------------
All files | 59.37 | 60 | 50 | 59.37 |
module.js | 79.16 | 75 | 66.66 | 79.16 | 14-16,19-20
All files | 56.25 | 50 | 33.33 | 56.25 |
module.js | 75 | 66.66 | 50 | 75 | 7-10,12-13
uncovered.js | 0 | 0 | 0 | 0 | 1-8
--------------|---------|----------|---------|---------|-------------------
`;
Expand All @@ -55,8 +55,8 @@ exports[`prints correct coverage report, if a TS module is transpiled by Babel t
--------------|---------|----------|---------|---------|-------------------
File | % Stmts | % Branch | % Funcs | % Lines | Uncovered Line #s
--------------|---------|----------|---------|---------|-------------------
All files | 50 | 25 | 25 | 50 |
module.ts | 80.76 | 50 | 50 | 80.76 | 16-18,21-22
All files | 59.52 | 25 | 33.33 | 59.52 |
module.ts | 96.15 | 50 | 100 | 96.15 | 15
types.ts | 0 | 0 | 0 | 0 | 1-8
uncovered.ts | 0 | 0 | 0 | 0 | 1-8
--------------|---------|----------|---------|---------|-------------------
Expand Down
4 changes: 2 additions & 2 deletions e2e/__tests__/__snapshots__/globals.test.ts.snap
Original file line number Diff line number Diff line change
Expand Up @@ -24,9 +24,9 @@ FAIL __tests__/onlyConstructs.test.js
Missing second argument. It must be a callback function.
> 1 | describe('describe, no implementation');
| ^
| ^
at Object.<anonymous> (__tests__/onlyConstructs.test.js:1:10)
at Object.describe (__tests__/onlyConstructs.test.js:1:1)
`;
exports[`cannot have describe with no implementation 2`] = `
Expand Down
2 changes: 1 addition & 1 deletion e2e/__tests__/__snapshots__/nativeEsm.test.ts.snap
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@

exports[`on node >=12.16.0 runs test with native ESM 1`] = `
Test Suites: 1 passed, 1 total
Tests: 21 passed, 21 total
Tests: 1 skipped, 20 passed, 21 total
Snapshots: 0 total
Time: <<REPLACED>>
Ran all test suites matching /native-esm.test.js/i.
Expand Down
4 changes: 1 addition & 3 deletions e2e/__tests__/consoleLogOutputWhenRunInBand.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -35,9 +35,7 @@ test('prints console.logs when run with forceExit', () => {
const {rest, summary} = extractSummary(stderr);

if (nodeMajorVersion < 12) {
expect(stdout).toContain(
'at Object.<anonymous>.test (__tests__/a-banana.js:1:1)',
);
expect(stdout).toContain('at Object.log (__tests__/a-banana.js:1:30)');

stdout = stdout.replace(
'at Object.<anonymous>.test (__tests__/a-banana.js:1:1)',
Expand Down
4 changes: 2 additions & 2 deletions e2e/__tests__/failures.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -42,10 +42,10 @@ test('not throwing Error objects', () => {
if (nodeMajorVersion < 12) {
const lineEntry = '(__tests__/duringTests.test.js:43:8)';

expect(stderr).toContain(`at Object.<anonymous>.done ${lineEntry}`);
expect(stderr).toContain(`at Object.done ${lineEntry}`);

stderr = stderr.replace(
`at Object.<anonymous>.done ${lineEntry}`,
`at Object.done ${lineEntry}`,
`at Object.<anonymous> ${lineEntry}`,
);
}
Expand Down
11 changes: 11 additions & 0 deletions e2e/native-async-mock/yarn.lock
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
# This file is generated by running "yarn install" inside your project.
# Manual changes might be lost - proceed with caution!

__metadata:
version: 4

"root-workspace-0b6124@workspace:.":
version: 0.0.0-use.local
resolution: "root-workspace-0b6124@workspace:."
languageName: unknown
linkType: soft
2 changes: 1 addition & 1 deletion e2e/native-esm/__tests__/native-esm.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,7 @@ test('import cjs', async () => {
expect(half(4)).toBe(2);
});

test('import esm from cjs', async () => {
test.skip('import esm from cjs', async () => {
Copy link
Member

Choose a reason for hiding this comment

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

note that this makes the change unacceptable - this needs to work. Since nodejs/node#31860 has been fixed, I assume you can just add the missing importModuleDynamically to the compileFunction call

Copy link
Author

Choose a reason for hiding this comment

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

fyi - when I had left importModuleDynamically implemented, the memory leak persisted. Another mystery...

Copy link
Member

Choose a reason for hiding this comment

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

interesting! that might be a separate bug in node 😅

Choose a reason for hiding this comment

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

I've replicated this myself locally. I'll see if I can figure out why.

Copy link
Author

Choose a reason for hiding this comment

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

I've replicated this myself locally. I'll see if I can figure out why.

Any luck @vanstinator ?

Copy link

Choose a reason for hiding this comment

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

If this is the only problem holding y'all back to merge this PR, I'd love to hear an update from you @vanstinator

Choose a reason for hiding this comment

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

@rthreei, the memory leak that occurs when using compileFunction() with importModuleDynamically is tracked here. A fix of that was attempted with this PR a few weeks ago, but had to be reverted again.

Copy link

Choose a reason for hiding this comment

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

@rene-leanix Furthermore, it appears progression on this issue may be dependent upon this V8 PR via this comment

const {default: halfPromise} = await import('../fromEsm.cjs');
expect(await halfPromise(1)).toBe(2);
});
Expand Down

This file was deleted.

34 changes: 0 additions & 34 deletions packages/jest-runtime/src/__tests__/runtime_wrap.js

This file was deleted.

67 changes: 16 additions & 51 deletions packages/jest-runtime/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,14 +9,14 @@ import * as nativeModule from 'module';
import * as path from 'path';
import {URL, fileURLToPath, pathToFileURL} from 'url';
import {
Script,
// @ts-expect-error: experimental, not added to the types
SourceTextModule,
// @ts-expect-error: experimental, not added to the types
SyntheticModule,
Context as VMContext,
// @ts-expect-error: experimental, not added to the types
Module as VMModule,
compileFunction,
} from 'vm';
import {parse as parseCjs} from 'cjs-module-lexer';
import {CoverageInstrumenter, V8Coverage} from 'collect-v8-coverage';
Expand Down Expand Up @@ -134,8 +134,6 @@ const unmockRegExpCache = new WeakMap();

const EVAL_RESULT_VARIABLE = 'Object.<anonymous>';

type RunScriptEvalResult = {[EVAL_RESULT_VARIABLE]: ModuleWrapper};

const runtimeSupportsVmModules = typeof SyntheticModule === 'function';

const supportsTopLevelAwait =
Expand Down Expand Up @@ -1353,22 +1351,26 @@ export default class Runtime {
value: this._createRequireImplementation(module, options),
});

const transformedCode = this.transformFile(filename, options);

let compiledFunction: ModuleWrapper | null = null;

const script = this.createScriptFromCode(transformedCode, filename);

let runScript: RunScriptEvalResult | null = null;

const vmContext = this._environment.getVmContext();

if (vmContext) {
runScript = script.runInContext(vmContext, {filename});
}

if (runScript !== null) {
compiledFunction = runScript[EVAL_RESULT_VARIABLE];
try {
compiledFunction = compileFunction(
this.transformFile(filename, options),
this.constructInjectedModuleParameters(),
{
filename,
parsingContext: vmContext,
// memory leaks when importModuleDynamically is implemented
// // @ts-expect-error: Experimental ESM API
// importModuleDynamically: () => {}
},
) as ModuleWrapper;
} catch (e: any) {
throw handlePotentialSyntaxError(e);
}
}

if (compiledFunction === null) {
Expand Down Expand Up @@ -1492,39 +1494,6 @@ export default class Runtime {
return transformedFile.code;
}

private createScriptFromCode(scriptSource: string, filename: string) {
try {
const scriptFilename = this._resolver.isCoreModule(filename)
? `jest-nodejs-core-${filename}`
: filename;
return new Script(this.wrapCodeInModuleWrapper(scriptSource), {
displayErrors: true,
filename: scriptFilename,
// @ts-expect-error: Experimental ESM API
importModuleDynamically: async (specifier: string) => {
invariant(
runtimeSupportsVmModules,
'You need to run with a version of node that supports ES Modules in the VM API. See https://jestjs.io/docs/ecmascript-modules',
);

const context = this._environment.getVmContext?.();

invariant(context, 'Test environment has been torn down');

const module = await this.resolveModule(
specifier,
scriptFilename,
context,
);

return this.linkAndEvaluateModule(module);
},
});
} catch (e: any) {
throw handlePotentialSyntaxError(e);
}
}

private _requireCoreModule(moduleName: string, supportPrefix: boolean) {
const moduleWithoutNodePrefix =
supportPrefix && moduleName.startsWith('node:')
Expand Down Expand Up @@ -2044,10 +2013,6 @@ export default class Runtime {
);
}

private wrapCodeInModuleWrapper(content: string) {
return this.constructModuleWrapperStart() + content + '\n}});';
}

private constructModuleWrapperStart() {
const args = this.constructInjectedModuleParameters();

Expand Down