fix: exampleDependencies aren't used (#68)

Rosetta has a feature to include external packages into its example compilation dependency tree, so that you can write examples in your README using packages that depend on the package you're writing the README for. You couldn't have those dependencies in your actual dependency tree, but there is a special section in `package.json` that can do it. Schematically: ``` mydep ▲ │ mypackage < write a compilable example that uses 'myinteg' ▲ │ myinteg ``` Put the following into `package.json`: ```json { "jsiiRosetta": { "exampleDependencies": { "myinteg": "^1.2.3" } } } ``` While this feature was advertised, it didn't actually work: * The dependency closure tree was built, but it was never used during compilation (we erroneously set the compilation directory to the directory of the package the sample was found). * You could not specify `*` as a version number because a package we use, `semver-intersect`, doesn't support `*`. * We need to take additional care when mixing symlinked and installed packages that `peerDependency` (e.g., `constructs`). By the original closure directory construction method, we would end up with the wrong version of `aws-cdk-lib` in the dependency closure, and 2 copies of `constructs` (leading to the `Construct is not assignable to Construct` error). To solve all of this, fully crawl the dependency tree, build a `package.json` to install everything in one go, and do use the compilation directory that we build this way. Also in this PR: * Wrap `semver-intersect` with a helper function to handle one more case and fail gracefully if we encounter more cases it doesn't handle. * Add some debug printing and a `--no-cleanup` flag for easier debugging. --- By submitting this pull request, I confirm that my contribution is made under the terms of the [Apache 2.0 license]. [Apache 2.0 license]: https://www.apache.org/licenses/LICENSE-2.0 --------- Signed-off-by: github-actions <[email protected]> Co-authored-by: github-actions <[email protected]>
aws · Apr 24, 2023 · ed8ac1a · ed8ac1a
1 parent d747938
commit ed8ac1a
Show file tree

Hide file tree

