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

feat: override Module#_compile #10072

Closed
wants to merge 10 commits into from
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@

- `[jest-circus, jest-config, jest-runtime]` Add new `injectGlobals` config and CLI option to disable injecting global variables into the runtime ([#10484](https://github.com/facebook/jest/pull/10484))
- `[jest-each]` Fixes `.each` type to always be callable ([#10447](https://github.com/facebook/jest/pull/10447))
- `[jest-runtime]` Override `Module#_compile` to hook into Jest's module loader ([#10072](https://github.com/facebook/jest/pull/10072))

### Fixes

Expand Down
11 changes: 7 additions & 4 deletions packages/jest-runtime/src/__mocks__/createRuntime.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,8 +7,9 @@

import path from 'path';

module.exports = async function createRuntime(filename, config) {
const NodeEnvironment = require('jest-environment-node');
module.exports = async function createRuntime(filename, config, options) {
const Environment =
options?.EnvironmentImpl || require('jest-environment-node');
const Runtime = require('../');

const {normalize} = require('jest-config');
Expand All @@ -35,8 +36,10 @@ module.exports = async function createRuntime(filename, config) {
{},
).options;

const environment = new NodeEnvironment(config);
environment.global.console = console;
const environment = new Environment(config);
if (environment.global) {
environment.global.console = console;
}

const hasteMap = await Runtime.createHasteMap(config, {
maxWorkers: 1,
Expand Down
110 changes: 110 additions & 0 deletions packages/jest-runtime/src/__tests__/runtime_compile_module.test.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,110 @@
/**
* 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 {Module} from 'module';
import path from 'path';
import * as fs from 'graceful-fs';

let createRuntime;

describe('Runtime requireModule', () => {
beforeEach(() => {
createRuntime = require('createRuntime');
});

it('overrides `Module#compile`', () =>
createRuntime(__filename).then(runtime => {
const exports = runtime.requireModule(runtime.__mockRootPath, 'module');
expect(exports.Module).not.toBe(Module);

const mockFilename = name =>
path.join(path.dirname(runtime.__mockRootPath), name);

{
const pathRegularModule = mockFilename('RegularModule.js');
const source = fs.readFileSync(pathRegularModule, 'utf-8');

const module = new exports.Module();
module._compile(source, pathRegularModule);
expect(module).toMatchObject({
children: expect.anything(),
exports: expect.anything(),
filename: null,
loaded: false,
parent: null,
paths: expect.anything(),
});
// This is undefined in Node 10 and '' in Node 14 by default.
expect(module.id).toBeFalsy();
expect(Object.keys(module.exports)).toEqual([
'filename',
'getModuleStateValue',
'isRealModule',
'jest',
'lazyRequire',
'object',
'parent',
'paths',
'setModuleStateValue',
'module',
'loaded',
'isLoaded',
]);

expect(module.exports.getModuleStateValue()).toBe('default');

module.exports.lazyRequire();

// The dynamically compiled module should not be added to the registry,
// so no side effects should occur.
expect(module.exports.getModuleStateValue()).toBe('default');
}

{
const module = new exports.Module();
module._compile('exports.value = 12;', mockFilename('dynamic.js'));
expect(module.exports).toEqual({value: 12});
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this actually verify that the module was run in the runtime, with access to the environment etc.?

Copy link
Author

Choose a reason for hiding this comment

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

It really just exists to verify that it doesn't break basic functionality. The other test does verify that the module was run in the runtime because it verifies that jest is available. I do intend to add another test that checks that jsdom is present, just as soon as I figure out how to do that in this test setup.

Copy link
Author

Choose a reason for hiding this comment

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

}

expect(() =>
module._compile('{"value":12}', mockFilename('dynamic.json')),
).toThrow(expect.objectContaining({name: 'SyntaxError'}));

{
const module = new exports.Module();
module._compile(
'exports.windowType = typeof window;',
mockFilename('example.js'),
);
expect(module.exports).toEqual({windowType: 'undefined'});
}
}));

it('provides the appropriate environment', () =>
createRuntime(
__filename,
{},
{
EnvironmentImpl: require('../../../jest-environment-jsdom/src'),
},
).then(runtime => {
const exports = runtime.requireModule(runtime.__mockRootPath, 'module');

const mockFilename = name =>
path.join(path.dirname(runtime.__mockRootPath), name);

{
const module = new exports.Module();
module._compile(
'exports.hasWindow = !!window;',
mockFilename('example.js'),
);
expect(module.exports).toEqual({hasWindow: true});
}
}));
});
73 changes: 58 additions & 15 deletions packages/jest-runtime/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,7 @@ type InternalModuleOptions = {
isInternalModule: boolean;
supportsDynamicImport: boolean;
supportsStaticESM: boolean;
filenameOverride?: string;
};

const defaultTransformOptions: InternalModuleOptions = {
Expand Down Expand Up @@ -368,7 +369,7 @@ class Runtime {
return core;
}

const transformedCode = this.transformFile(modulePath, {
const transformedCode = this.transformFile(modulePath, undefined, {
isInternalModule: false,
supportsDynamicImport: true,
supportsStaticESM: true,
Expand Down Expand Up @@ -689,7 +690,13 @@ class Runtime {
} else {
// Only include the fromPath if a moduleName is given. Else treat as root.
const fromPath = moduleName ? from : null;
this._execModule(localModule, options, moduleRegistry, fromPath);
this._execModule(
localModule,
undefined,
options,
moduleRegistry,
fromPath,
);
}
localModule.loaded = true;
}
Expand Down Expand Up @@ -968,18 +975,19 @@ class Runtime {
return this._resolver.getModulePaths(path.resolve(from, '..'));
}

private _execModule(
private _execModule<T = unknown>(
localModule: InitialModule,
runtimeModuleSource: string | undefined,
options: InternalModuleOptions | undefined,
moduleRegistry: ModuleRegistry,
from: Config.Path | null,
) {
): T | undefined {
// If the environment was disposed, prevent this module from being executed.
if (!this._environment.global) {
return;
return undefined;
}

const filename = localModule.filename;
const filename = options?.filenameOverride ?? localModule.filename;
const lastExecutingModulePath = this._currentlyExecutingModulePath;
this._currentlyExecutingModulePath = filename;
const origCurrExecutingManualMock = this._isCurrentlyExecutingManualMock;
Expand All @@ -1001,7 +1009,11 @@ class Runtime {
value: this._createRequireImplementation(localModule, options),
});

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

let compiledFunction: ModuleWrapper | null = null;

Expand Down Expand Up @@ -1042,7 +1054,7 @@ class Runtime {
'You are trying to `import` a file after the Jest environment has been torn down.',
);
process.exitCode = 1;
return;
return undefined;
}

const jestObject = this._createJestObjectFor(filename);
Expand All @@ -1063,7 +1075,7 @@ class Runtime {
];

try {
compiledFunction.call(
return compiledFunction.call(
localModule.exports,
localModule as NodeModule, // module object
localModule.exports, // module exports
Expand All @@ -1079,13 +1091,16 @@ class Runtime {

this._isCurrentlyExecutingManualMock = origCurrExecutingManualMock;
this._currentlyExecutingModulePath = lastExecutingModulePath;

return undefined;
}

private transformFile(
filename: string,
runtimeModuleSource: string | undefined,
options?: InternalModuleOptions,
): string {
const source = this.readFile(filename);
const source = runtimeModuleSource ?? this.readFile(filename);

if (options?.isInternalModule) {
return source;
Expand Down Expand Up @@ -1179,8 +1194,28 @@ class Runtime {
});
};

const runtime = this;

// should we implement the class ourselves?
class Module extends nativeModule.Module {}
class Module extends nativeModule.Module {
_compile(content: string, filename: string) {
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
_compile(content: string, filename: string) {
_compile(content: unknown, filename: unknown) {

I believe your typeofs narrow the type correctly.

Copy link
Author

Choose a reason for hiding this comment

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

I'm not really familiar with Typescript - I'm guessing that it'll infer the type based on type guards? This feels like a convention I'm not aware of - wouldn't we rather have the types be explicit for readability?

Copy link
Member

Choose a reason for hiding this comment

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

it's more honest since we get it from user input. And the typeofs will narrow it to string

if (typeof content !== 'string') {
throw new TypeError('Module#_compile must receive string content');
}

if (typeof filename !== 'string') {
throw new TypeError('Module#_compile must receive string filename');
}

return runtime._execModule(
this,
Copy link
Member

Choose a reason for hiding this comment

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

jest-runtime's internal InitialModule is not instanceof Module, I believe? It probably should, but if not (and it's hard to make it so) I think constructing a new object here rather than passing this makes sense

Copy link
Author

@skeggse skeggse Jul 29, 2020

Choose a reason for hiding this comment

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

You're suggesting we make a fake Module instead, and provide that to the application code instead of leveraging the existing jest-runtime-specific instance? That seems reasonable to me, and might (?) fix the issues around filename and filenameOverride that @jeysal identified.

Copy link
Member

Choose a reason for hiding this comment

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

The current jest-runtime one should be Module ideally, yeah, then reused here. I think? 😅

content,
{...defaultTransformOptions, filenameOverride: filename},
runtime._moduleRegistry,
null,
);
}
}

Object.entries(nativeModule.Module).forEach(([key, value]) => {
// @ts-expect-error
Expand Down Expand Up @@ -1331,9 +1366,11 @@ class Runtime {
from: InitialModule,
options?: InternalModuleOptions,
): NodeRequire {
const filenameOverride = options?.filenameOverride;

const resolve = (moduleName: string, resolveOptions?: ResolveOptions) => {
const resolved = this._requireResolve(
from.filename,
filenameOverride ?? from.filename,
moduleName,
resolveOptions,
);
Expand All @@ -1346,12 +1383,18 @@ class Runtime {
return resolved;
};
resolve.paths = (moduleName: string) =>
this._requireResolvePaths(from.filename, moduleName);
this._requireResolvePaths(filenameOverride ?? from.filename, moduleName);

const moduleRequire = (options?.isInternalModule
? (moduleName: string) =>
this.requireInternalModule(from.filename, moduleName)
: this.requireModuleOrMock.bind(this, from.filename)) as NodeRequire;
this.requireInternalModule(
filenameOverride ?? from.filename,
moduleName,
)
: this.requireModuleOrMock.bind(
this,
filenameOverride ?? from.filename,
)) as NodeRequire;
moduleRequire.extensions = Object.create(null);
moduleRequire.resolve = resolve;
moduleRequire.cache = (() => {
Expand Down
3 changes: 2 additions & 1 deletion packages/jest-transform/src/ScriptTransformer.ts
Original file line number Diff line number Diff line change
Expand Up @@ -445,7 +445,8 @@ export default class ScriptTransformer {
let scriptCacheKey = undefined;
let instrument = false;

if (!options.isCoreModule) {
// Skip cache for core and dynamically loaded modules.
if (!options.isCoreModule && typeof options.filenameOverride !== 'string') {
Comment on lines +448 to +449
Copy link
Author

Choose a reason for hiding this comment

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

The alternative here is to explicitly handle the case that the file doesn't exist when calling statSync in getScriptCacheKey, but it seemed best to just skip the cache for dynamically loaded modules.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah def not the stat solution, there could still be a regular file for the path of the module that was compiled. Can we not use the regular filename arg and just flag it as dynamically compiled in the options though?

Copy link
Contributor

Choose a reason for hiding this comment

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

This also goes for all the filenameOverride ?? from.filename places - if we set the module's filename when it is compiled, can we not use that and just flag it as dynamically compiled?

Copy link
Contributor

Choose a reason for hiding this comment

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

Although I guess that would deviate from Node, where the filename is always null?

Copy link
Author

Choose a reason for hiding this comment

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

Yeah, that was my concern.

instrument =
options.coverageProvider === 'babel' &&
shouldInstrument(filename, options, this._config);
Expand Down
1 change: 1 addition & 0 deletions packages/jest-transform/src/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ export type Options = ShouldInstrumentOptions &
isInternalModule: boolean;
supportsDynamicImport: boolean;
supportsStaticESM: boolean;
filenameOverride?: string;
}>;

// This is fixed in source-map@0.7.x, but we can't upgrade yet since it's async
Expand Down