From 0a4cf73c2c8270d9d3789be3437666cc589d90c2 Mon Sep 17 00:00:00 2001 From: Christoph Thiede Date: Tue, 12 Oct 2021 18:36:37 +0000 Subject: [PATCH] Fix import errors from core package once again After restructuring the core package, dynamic imports did not work any longer because apparently, they must not be called from submodules. Relates #65 and https://github.com/microsoft/TypeScript/issues/43329, again. --- packages/cli/src/oclif.init.ts | 2 + packages/core/src/dependencies/sourcegraph.ts | 7 +-- packages/core/src/externalModules.ts | 19 +++++++ packages/core/src/index.ts | 49 +++++++++++++++++ packages/core/src/references/heuristic.ts | 53 ++----------------- packages/core/test/references.test.ts | 3 ++ packages/vscode-extension/src/extension.ts | 7 +-- 7 files changed, 82 insertions(+), 58 deletions(-) create mode 100644 packages/core/src/externalModules.ts diff --git a/packages/cli/src/oclif.init.ts b/packages/cli/src/oclif.init.ts index cda46f90..66b68516 100644 --- a/packages/cli/src/oclif.init.ts +++ b/packages/cli/src/oclif.init.ts @@ -1,6 +1,8 @@ import dotenv from 'dotenv' import { Hook } from '@oclif/config' +import { loadExternalModules } from 'dowdep' export const hook: Hook<'init'> = async function (options) { dotenv.config() + await loadExternalModules() } diff --git a/packages/core/src/dependencies/sourcegraph.ts b/packages/core/src/dependencies/sourcegraph.ts index 2c7bebea..8d50bab4 100644 --- a/packages/core/src/dependencies/sourcegraph.ts +++ b/packages/core/src/dependencies/sourcegraph.ts @@ -5,6 +5,7 @@ import normalizePackageData from 'normalize-package-data' import { Dependency, DependencySearcher } from './base' import { Dowdep } from '../dowdep' +import externalModules from '../externalModules' import { Package } from '../packages' import { OnlyData } from '../utils/OnlyData' @@ -147,11 +148,7 @@ class SourcegraphClient { 'count': `${limit || this.maximumLimit}` } - // Workaround for https://github.com/microsoft/vscode/issues/130367 and https://github.com/microsoft/TypeScript/issues/43329 🤯 - const dynamicImport = new Function('moduleName', 'return import(moduleName)') - const escapeRegexp: (regex: string) => string = (await dynamicImport('escape-string-regexp')).default - - const query = `"${escapeRegexp(packageName)}": ` + Object.entries(queryArgs).map(([key, value]) => `${key}:${value}`).join(' ') + const query = `"${externalModules.escapeRegexp(packageName)}": ` + Object.entries(queryArgs).map(([key, value]) => `${key}:${value}`).join(' ') const response = this.documentSpecifier.protoResponse Object.assign(response, await graphql.request(this.documentSpecifier.document, { query })) diff --git a/packages/core/src/externalModules.ts b/packages/core/src/externalModules.ts new file mode 100644 index 00000000..300d7cc2 --- /dev/null +++ b/packages/core/src/externalModules.ts @@ -0,0 +1,19 @@ +import type { Import, Options } from "parse-imports" + + +/** + * Stores external modules that have been loaded in a non-standard, weird way. + * + * The existence of this is a large workaround. Some components of our toolchain, including oclif and jest, do not support dynamic importing of pure ES modules correctly. See `loadExternalModules()` in `index.ts`. As the technique used there cannot be applied from submodules, imported modules are stored in this file. + */ +const modules = { + /** Escape RegExp special characters */ + escapeRegexp: <(regex: string) => string>undefined, + /** A blazing fast ES module imports parser. */ + parseImports: <( + code: string, + options?: Options + ) => Promise>>undefined +} + +export default modules diff --git a/packages/core/src/index.ts b/packages/core/src/index.ts index cb6b9d7a..85d9eb3f 100644 --- a/packages/core/src/index.ts +++ b/packages/core/src/index.ts @@ -16,3 +16,52 @@ export { ReferenceSearcher, ReferenceSearchStrategy } from './references' + +import pkgDir from 'pkg-dir' +import externalModules from './externalModules' + +let _loadedExternalModules = false + +/** Load external modules asynchronously that cannot be imported in the usual way. See `externalModules.ts` for details. This function must be called before using any other functionality of this package! */ +export async function loadExternalModules() { + if (_loadedExternalModules) { + return + } + + /** + * Truly awful hack! There are a few things going on here: + * - Jest (or something) can't find parse-imports by just importing its package name + * no matter what. Just give it the path to the src/index.js file + * - Workaround for https://github.com/microsoft/TypeScript/issues/43329. + * + * - TypeScript will always try to replace dynamic imports with `requires` which doesn't work for importing ESM from CJS. + * We work around by "hiding" our dynamic import in a Function constructor (terrible...). + * + * In particular, we must not extract this call into a separate module. + * This would result in sporadic unresolved promises in the jest environment. + * See #65. + * + * All of this required jest@next, ts-jest@next, AND `NODE_OPTIONS=--experimental-vm-modules` + * + * Developed with the kind help of @TomerAbach. + */ + const dynamicImport = new Function('moduleName', 'return import(moduleName)') + + externalModules.escapeRegexp = (await dynamicImport('escape-string-regexp')).default + const parseImportsIndexPath = `${await pkgDir()}/node_modules/parse-imports/src/index.js` + + try { + externalModules.parseImports = (await dynamicImport(parseImportsIndexPath)).default + } catch (parseError) { + if (!(parseError instanceof Error && 'code' in parseError && (<{ code: string }>parseError).code == 'ERR_MODULE_NOT_FOUND')) { + throw parseError + } + // This will occur if this package is imported as a local dependency from another package via a symlink. + // For now, let's handle this by assuming the depending package is a sibling of ourselves ... + // Hardcoded! So many hacks! 😭 + const parseImportsIndexPath = `${await pkgDir()}/../core/node_modules/parse-imports/src/index.js` + externalModules.parseImports = (await dynamicImport(parseImportsIndexPath)).default + } + + _loadedExternalModules = true +} diff --git a/packages/core/src/references/heuristic.ts b/packages/core/src/references/heuristic.ts index cd4ca653..80f554ba 100644 --- a/packages/core/src/references/heuristic.ts +++ b/packages/core/src/references/heuristic.ts @@ -8,6 +8,7 @@ import path from 'path' import pkgDir from 'pkg-dir' import { DeclarationImport, FilePosition, ReferenceSearcher, Reference, ReferenceLocation } from './base' +import externalModules from '../externalModules' import rex from '../utils/rex' @@ -40,25 +41,12 @@ export class HeuristicReferenceSearcher extends ReferenceSearcher { const identifierPattern = /[\p{L}\p{Nl}$_][\p{L}\p{Nl}$\p{Mn}\p{Mc}\p{Nd}\p{Pc}]*/u - /** - * Workaround for https://github.com/microsoft/TypeScript/issues/43329. - * - * TypeScript will always try to replace dynamic imports with `requires` which doesn't work for importing ESM from CJS. - * We work around by "hiding" our dynamic import in a Function constructor (terrible...). - * - * In particular, we must not extract this call into a separate module. - * This would result in sporadic unresolved promises in the jest environment. - * See #65. - */ - const dynamicImport = new Function('moduleName', 'return import(moduleName)') - const escapeRegexp: (regex: string) => string = (await dynamicImport('escape-string-regexp')).default - const requirePattern = rex` (? ${identifierPattern} ) \s* = \s* require \s* \( \s* (?['"]) - (? ${escapeRegexp(this.$package.name)} ) + (? ${externalModules.escapeRegexp!(this.$package.name)} ) ( \/ (? ${identifierPattern} ) )? @@ -154,42 +142,7 @@ export class HeuristicReferenceSearcher extends ReferenceSearcher { async* collectEsmBindings(source: string): AsyncGenerator { const imports = await (async () => { try { - // Truly awful hack! There are a few things going on here: - // - Jest (or something) can't find parse-imports by just importing its package name - // no matter what. Just give it the path to the src/index.js file - // - // - Workaround for https://github.com/microsoft/TypeScript/issues/43329. - // - // TypeScript will always try to replace dynamic imports with `requires` which doesn't work for importing ESM from CJS. - // We work around by "hiding" our dynamic import in a Function constructor (terrible...). - // - // In particular, we must not extract this call into a separate module. - // This would result in sporadic unresolved promises in the jest environment. - // See #65. - // - // - All of this required jest@next, ts-jest@next, AND `NODE_OPTIONS=--experimental-vm-modules` - const parseImportsIndexPath = `${await pkgDir()}/node_modules/parse-imports/src/index.js` - const dynamicImport = new Function('moduleName', 'return import(moduleName)') - let parseImports: ( - code: string, - options?: Options - ) => Promise> - - try { - parseImports = (await dynamicImport(parseImportsIndexPath)).default - } catch (parseError) { - if (!(parseError instanceof Error && 'code' in parseError && (<{ code: string }>parseError).code == 'ERR_MODULE_NOT_FOUND')) { - throw parseError - } - // This will occur if this package is imported as a local dependency from another package via a symlink. - // For now, let's handle this by assuming the depending package is a sibling of ourselves ... - // Hardcoded! So many hacks! 😭 - const parseImportsIndexPath = `${await pkgDir()}/../core/node_modules/parse-imports/src/index.js` - const dynamicImport = new Function('moduleName', 'return import(moduleName)') - parseImports = (await dynamicImport(parseImportsIndexPath)).default - } - - return await parseImports(source) + return await externalModules.parseImports(source) } catch (parseError) { console.warn("Error from parse-imports", { parseError, source: source.slice(0, 100), dependencyName: this.dependency.name }) // TODO: Make getter denedencyName? // This includes syntax errors but also TypeScript syntax which is not (yet?) supported by parse-imports. diff --git a/packages/core/test/references.test.ts b/packages/core/test/references.test.ts index 8b9761f4..005002bf 100644 --- a/packages/core/test/references.test.ts +++ b/packages/core/test/references.test.ts @@ -8,6 +8,7 @@ import ifCurtailed from '../src/utils/if-curtailed' import { lodashClassifyNested } from '../src/utils/lodash-classify' import { getSourceDirectory } from './_utils/sourceDirectory' import { printDiff } from './_utils/printDiff' +import { loadExternalModules } from '../src' const SOURCE_DIRECTORY = getSourceDirectory(__filename) @@ -37,6 +38,8 @@ describe('ReferenceSearcher', () => { ([packageName, expectedReferences]) => ({ strategy: strategy, packageName, expectedReferences })) ))("should find relevant references for %s", async ( { strategy, packageName, expectedReferences }) => { + await loadExternalModules() + const $package = new Package( packageName, `${SOURCE_DIRECTORY}/examples/packages/${packageName}` diff --git a/packages/vscode-extension/src/extension.ts b/packages/vscode-extension/src/extension.ts index 082a4aeb..57d4f8e7 100644 --- a/packages/vscode-extension/src/extension.ts +++ b/packages/vscode-extension/src/extension.ts @@ -1,4 +1,4 @@ -import { DeclarationLocation, Dependency, Dowdep, FilePosition, Package, Reference, ReferenceSearchStrategy } from 'dowdep' +import { DeclarationLocation, Dependency, Dowdep, FilePosition, loadExternalModules, Package, Reference, ReferenceSearchStrategy } from 'dowdep' import _ from 'lodash' import filterAsync from 'node-filter-async' import normalizePackageData from 'normalize-package-data' @@ -23,12 +23,13 @@ type PackageLike = Package | readonly Package[] /** * This method is called on the first activation event. */ -export function activate(context: vscode.ExtensionContext) { - console.log("The extension \"dowdep\" is now active.") +export async function activate(context: vscode.ExtensionContext) { + await loadExternalModules() extension = new Extension(context) extension.activate() + console.log("The extension \"dowdep\" is now active.") return extension }