Showing 12 changed files with 277 additions and 103 deletions.
diff --git a/src/commands/extract.ts b/src/commands/extract.ts
@@ -92,6 +92,13 @@ export interface ExtractOptions {
    * @default false
    */
   readonly compressCacheToFile?: boolean;
+
+  /**
+   * Cleanup temporary directories
+   *
+   * @default true
+   */
+  readonly cleanup?: boolean;
 }
 
 export async function extractAndInfuse(assemblyLocations: string[], options: ExtractOptions): Promise<ExtractResult> {
@@ -157,7 +164,10 @@ export async function extractSnippets(
     logging.info('Translating');
     const startTime = Date.now();
 
-    const result = await translator.translateAll(snippets);
+    const result = await translator.translateAll(snippets, {
+      compilationDirectory: options.compilationDirectory,
+      cleanup: options.cleanup,
+    });
 
     const delta = (Date.now() - startTime) / 1000;
     logging.info(

diff --git a/src/jsii/assemblies.ts b/src/jsii/assemblies.ts
@@ -3,8 +3,6 @@ import * as fs from 'node:fs';
 import * as path from 'node:path';
 import { loadAssemblyFromFile, loadAssemblyFromPath, findAssemblyFile } from '@jsii/spec';
 import * as spec from '@jsii/spec';
-
-import { findDependencyDirectory, isBuiltinModule } from '../find-utils';
 import { fixturize } from '../fixtures';
 import { extractTypescriptSnippetsFromMarkdown } from '../markdown/extract-snippets';
 import {
@@ -17,9 +15,10 @@ import {
   CompilationDependency,
   INITIALIZER_METHOD_NAME,
 } from '../snippet';
+import { resolveDependenciesFromPackageJson } from '../snippet-dependencies';
 import { enforcesStrictMode } from '../strict';
 import { LanguageTablet, DEFAULT_TABLET_NAME, DEFAULT_TABLET_NAME_COMPRESSED } from '../tablets/tablets';
-import { fmap, mkDict, sortBy } from '../util';
+import { fmap, mkDict, pathExists, sortBy } from '../util';
 
 /**
  * The JSDoc tag users can use to associate non-visible metadata with an example
@@ -341,41 +340,21 @@ function withProjectDirectory(dir: string, snippet: TypeScriptSnippet) {
  * The dependencies will be taken from the package.json, and will consist of:
  *
  * - The package itself
- * - The package's dependencies and peerDependencies
+ * - The package's dependencies and peerDependencies (but NOT devDependencies). Will
+ *   symlink to the files on disk.
  * - Any additional dependencies declared in `jsiiRosetta.exampleDependencies`.
  */
 async function withDependencies(asm: LoadedAssembly, snippet: TypeScriptSnippet): Promise<TypeScriptSnippet> {
   const compilationDependencies: Record<string, CompilationDependency> = {};
 
-  compilationDependencies[asm.assembly.name] = {
-    type: 'concrete',
-    resolvedDirectory: await fsPromises.realpath(asm.directory),
-  };
+  if (await pathExists(path.join(asm.directory, 'package.json'))) {
+    compilationDependencies[asm.assembly.name] = {
+      type: 'concrete',
+      resolvedDirectory: await fsPromises.realpath(asm.directory),
+    };
+  }
 
-  Object.assign(
-    compilationDependencies,
-    mkDict(
-      await Promise.all(
-        Object.keys({ ...asm.packageJson?.dependencies, ...asm.packageJson?.peerDependencies })
-          .filter((name) => !isBuiltinModule(name))
-          .filter(
-            (name) =>
-              !asm.packageJson?.bundledDependencies?.includes(name) &&
-              !asm.packageJson?.bundleDependencies?.includes(name),
-          )
-          .map(
-            async (name) =>
-              [
-                name,
-                {
-                  type: 'concrete',
-                  resolvedDirectory: await fsPromises.realpath(await findDependencyDirectory(name, asm.directory)),
-                },
-              ] as const,
-          ),
-      ),
-    ),
-  );
+  Object.assign(compilationDependencies, await resolveDependenciesFromPackageJson(asm.packageJson, asm.directory));
 
   Object.assign(
     compilationDependencies,

diff --git a/src/main.ts b/src/main.ts
@@ -243,6 +243,11 @@ async function main() {
             describe: 'Compress the cache-to file',
             default: false,
           })
+          .options('cleanup', {
+            type: 'boolean',
+            describe: 'Clean up temporary directories',
+            default: true,
+          })
           .conflicts('loose', 'strict')
           .conflicts('loose', 'fail'),
       wrapHandler(async (args) => {
@@ -269,6 +274,7 @@ async function main() {
           loose: args.loose,
           compressTablet: args['compress-tablet'],
           compressCacheToFile: args['compress-cache'],
+          cleanup: args.cleanup,
         };
 
         const result = args.infuse

diff --git a/src/rosetta-translator.ts b/src/rosetta-translator.ts
@@ -5,7 +5,12 @@ import { TypeFingerprinter } from './jsii/fingerprinting';
 import { TARGET_LANGUAGES } from './languages';
 import * as logging from './logging';
 import { TypeScriptSnippet, completeSource } from './snippet';
-import { collectDependencies, validateAvailableDependencies, prepareDependencyDirectory } from './snippet-dependencies';
+import {
+  collectDependencies,
+  validateAvailableDependencies,
+  prepareDependencyDirectory,
+  expandWithTransitiveDependencies,
+} from './snippet-dependencies';
 import { snippetKey } from './tablets/key';
 import { LanguageTablet, TranslatedSnippet } from './tablets/tablets';
 import { translateAll, TranslateAllResult } from './translate_all';
@@ -168,6 +173,7 @@ export class RosettaTranslator {
         : { addToTablet: optionsOrAddToTablet };
 
     const exampleDependencies = collectDependencies(snippets);
+    await expandWithTransitiveDependencies(exampleDependencies);
 
     let compilationDirectory;
     let cleanCompilationDir = false;
@@ -190,7 +196,11 @@ export class RosettaTranslator {
     } finally {
       process.chdir(origDir);
       if (cleanCompilationDir) {
-        await fs.rm(compilationDirectory, { force: true, recursive: true });
+        if (options.cleanup ?? true) {
+          await fs.rm(compilationDirectory, { force: true, recursive: true });
+        } else {
+          logging.info(`Leaving directory uncleaned: ${compilationDirectory}`);
+        }
       }
     }
 
@@ -309,4 +319,9 @@ export interface TranslateAllOptions {
    * @default true
    */
   readonly addToTablet?: boolean;
+
+  /**
+   * @default true
+   */
+  readonly cleanup?: boolean;
 }
diff --git a/src/snippet-dependencies.ts b/src/snippet-dependencies.ts
@@ -3,11 +3,12 @@ import { promises as fsPromises } from 'node:fs';
 import * as fs from 'node:fs';
 import * as os from 'node:os';
 import * as path from 'node:path';
+import { PackageJson } from '@jsii/spec';
 import * as fastGlob from 'fast-glob';
 import * as semver from 'semver';
 import { intersect } from 'semver-intersect';
 
-import { findDependencyDirectory, findUp } from './find-utils';
+import { findDependencyDirectory, findUp, isBuiltinModule } from './find-utils';
 import * as logging from './logging';
 import { TypeScriptSnippet, CompilationDependency } from './snippet';
 import { mkDict, formatList, pathExists } from './util';
@@ -27,6 +28,72 @@ export function collectDependencies(snippets: TypeScriptSnippet[]) {
   return ret;
 }
 
+/**
+ * Add transitive dependencies of concrete dependencies to the array
+ *
+ * This is necessary to prevent multiple copies of transitive dependencies on disk, which
+ * jsii-based packages might not deal with very well.
+ */
+export async function expandWithTransitiveDependencies(deps: Record<string, CompilationDependency>) {
+  const pathsSeen = new Set<string>();
+  const queue = Object.values(deps).filter(isConcrete);
+
+  let next = queue.shift();
+  while (next) {
+    await addDependenciesOf(next.resolvedDirectory);
+    next = queue.shift();
+  }
+
+  async function addDependenciesOf(dir: string) {
+    if (pathsSeen.has(dir)) {
+      return;
+    }
+    pathsSeen.add(dir);
+    try {
+      const pj: PackageJson = JSON.parse(
+        await fsPromises.readFile(path.join(dir, 'package.json'), { encoding: 'utf-8' }),
+      );
+      for (const [name, dep] of Object.entries(await resolveDependenciesFromPackageJson(pj, dir))) {
+        if (!deps[name]) {
+          deps[name] = dep;
+          queue.push(dep);
+        }
+      }
+    } catch (e: any) {
+      if (e.code === 'ENOENT') {
+        return;
+      }
+      throw e;
+    }
+  }
+}
+
+/**
+ * Find the corresponding package directories for all dependencies in a package.json
+ */
+export async function resolveDependenciesFromPackageJson(packageJson: PackageJson | undefined, directory: string) {
+  return mkDict(
+    await Promise.all(
+      Object.keys({ ...packageJson?.dependencies, ...packageJson?.peerDependencies })
+        .filter((name) => !isBuiltinModule(name))
+        .filter(
+          (name) =>
+            !packageJson?.bundledDependencies?.includes(name) && !packageJson?.bundleDependencies?.includes(name),
+        )
+        .map(
+          async (name) =>
+            [
+              name,
+              {
+                type: 'concrete',
+                resolvedDirectory: await fsPromises.realpath(await findDependencyDirectory(name, directory)),
+              },
+            ] as const,
+        ),
+    ),
+  );
+}
+
 function resolveConflict(
   name: string,
   a: CompilationDependency,
@@ -47,7 +114,7 @@ function resolveConflict(
     // Intersect the ranges
     return {
       type: 'symbolic',
-      versionRange: intersect(a.versionRange, b.versionRange),
+      versionRange: myVersionIntersect(a.versionRange, b.versionRange),
     };
   }
 
@@ -79,6 +146,7 @@ function resolveConflict(
  * It's a warning if this is not true, not an error.
  */
 export async function validateAvailableDependencies(directory: string, deps: Record<string, CompilationDependency>) {
+  logging.info(`Validating dependencies at ${directory}`);
   const failures = await Promise.all(
     Object.entries(deps).flatMap(async ([name, _dep]) => {
       try {
@@ -97,6 +165,27 @@ export async function validateAvailableDependencies(directory: string, deps: Rec
   }
 }
 
+/**
+ * Intersect two semver ranges
+ *
+ * The package we are using for this doesn't support all syntaxes yet.
+ * Do some work on top.
+ */
+function myVersionIntersect(a: string, b: string): string {
+  if (a === '*') {
+    return b;
+  }
+  if (b === '*') {
+    return a;
+  }
+
+  try {
+    return intersect(a, b);
+  } catch (e: any) {
+    throw new Error(`semver-intersect does not support either '${a}' or '${b}': ${e.message}`);
+  }
+}
+
 /**
  * Prepare a temporary directory with symlinks to all the dependencies we need.
  *
@@ -112,7 +201,7 @@ export async function prepareDependencyDirectory(deps: Record<string, Compilatio
   const monorepoPackages = await scanMonoRepos(concreteDirs);
 
   const tmpDir = await fsPromises.mkdtemp(path.join(os.tmpdir(), 'rosetta'));
-  logging.info(`Preparing dependency closure at ${tmpDir}`);
+  logging.info(`Preparing dependency closure at ${tmpDir} (-vv for more details)`);
 
   // Resolved symbolic packages against monorepo
   const resolvedDeps = mkDict(
@@ -126,39 +215,38 @@ export async function prepareDependencyDirectory(deps: Record<string, Compilatio
     ]),
   );
 
-  // Use 'npm install' only for the symbolic packages. For the concrete packages,
-  // npm is going to try and find transitive dependencies as well and it won't know
-  // about monorepos.
-  const symbolicInstalls = Object.entries(resolvedDeps).flatMap(([name, dep]) =>
-    isSymbolic(dep) ? [`${name}@${dep.versionRange}`] : [],
-  );
-  const linkedInstalls = mkDict(
-    Object.entries(resolvedDeps).flatMap(([name, dep]) =>
-      isConcrete(dep) ? [[name, dep.resolvedDirectory] as const] : [],
+  const dependencies: Record<string, string> = {};
+  for (const [name, dep] of Object.entries(resolvedDeps)) {
+    if (isConcrete(dep)) {
+      logging.debug(`${name} -> ${dep.resolvedDirectory}`);
+      dependencies[name] = `file:${dep.resolvedDirectory}`;
+    } else {
+      logging.debug(`${name} @ ${dep.versionRange}`);
+      dependencies[name] = dep.versionRange;
+    }
+  }
+
+  await fsPromises.writeFile(
+    path.join(tmpDir, 'package.json'),
+    JSON.stringify(
+      {
+        name: 'examples',
+        version: '0.0.1',
+        private: true,
+        dependencies,
+      },
+      undefined,
+      2,
     ),
+    {
+      encoding: 'utf-8',
+    },
   );
 
-  // Run 'npm install' on it
-  if (symbolicInstalls.length > 0) {
-    logging.debug(`Installing example dependencies: ${symbolicInstalls.join(' ')}`);
-    cp.execSync(`npm install ${symbolicInstalls.join(' ')}`, { cwd: tmpDir, encoding: 'utf-8' });
-  }
-
-  // Symlink the rest
-  if (Object.keys(linkedInstalls).length > 0) {
-    logging.debug(`Symlinking example dependencies: ${Object.values(linkedInstalls).join(' ')}`);
-    const modDir = path.join(tmpDir, 'node_modules');
-    await Promise.all(
-      Object.entries(linkedInstalls).map(async ([name, source]) => {
-        const target = path.join(modDir, name);
-        if (!(await pathExists(target))) {
-          // Package could be namespaced, so ensure the namespace dir exists
-          await fsPromises.mkdir(path.dirname(target), { recursive: true });
-          await fsPromises.symlink(source, target, 'dir');
-        }
-      }),
-    );
-  }
+  // Run NPM install on this package.json. We need to include --force for packages
+  // that have a symbolic version in the symlinked dev tree (like "0.0.0"), but have
+  // actual version range dependencies from externally installed packages (like "^2.0.0").
+  cp.execSync(`npm install --force --loglevel error`, { cwd: tmpDir, encoding: 'utf-8' });
 
   return tmpDir;
 }
@@ -226,10 +314,6 @@ async function findMonoRepoGlobs(startingDir: string): Promise<Set<string>> {
   return ret;
 }
 
-function isSymbolic(x: CompilationDependency): x is Extract<CompilationDependency, { type: 'symbolic' }> {
-  return x.type === 'symbolic';
-}
-
 function isConcrete(x: CompilationDependency): x is Extract<CompilationDependency, { type: 'concrete' }> {
   return x.type === 'concrete';
 }

diff --git a/src/snippet.ts b/src/snippet.ts
@@ -310,7 +310,8 @@ export enum SnippetParameters {
   /**
    * What directory to resolve fixtures in for this snippet (system parameter)
    *
-   * Attached during processing, should not be used by authors.
+   * Attached during processing, should not be used by authors. Does NOT imply
+   * anything about the directory where we pretend to compile this file.
    */
   $PROJECT_DIRECTORY = '$directory',