From 9c5f709f13274a6099b296e353a54cc324d2e494 Mon Sep 17 00:00:00 2001 From: Takeru Date: Mon, 30 Nov 2020 19:30:40 +0800 Subject: [PATCH 1/4] fix: unable to import mutually dependent ES module --- CHANGELOG.md | 1 + packages/jest-runtime/src/index.ts | 64 ++++++++++++++++++++++-------- 2 files changed, 48 insertions(+), 17 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 55020af9e4f8..cde16dcab443 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -24,6 +24,7 @@ - `[jest-resolve]` Disable `jest-pnp-resolver` for Yarn 2 ([#10847](https://github.com/facebook/jest/pull/10847)) - `[jest-runtime]` [**BREAKING**] Do not inject `global` variable into module wrapper ([#10644](https://github.com/facebook/jest/pull/10644)) - `[jest-runtime]` [**BREAKING**] remove long-deprecated `jest.addMatchers`, `jest.resetModuleRegistry`, and `jest.runTimersToTime` ([#9853](https://github.com/facebook/jest/pull/9853)) +- `[jest-runtime]` Fix stack overflow and promise deadlock when importing mutual dependant ES module - `[jest-transform]` Show enhanced `SyntaxError` message for all `SyntaxError`s ([#10749](https://github.com/facebook/jest/pull/10749)) - `[jest-transform]` [**BREAKING**] Refactor API to pass an options bag around rather than multiple boolean options ([#10753](https://github.com/facebook/jest/pull/10753)) - `[jest-transform]` [**BREAKING**] Refactor API of transformers to pass an options bag rather than separate `config` and other options diff --git a/packages/jest-runtime/src/index.ts b/packages/jest-runtime/src/index.ts index c843b74dd338..818cc090387a 100644 --- a/packages/jest-runtime/src/index.ts +++ b/packages/jest-runtime/src/index.ts @@ -57,6 +57,11 @@ import type {Context} from './types'; export type {Context} from './types'; +interface EsmModuleCache { + beforeEvaluation: Promise; + fullyEvaluated: Promise; +} + const esmIsAvailable = typeof SourceTextModule === 'function'; interface JestGlobals extends Global.TestFrameworkGlobals { @@ -172,7 +177,7 @@ export default class Runtime { private _moduleMocker: typeof jestMock; private _isolatedModuleRegistry: ModuleRegistry | null; private _moduleRegistry: ModuleRegistry; - private _esmoduleRegistry: Map>; + private _esmoduleRegistry: Map; private _testPath: Config.Path | undefined; private _resolver: Resolver; private _shouldAutoMock: boolean; @@ -363,6 +368,7 @@ export default class Runtime { private async loadEsmModule( modulePath: Config.Path, query = '', + isStaticImport = false ): Promise { const cacheKey = modulePath + query; @@ -378,7 +384,10 @@ export default class Runtime { if (this._resolver.isCoreModule(modulePath)) { const core = this._importCoreModule(modulePath, context); - this._esmoduleRegistry.set(cacheKey, core); + this._esmoduleRegistry.set(cacheKey, { + beforeEvaluation: core, + fullyEvaluated: core + }); return core; } @@ -401,31 +410,48 @@ export default class Runtime { specifier, referencingModule.identifier, referencingModule.context, + false ), initializeImportMeta(meta: ImportMeta) { meta.url = pathToFileURL(modulePath).href; }, }); + let resolve: (value: VMModule) => void; + let reject: (value: any) => void; + let promise = new Promise((_resolve, _reject) => { resolve = _resolve; reject = _reject }); + + // add to registry before link so that circular import won't end up stack overflow this._esmoduleRegistry.set( cacheKey, // we wanna put the linking promise in the cache so modules loaded in // parallel can all await it. We then await it synchronously below, so // we shouldn't get any unhandled rejections - module - .link((specifier: string, referencingModule: VMModule) => - this.linkModules( - specifier, - referencingModule.identifier, - referencingModule.context, - ), - ) - .then(() => module.evaluate()) - .then(() => module), + { + beforeEvaluation: Promise.resolve(module), + fullyEvaluated: promise + } ); + + module + .link((specifier: string, referencingModule: VMModule) => + this.linkModules( + specifier, + referencingModule.identifier, + referencingModule.context, + true + ), + ) + .then(() => module.evaluate()) + .then(() => resolve(module), (e: any) => reject(e)); } - const module = this._esmoduleRegistry.get(cacheKey); + const entry = this._esmoduleRegistry.get(cacheKey); + + // return the already resolved, pre-evaluation promise + // is loaded through static import to prevent promise deadlock + // because module is evaluated after all static import is resolved + const module = isStaticImport ? entry?.beforeEvaluation : entry?.fullyEvaluated; invariant(module); @@ -436,15 +462,19 @@ export default class Runtime { specifier: string, referencingIdentifier: string, context: VMContext, + isStaticImport: boolean ) { if (specifier === '@jest/globals') { const fromCache = this._esmoduleRegistry.get('@jest/globals'); if (fromCache) { - return fromCache; + return isStaticImport ? fromCache.beforeEvaluation : fromCache.fullyEvaluated; } const globals = this.getGlobalsForEsm(referencingIdentifier, context); - this._esmoduleRegistry.set('@jest/globals', globals); + this._esmoduleRegistry.set('@jest/globals', { + beforeEvaluation: globals, + fullyEvaluated: globals + }); return globals; } @@ -461,7 +491,7 @@ export default class Runtime { this._resolver.isCoreModule(resolved) || this.unstable_shouldLoadAsEsm(resolved) ) { - return this.loadEsmModule(resolved, query); + return this.loadEsmModule(resolved, query, isStaticImport); } return this.loadCjsAsEsm(referencingIdentifier, resolved, context); @@ -1169,7 +1199,7 @@ export default class Runtime { invariant(context); - return this.linkModules(specifier, scriptFilename, context); + return this.linkModules(specifier, scriptFilename, context, false); }, }); } catch (e) { From 53b721f719491104c311cb57fbb935501f61f9c1 Mon Sep 17 00:00:00 2001 From: Takeru Date: Mon, 30 Nov 2020 23:52:32 +0800 Subject: [PATCH 2/4] chore: add test verifying circular dependency --- e2e/native-esm/__tests__/native-esm.test.js | 7 +++++++ e2e/native-esm/circularDependentA.mjs | 8 ++++++++ e2e/native-esm/circularDependentB.mjs | 8 ++++++++ 3 files changed, 23 insertions(+) create mode 100644 e2e/native-esm/circularDependentA.mjs create mode 100644 e2e/native-esm/circularDependentB.mjs diff --git a/e2e/native-esm/__tests__/native-esm.test.js b/e2e/native-esm/__tests__/native-esm.test.js index 019f0ff2f340..f46a9ab57cd4 100644 --- a/e2e/native-esm/__tests__/native-esm.test.js +++ b/e2e/native-esm/__tests__/native-esm.test.js @@ -156,3 +156,10 @@ test('supports file urls as imports', async () => { test('namespace export', () => { expect(bag.double).toBe(double); }); + +test('handle circular dependency', async () => { + const moduleA = (await import('../circularDependentA.mjs')).default; + expect(moduleA.id).toBe('circularDependentA'); + expect(moduleA.moduleB.id).toBe('circularDependentB'); + expect(moduleA.moduleB.moduleA).toBe(moduleA); +}); diff --git a/e2e/native-esm/circularDependentA.mjs b/e2e/native-esm/circularDependentA.mjs new file mode 100644 index 000000000000..8cf9fe7b9f9d --- /dev/null +++ b/e2e/native-esm/circularDependentA.mjs @@ -0,0 +1,8 @@ +import circularDependentB from './circularDependentB.mjs'; + +export default { + id: 'circularDependentA', + get moduleB() { + return circularDependentB; + } +}; diff --git a/e2e/native-esm/circularDependentB.mjs b/e2e/native-esm/circularDependentB.mjs new file mode 100644 index 000000000000..9c6d7ef89407 --- /dev/null +++ b/e2e/native-esm/circularDependentB.mjs @@ -0,0 +1,8 @@ +import circularDependentA from './circularDependentA.mjs'; + +export default { + id: 'circularDependentB', + get moduleA() { + return circularDependentA; + } +}; From 410de0970b6e877c7c56e02c085eaae2662375da Mon Sep 17 00:00:00 2001 From: Takeru Date: Tue, 1 Dec 2020 22:43:20 +0800 Subject: [PATCH 3/4] chore: fix linting errors, update snapshot --- CHANGELOG.md | 2 +- .../__snapshots__/nativeEsm.test.ts.snap | 2 +- e2e/native-esm/circularDependentA.mjs | 8 ++--- e2e/native-esm/circularDependentB.mjs | 8 ++--- packages/jest-runtime/src/index.ts | 34 ++++++++++++------- 5 files changed, 32 insertions(+), 22 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index cde16dcab443..ea09f686a44e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -24,7 +24,7 @@ - `[jest-resolve]` Disable `jest-pnp-resolver` for Yarn 2 ([#10847](https://github.com/facebook/jest/pull/10847)) - `[jest-runtime]` [**BREAKING**] Do not inject `global` variable into module wrapper ([#10644](https://github.com/facebook/jest/pull/10644)) - `[jest-runtime]` [**BREAKING**] remove long-deprecated `jest.addMatchers`, `jest.resetModuleRegistry`, and `jest.runTimersToTime` ([#9853](https://github.com/facebook/jest/pull/9853)) -- `[jest-runtime]` Fix stack overflow and promise deadlock when importing mutual dependant ES module +- `[jest-runtime]` Fix stack overflow and promise deadlock when importing mutual dependant ES module ([#10892](https://github.com/facebook/jest/pull/10892)) - `[jest-transform]` Show enhanced `SyntaxError` message for all `SyntaxError`s ([#10749](https://github.com/facebook/jest/pull/10749)) - `[jest-transform]` [**BREAKING**] Refactor API to pass an options bag around rather than multiple boolean options ([#10753](https://github.com/facebook/jest/pull/10753)) - `[jest-transform]` [**BREAKING**] Refactor API of transformers to pass an options bag rather than separate `config` and other options diff --git a/e2e/__tests__/__snapshots__/nativeEsm.test.ts.snap b/e2e/__tests__/__snapshots__/nativeEsm.test.ts.snap index f336c89b96aa..b1e95ff54928 100644 --- a/e2e/__tests__/__snapshots__/nativeEsm.test.ts.snap +++ b/e2e/__tests__/__snapshots__/nativeEsm.test.ts.snap @@ -10,7 +10,7 @@ Ran all test suites matching /native-esm.tla.test.js/i. exports[`on node ^12.16.0 || >=13.7.0 runs test with native ESM 1`] = ` Test Suites: 1 passed, 1 total -Tests: 17 passed, 17 total +Tests: 18 passed, 18 total Snapshots: 0 total Time: <> Ran all test suites matching /native-esm.test.js/i. diff --git a/e2e/native-esm/circularDependentA.mjs b/e2e/native-esm/circularDependentA.mjs index 8cf9fe7b9f9d..9ec3a9246f54 100644 --- a/e2e/native-esm/circularDependentA.mjs +++ b/e2e/native-esm/circularDependentA.mjs @@ -1,8 +1,8 @@ import circularDependentB from './circularDependentB.mjs'; export default { - id: 'circularDependentA', - get moduleB() { - return circularDependentB; - } + id: 'circularDependentA', + get moduleB() { + return circularDependentB; + }, }; diff --git a/e2e/native-esm/circularDependentB.mjs b/e2e/native-esm/circularDependentB.mjs index 9c6d7ef89407..7d7003b660cd 100644 --- a/e2e/native-esm/circularDependentB.mjs +++ b/e2e/native-esm/circularDependentB.mjs @@ -1,8 +1,8 @@ import circularDependentA from './circularDependentA.mjs'; export default { - id: 'circularDependentB', - get moduleA() { - return circularDependentA; - } + id: 'circularDependentB', + get moduleA() { + return circularDependentA; + }, }; diff --git a/packages/jest-runtime/src/index.ts b/packages/jest-runtime/src/index.ts index 818cc090387a..52dca2ceb599 100644 --- a/packages/jest-runtime/src/index.ts +++ b/packages/jest-runtime/src/index.ts @@ -368,7 +368,7 @@ export default class Runtime { private async loadEsmModule( modulePath: Config.Path, query = '', - isStaticImport = false + isStaticImport = false, ): Promise { const cacheKey = modulePath + query; @@ -386,7 +386,7 @@ export default class Runtime { const core = this._importCoreModule(modulePath, context); this._esmoduleRegistry.set(cacheKey, { beforeEvaluation: core, - fullyEvaluated: core + fullyEvaluated: core, }); return core; } @@ -410,7 +410,7 @@ export default class Runtime { specifier, referencingModule.identifier, referencingModule.context, - false + false, ), initializeImportMeta(meta: ImportMeta) { meta.url = pathToFileURL(modulePath).href; @@ -419,7 +419,10 @@ export default class Runtime { let resolve: (value: VMModule) => void; let reject: (value: any) => void; - let promise = new Promise((_resolve, _reject) => { resolve = _resolve; reject = _reject }); + const promise = new Promise((_resolve, _reject) => { + resolve = _resolve; + reject = _reject; + }); // add to registry before link so that circular import won't end up stack overflow this._esmoduleRegistry.set( @@ -429,8 +432,8 @@ export default class Runtime { // we shouldn't get any unhandled rejections { beforeEvaluation: Promise.resolve(module), - fullyEvaluated: promise - } + fullyEvaluated: promise, + }, ); module @@ -439,11 +442,14 @@ export default class Runtime { specifier, referencingModule.identifier, referencingModule.context, - true + true, ), ) .then(() => module.evaluate()) - .then(() => resolve(module), (e: any) => reject(e)); + .then( + () => resolve(module), + (e: any) => reject(e), + ); } const entry = this._esmoduleRegistry.get(cacheKey); @@ -451,7 +457,9 @@ export default class Runtime { // return the already resolved, pre-evaluation promise // is loaded through static import to prevent promise deadlock // because module is evaluated after all static import is resolved - const module = isStaticImport ? entry?.beforeEvaluation : entry?.fullyEvaluated; + const module = isStaticImport + ? entry?.beforeEvaluation + : entry?.fullyEvaluated; invariant(module); @@ -462,18 +470,20 @@ export default class Runtime { specifier: string, referencingIdentifier: string, context: VMContext, - isStaticImport: boolean + isStaticImport: boolean, ) { if (specifier === '@jest/globals') { const fromCache = this._esmoduleRegistry.get('@jest/globals'); if (fromCache) { - return isStaticImport ? fromCache.beforeEvaluation : fromCache.fullyEvaluated; + return isStaticImport + ? fromCache.beforeEvaluation + : fromCache.fullyEvaluated; } const globals = this.getGlobalsForEsm(referencingIdentifier, context); this._esmoduleRegistry.set('@jest/globals', { beforeEvaluation: globals, - fullyEvaluated: globals + fullyEvaluated: globals, }); return globals; From ebec391cef73a7fbb8c29886d80dad80dc6e7ecd Mon Sep 17 00:00:00 2001 From: Takeru Date: Wed, 2 Dec 2020 00:55:13 +0800 Subject: [PATCH 4/4] chore: update copyright header --- e2e/native-esm/circularDependentA.mjs | 7 +++++++ e2e/native-esm/circularDependentB.mjs | 7 +++++++ 2 files changed, 14 insertions(+) diff --git a/e2e/native-esm/circularDependentA.mjs b/e2e/native-esm/circularDependentA.mjs index 9ec3a9246f54..801bd8d8443d 100644 --- a/e2e/native-esm/circularDependentA.mjs +++ b/e2e/native-esm/circularDependentA.mjs @@ -1,3 +1,10 @@ +/** + * 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 circularDependentB from './circularDependentB.mjs'; export default { diff --git a/e2e/native-esm/circularDependentB.mjs b/e2e/native-esm/circularDependentB.mjs index 7d7003b660cd..7a81d4c331c6 100644 --- a/e2e/native-esm/circularDependentB.mjs +++ b/e2e/native-esm/circularDependentB.mjs @@ -1,3 +1,10 @@ +/** + * 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 circularDependentA from './circularDependentA.mjs'; export default {