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: isolate esm async import bug #14397

Merged
merged 15 commits into from
Aug 21, 2023
2 changes: 2 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@

### Fixes

- `[jest-runtime]` Fix dynamic esm import module bug when loaded module through `jest.isolateModulesAsync` ([14397](https://github.com/jestjs/jest/pull/14397))
eryue0220 marked this conversation as resolved.
Show resolved Hide resolved

### Chore & Maintenance

### Performance
Expand Down
9 changes: 9 additions & 0 deletions e2e/__tests__/__snapshots__/asyncESMImport.test.ts.snap
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
// Jest Snapshot v1, https://goo.gl/fbAQLP

exports[`runs test with async ESM import 1`] = `
"Test Suites: 1 passed, 1 total
Tests: 1 passed, 1 total
Snapshots: 0 total
Time: <<REPLACED>>
Ran all test suites."
`;
20 changes: 20 additions & 0 deletions e2e/__tests__/asyncESMImport.test.ts
eryue0220 marked this conversation as resolved.
Show resolved Hide resolved
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
/**
* Copyright (c) Meta Platforms, Inc. and affiliates.
*
* This source code is licensed under the MIT license found in the
* LICENSE file in the root directory of this source tree.
*/

import {extractSummary} from '../Utils';
import runJest from '../runJest';

test('runs test with async ESM import', () => {
const {exitCode, stderr} = runJest('async-esm-import', [], {
nodeOptions: '--experimental-vm-modules --no-warnings',
});

const {summary} = extractSummary(stderr);

expect(summary).toMatchSnapshot();
expect(exitCode).toBe(0);
});
22 changes: 22 additions & 0 deletions e2e/async-esm-import/__tests__/main.js
eryue0220 marked this conversation as resolved.
Show resolved Hide resolved
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
/**
* Copyright (c) Meta Platforms, Inc. and affiliates.
*
* This source code is licensed under the MIT license found in the
* LICENSE file in the root directory of this source tree.
*/
import {jest} from '@jest/globals';

test('should have a fresh module state in each isolateModulesAsync context', async () => {
await jest.isolateModulesAsync(async () => {
const {getState, incState} = await import('../main.js');
expect(getState()).toBe(0);
incState();
expect(getState()).toBe(1);
});
await jest.isolateModulesAsync(async () => {
const {getState, incState} = await import('../main.js');
expect(getState()).toBe(0);
incState();
expect(getState()).toBe(1);
});
});
15 changes: 15 additions & 0 deletions e2e/async-esm-import/main.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
/**
* Copyright (c) Meta Platforms, Inc. and affiliates.
*
* This source code is licensed under the MIT license found in the
* LICENSE file in the root directory of this source tree.
*/
let myState = 0;

export function incState() {
myState += 1;
}

export function getState() {
return myState;
}
9 changes: 9 additions & 0 deletions e2e/async-esm-import/package.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
{
"name": "async-esm-import",
"type": "module",
"jest": {
"transform": {},
"testEnvironment": "node",
"testRegex": "(/__tests__/.*|(\\.|/)(test|spec))\\.(js?|ts?|mjs?|mts?)$"
eryue0220 marked this conversation as resolved.
Show resolved Hide resolved
}
}
38 changes: 27 additions & 11 deletions packages/jest-runtime/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -179,6 +179,7 @@ export default class Runtime {
private readonly _moduleMockFactories: Map<string, () => unknown>;
private readonly _moduleMocker: ModuleMocker;
private _isolatedModuleRegistry: ModuleRegistry | null;
private _isolatedESMModuleRegistry: ModuleRegistry | null;
private _moduleRegistry: ModuleRegistry;
private readonly _esmoduleRegistry: Map<string, VMModule>;
private readonly _cjsNamedExports: Map<string, Set<string>>;
Expand Down Expand Up @@ -242,6 +243,7 @@ export default class Runtime {
this._moduleMocker = this._environment.moduleMocker;
this._isolatedModuleRegistry = null;
this._isolatedMockRegistry = null;
this._isolatedESMModuleRegistry = null;
this._moduleRegistry = new Map();
this._esmoduleRegistry = new Map();
this._cjsNamedExports = new Map();
Expand Down Expand Up @@ -416,12 +418,15 @@ export default class Runtime {
query = '',
): Promise<VMModule> {
const cacheKey = modulePath + query;
const registry = this._isolatedESMModuleRegistry
? this._isolatedESMModuleRegistry
: this._esmoduleRegistry;
Copy link
Contributor

Choose a reason for hiding this comment

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

Hm.. Isn’t it enough to use the same _isolatedMockRegistry? Did you try that?

It was my idea about missing _isolatedESMModuleRegistry, but it can I was wrong. Here we can see that _isolatedMockRegistry was simply not used while loading ES modules. Sorry about slow thinking. Could you try to _isolatedMockRegistry everywhere?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Of course, it is possible. I originally thought it was intentionally differentiated because I saw that there was already an existing _isolatedModuleRegistry, moreover, I think it seems more appropriate.


if (this._fileTransformsMutex.has(cacheKey)) {
await this._fileTransformsMutex.get(cacheKey);
}

if (!this._esmoduleRegistry.has(cacheKey)) {
if (!registry.has(cacheKey)) {
invariant(
typeof this._environment.getVmContext === 'function',
'ES Modules are only supported if your test environment has the `getVmContext` function',
Expand Down Expand Up @@ -454,15 +459,15 @@ export default class Runtime {
context,
);

this._esmoduleRegistry.set(cacheKey, wasm);
registry.set(cacheKey, wasm);

transformResolve();
return wasm;
}

if (this._resolver.isCoreModule(modulePath)) {
const core = this._importCoreModule(modulePath, context);
this._esmoduleRegistry.set(cacheKey, core);
registry.set(cacheKey, core);

transformResolve();

Expand Down Expand Up @@ -526,11 +531,11 @@ export default class Runtime {
}

invariant(
!this._esmoduleRegistry.has(cacheKey),
!registry.has(cacheKey),
`Module cache already has entry ${cacheKey}. This is a bug in Jest, please report it!`,
);

this._esmoduleRegistry.set(cacheKey, module);
registry.set(cacheKey, module);

transformResolve();
} catch (error) {
Expand All @@ -539,7 +544,7 @@ export default class Runtime {
}
}

const module = this._esmoduleRegistry.get(cacheKey);
const module = registry.get(cacheKey);

invariant(
module,
Expand All @@ -563,14 +568,18 @@ export default class Runtime {
return;
}

const registry = this._isolatedESMModuleRegistry
? this._isolatedESMModuleRegistry
: this._esmoduleRegistry;

if (specifier === '@jest/globals') {
const fromCache = this._esmoduleRegistry.get('@jest/globals');
const fromCache = registry.get('@jest/globals');

if (fromCache) {
return fromCache;
}
const globals = this.getGlobalsForEsm(referencingIdentifier, context);
this._esmoduleRegistry.set('@jest/globals', globals);
registry.set('@jest/globals', globals);

return globals;
}
Expand All @@ -586,7 +595,7 @@ export default class Runtime {
return this.importMock(referencingIdentifier, specifier, context);
}

const fromCache = this._esmoduleRegistry.get(specifier);
const fromCache = registry.get(specifier);

if (fromCache) {
return fromCache;
Expand Down Expand Up @@ -662,7 +671,7 @@ export default class Runtime {
}
}

this._esmoduleRegistry.set(specifier, module);
registry.set(specifier, module);
return module;
}

Expand Down Expand Up @@ -1164,21 +1173,28 @@ export default class Runtime {
}

async isolateModulesAsync(fn: () => Promise<void>): Promise<void> {
if (this._isolatedModuleRegistry || this._isolatedMockRegistry) {
if (
this._isolatedModuleRegistry ||
this._isolatedMockRegistry ||
this._isolatedESMModuleRegistry
) {
throw new Error(
'isolateModulesAsync cannot be nested inside another isolateModulesAsync or isolateModules.',
);
}
this._isolatedModuleRegistry = new Map();
this._isolatedMockRegistry = new Map();
this._isolatedESMModuleRegistry = new Map();
try {
await fn();
} finally {
// might be cleared within the callback
this._isolatedModuleRegistry?.clear();
this._isolatedMockRegistry?.clear();
this._isolatedESMModuleRegistry?.clear();
this._isolatedModuleRegistry = null;
this._isolatedMockRegistry = null;
this._isolatedESMModuleRegistry = null;
}
}

eryue0220 marked this conversation as resolved.
Show resolved Hide resolved
Expand Down
Loading