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: improve compatibility with restricted-export modules #3205

Merged
merged 18 commits into from
Dec 7, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions packages/jsii-pacmak/lib/dependency-graph.ts
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ export async function traverseDependencyGraph(
callback: Callback,
host: TraverseDependencyGraphHost = {
readJson: fs.readJson,
resolveDependencyDirectory: util.resolveDependencyDirectory,
findDependencyDirectory: util.findDependencyDirectory,
},
): Promise<void> {
return real$traverseDependencyGraph(packageDir, callback, host, new Set());
Expand Down Expand Up @@ -49,7 +49,7 @@ export type Callback = (
*/
export interface TraverseDependencyGraphHost {
readonly readJson: typeof fs.readJson;
readonly resolveDependencyDirectory: typeof util.resolveDependencyDirectory;
readonly findDependencyDirectory: typeof util.findDependencyDirectory;
}

/**
Expand Down Expand Up @@ -87,8 +87,8 @@ async function real$traverseDependencyGraph(
...Object.keys(meta.peerDependencies ?? {}),
]);
return Promise.all(
Array.from(deps).map((dep) => {
const dependencyDir = host.resolveDependencyDirectory(packageDir, dep);
Array.from(deps).map(async (dep) => {
const dependencyDir = await host.findDependencyDirectory(dep, packageDir);
return real$traverseDependencyGraph(
dependencyDir,
callback,
Expand Down
17 changes: 13 additions & 4 deletions packages/jsii-pacmak/lib/npm-modules.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ import * as path from 'path';
import * as logging from '../lib/logging';
import { JsiiModule } from './packaging';
import { topologicalSort, Toposorted } from './toposort';
import { resolveDependencyDirectory } from './util';
import { findDependencyDirectory } from './util';

/**
* Find all modules that need to be packagerd
Expand Down Expand Up @@ -67,9 +67,18 @@ export async function findJsiiModules(
// if --recurse is set, find dependency dirs and build them.
if (recurse) {
await Promise.all(
dependencyNames
.map((dep) => resolveDependencyDirectory(realPath, dep))
.map((depDir) => visitPackage(depDir, false)),
dependencyNames.flatMap(async (dep) => {
try {
const depDir = await findDependencyDirectory(dep, realPath);
return [await visitPackage(depDir, false)];
} catch (e) {
// Some modules like `@types/node` cannot be require()d, but we also don't need them.
if (e.code !== 'MODULE_NOT_FOUND') {
throw e;
}
return [];
}
}),
);
}

Expand Down
82 changes: 72 additions & 10 deletions packages/jsii-pacmak/lib/util.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,19 +6,81 @@ import * as path from 'path';
import * as logging from './logging';

/**
* Given an npm package directory and a dependency name, returns the package directory of the dep.
* @param packageDir the root of the package declaring the dependency.
* @param dependencyName the name of the dependency to be resolved.
* @return the resolved directory path.
* Find the directory that contains a given dependency, identified by its 'package.json', from a starting search directory
*
* (This code is duplicated among jsii/jsii-pacmak/jsii-reflect. Changes should be done in all
* 3 locations, and we should unify these at some point: https://github.com/aws/jsii/issues/3236)
*/
export function resolveDependencyDirectory(
packageDir: string,
export async function findDependencyDirectory(
dependencyName: string,
): string {
const lookupPaths = [path.join(packageDir, 'node_modules')];
return path.dirname(
require.resolve(`${dependencyName}/package.json`, { paths: lookupPaths }),
searchStart: string,
) {
// Explicitly do not use 'require("dep/package.json")' because that will fail if the
// package does not export that particular file.
const entryPoint = require.resolve(dependencyName, {
paths: [searchStart],
});
RomainMuller marked this conversation as resolved.
Show resolved Hide resolved

// Search up from the given directory, looking for a package.json that matches
// the dependency name (so we don't accidentally find stray 'package.jsons').
const depPkgJsonPath = await findPackageJsonUp(
dependencyName,
path.dirname(entryPoint),
);

if (!depPkgJsonPath) {
throw new Error(
`Could not find dependency '${dependencyName}' from '${searchStart}'`,
);
}

return depPkgJsonPath;
}

/**
* Find the package.json for a given package upwards from the given directory
*
* (This code is duplicated among jsii/jsii-pacmak/jsii-reflect. Changes should be done in all
* 3 locations, and we should unify these at some point: https://github.com/aws/jsii/issues/3236)
*/
export async function findPackageJsonUp(
packageName: string,
directory: string,
) {
return findUp(directory, async (dir) => {
const pjFile = path.join(dir, 'package.json');
return (
(await fs.pathExists(pjFile)) &&
(await fs.readJson(pjFile)).name === packageName
);
});
}

/**
* Find a directory up the tree from a starting directory matching a condition
*
* Will return `undefined` if no directory matches
*
* (This code is duplicated among jsii/jsii-pacmak/jsii-reflect. Changes should be done in all
* 3 locations, and we should unify these at some point: https://github.com/aws/jsii/issues/3236)
*/
export async function findUp(
directory: string,
pred: (dir: string) => Promise<boolean>,
): Promise<string | undefined> {
// eslint-disable-next-line no-constant-condition
while (true) {
// eslint-disable-next-line no-await-in-loop
if (await pred(directory)) {
return directory;
}

const parent = path.dirname(directory);
if (parent === directory) {
return undefined;
}
directory = parent;
}
}

export interface RetryOptions {
Expand Down
15 changes: 8 additions & 7 deletions packages/jsii-pacmak/test/dependency-graph.test.ts
Original file line number Diff line number Diff line change
@@ -1,13 +1,14 @@
/* eslint-disable @typescript-eslint/require-await */
import { tmpdir } from 'os';
import { join } from 'path';

import { Callback, traverseDependencyGraph } from '../lib/dependency-graph';

const mockHost = {
readJson: jest.fn<Promise<any>, [string]>().mockName('fs.readJson'),
resolveDependencyDirectory: jest
.fn<string, [string, string]>()
.mockName('resolveDependencyDirectory'),
findDependencyDirectory: jest
.fn<Promise<string>, [string, string]>()
.mockName('findDependencyDirectory'),
};

afterEach((done) => {
Expand Down Expand Up @@ -42,7 +43,7 @@ test('de-duplicates package root directories', async () => {
: Promise.reject(new Error(`Unexpected file access: ${file}`));
});

mockHost.resolveDependencyDirectory.mockImplementation((_dir, dep) => {
mockHost.findDependencyDirectory.mockImplementation(async (dep, _dir) => {
const result = packages[dep]?.root;
if (result == null) {
throw new Error(`Unknown dependency: ${dep}`);
Expand All @@ -63,7 +64,7 @@ test('de-duplicates package root directories', async () => {
}

expect(mockHost.readJson).toHaveBeenCalledTimes(3);
expect(mockHost.resolveDependencyDirectory).toHaveBeenCalledTimes(3);
expect(mockHost.findDependencyDirectory).toHaveBeenCalledTimes(3);
});

test('stops traversing when callback returns false', async () => {
Expand All @@ -90,7 +91,7 @@ test('stops traversing when callback returns false', async () => {
: Promise.reject(new Error(`Unexpected file access: ${file}`));
});

mockHost.resolveDependencyDirectory.mockImplementation((_dir, dep) => {
mockHost.findDependencyDirectory.mockImplementation(async (dep, _dir) => {
const result = packages[dep]?.root;
if (result == null) {
throw new Error(`Unknown dependency: ${dep}`);
Expand All @@ -110,5 +111,5 @@ test('stops traversing when callback returns false', async () => {
expect(cb).toHaveBeenCalledWith(packages.B.root, packages.B.meta, false);

expect(mockHost.readJson).toHaveBeenCalledTimes(2);
expect(mockHost.resolveDependencyDirectory).toHaveBeenCalledTimes(1);
expect(mockHost.findDependencyDirectory).toHaveBeenCalledTimes(1);
});
Original file line number Diff line number Diff line change
Expand Up @@ -100,6 +100,8 @@ async function mockSourceDirectory<T>(
},
{ spaces: 2 },
);
// Necessary otherwise 'require.resolve()' will fail
await fs.writeFile(join(pkgDir, 'index.js'), 'module.exports = {};');
}
/* eslint-enable no-await-in-loop */

Expand Down
55 changes: 23 additions & 32 deletions packages/jsii-reflect/lib/type-system.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import * as jsii from '@jsii/spec';
import * as fs from 'fs-extra';
import * as path from 'path';
import { promisify } from 'util';

import { Assembly } from './assembly';
import { ClassType } from './class';
Expand All @@ -10,6 +10,7 @@ import { Method } from './method';
import { ModuleLike } from './module-like';
import { Property } from './property';
import { Type } from './type';
import { findDependencyDirectory } from './util';

export class TypeSystem {
/**
Expand All @@ -34,30 +35,32 @@ export class TypeSystem {
packageRoot: string,
options: { validate?: boolean } = {},
): Promise<void> {
// eslint-disable-next-line @typescript-eslint/no-var-requires, @typescript-eslint/no-require-imports
const pkg = require(path.resolve(packageRoot, 'package.json'));
const pkg = await fs.readJson(path.resolve(packageRoot, 'package.json'));

for (const dep of dependenciesOf(pkg)) {
// Filter jsii dependencies
const depPkgJsonPath = require.resolve(`${dep}/package.json`, {
paths: [packageRoot],
});
// eslint-disable-next-line @typescript-eslint/no-var-requires, @typescript-eslint/no-require-imports
const depPkgJson = require(depPkgJsonPath);
// eslint-disable-next-line no-await-in-loop
const depDir = await findDependencyDirectory(dep, packageRoot);

// eslint-disable-next-line no-await-in-loop
const depPkgJson = await fs.readJson(path.join(depDir, 'package.json'));
if (!depPkgJson.jsii) {
continue;
}

// eslint-disable-next-line no-await-in-loop
await this.loadModule(path.dirname(depPkgJsonPath), options);
await this.loadModule(depDir, options);
}
}

/**
* Loads a jsii module or a single .jsii file into the type system.
*
* If `fileOrDirectory` is a directory, it will be treated as a jsii npm module,
* and its dependencies (as determined by its 'package.json' file) will be loaded
* as well.
*
* If `fileOrDirectory` is a file, it will be treated as a single .jsii file.
* If `fileOrDirectory` is a directory, it will be treated as a jsii npm module.
* No dependencies will be loaded. You almost never want this.
*
* Not validating makes the difference between loading assemblies with lots
* of dependencies (such as app-delivery) in 90ms vs 3500ms.
Expand All @@ -69,7 +72,7 @@ export class TypeSystem {
fileOrDirectory: string,
options: { validate?: boolean } = {},
) {
if ((await stat(fileOrDirectory)).isDirectory()) {
if ((await fs.stat(fileOrDirectory)).isDirectory()) {
return this.loadModule(fileOrDirectory, options);
}
return this.loadFile(fileOrDirectory, { ...options, isRoot: true });
Expand All @@ -92,7 +95,9 @@ export class TypeSystem {
isRoot = false,
) {
const filePath = path.join(moduleDirectory, 'package.json');
const pkg = JSON.parse((await readFile(filePath)).toString());
const pkg = JSON.parse(
await fs.readFile(filePath, { encoding: 'utf-8' }),
);
if (!pkg.jsii) {
throw new Error(`No "jsii" section in ${filePath}`);
}
Expand Down Expand Up @@ -134,11 +139,11 @@ export class TypeSystem {
continue;
}

const depDir = require.resolve(`${name}/package.json`, {
paths: [moduleDirectory],
});
// eslint-disable-next-line no-await-in-loop
await _loadModule.call(this, path.dirname(depDir));
const depDir = await findDependencyDirectory(name, moduleDirectory);

// eslint-disable-next-line no-await-in-loop
await _loadModule.call(this, depDir);
}

return root;
Expand Down Expand Up @@ -300,7 +305,7 @@ export class TypeSystem {
* @param validate Whether to validate the assembly or just assume it matches the schema
*/
private async loadAssembly(file: string, validate = true) {
const spec = JSON.parse((await readFile(file)).toString());
const spec = JSON.parse(await fs.readFile(file, { encoding: 'utf-8' }));
rix0rrr marked this conversation as resolved.
Show resolved Hide resolved
const ass = validate
? jsii.validateAssembly(spec)
: (spec as jsii.Assembly);
Expand Down Expand Up @@ -341,17 +346,3 @@ function flatMap<T, R>(
.map(mapper)
.reduce((acc, elt) => acc.concat(elt), new Array<R>());
}

function stat(p: string) {
// just-in-time require so that this file can be loaded in browsers as well.
// eslint-disable-next-line @typescript-eslint/no-require-imports,@typescript-eslint/no-var-requires
const fs = require('fs');
return promisify(fs.stat)(p);
}

function readFile(p: string) {
// just-in-time require so that this file can be loaded in browsers as well.
// eslint-disable-next-line @typescript-eslint/no-require-imports,@typescript-eslint/no-var-requires
const fs = require('fs');
return promisify(fs.readFile)(p);
}
Loading