Skip to content

Commit

Permalink
feat(jsii): Enforce use of peerDependencies
Browse files Browse the repository at this point in the history 
Any direct dependency to another jsii module must be recorded as a
peerDependency as well, guaranteeing a consistent behavior of
dependencies across all possible jsii runtimes.

BREAKING CHANGE: All direct dependencies must be duplicated in
                 peerDependencies unless they are in bundledDependencies.

Fixes #361
  • Loading branch information
@RomainMuller
RomainMuller committed Apr 2, 2019
1 parent 8ff9137 commit f265ea6
Show file tree
Hide file tree
Showing 7 changed files with 126 additions and 123 deletions.
11 changes: 6 additions & 5 deletions packages/jsii-spec/lib/spec.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -155,17 +155,18 @@ export interface PackageVersion {
dependencies?: { [assembly: string]: PackageVersion };

/**
* Indicates if this dependency is a peer dependency or a normal dependency.
* Indicates if this dependency is a direct (peer) dependency or a
* transitive dependency.
*
* Peer dependencies are expected to be explicitly defined by the user of
* this library instead of brought in as transitive dependencies.
*
* jsii enforces that if this module exports a type from a dependency, this
* dependency must be defined as a peer and not as a normal dependency.
* Otherwise, it would be impossible to safely use two versions of this
* dependency in a closure.
* jsii enforces that any direct dependency on another jsii module is also
* defined as a peerDependency. Otherwise, it would be impossible to safely
* use two versions of this dependency in a closure.
*
* @see https://github.com/awslabs/aws-cdk/issues/979
* @see https://github.com/awslabs/jsii/issues/361
* @see https://nodejs.org/en/blog/npm/peer-dependencies/
*/
peer?: boolean;
Expand Down
3 changes: 0 additions & 3 deletions packages/jsii/bin/jsii-fix-peers
View file

This file was deleted.

95 changes: 0 additions & 95 deletions packages/jsii/bin/jsii-fix-peers.ts
View file

This file was deleted.

14 changes: 11 additions & 3 deletions packages/jsii/bin/jsii.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -12,8 +12,16 @@ import { VERSION } from '../lib/version';
.env('JSII')
.option('watch', { alias: 'w', type: 'boolean', desc: 'Watch for file changes and recompile automatically' })
.option('verbose', { alias: 'v', type: 'count', desc: 'Increase the verbosity of output', global: true })
// tslint:disable-next-line:max-line-length
.option('project-references', { alias: 'r', type: 'boolean', desc: 'Generate TypeScript project references (also [package.json].jsii.projectReferences)' })
.option('project-references', {
alias: 'r',
type: 'boolean',
desc: 'Generate TypeScript project references (also [package.json].jsii.projectReferences)'
})
.option('fix-peer-dependencies', {
type: 'boolean',
default: true,
desc: 'Automatically add missing entries in the peerDependencies section of package.json'
})
.help()
.version(VERSION)
.argv;
Expand All @@ -23,7 +31,7 @@ import { VERSION } from '../lib/version';
const projectRoot = path.normalize(path.resolve(process.cwd(), argv._[0] || '.'));

const compiler = new Compiler({
projectInfo: await loadProjectInfo(projectRoot),
projectInfo: await loadProjectInfo(projectRoot, argv['fix-peer-dependencies']),
watch: argv.watch,
projectReferences: argv['project-references']
});
Expand Down
2 changes: 1 addition & 1 deletion packages/jsii/lib/assembler.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -100,7 +100,7 @@ export class Assembler implements Emitter {
version: this.projectInfo.version,
description: this.projectInfo.description || this.projectInfo.name,
license: this.projectInfo.license,
homepage: this.projectInfo.homepage || this.projectInfo.repository.url,
homepage: this.projectInfo.homepage || this.projectInfo.repository.url,
author: this.projectInfo.author,
contributors: this.projectInfo.contributors && [...this.projectInfo.contributors],
repository: this.projectInfo.repository,
Expand Down
42 changes: 37 additions & 5 deletions packages/jsii/lib/project-info.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -38,23 +38,55 @@ export interface ProjectInfo {
readonly projectReferences?: boolean;
}

export async function loadProjectInfo(projectRoot: string): Promise<ProjectInfo> {
const pkg = require(path.join(projectRoot, 'package.json'));
export async function loadProjectInfo(projectRoot: string, fixPeerDependencies: boolean): Promise<ProjectInfo> {
const packageJsonPath = path.join(projectRoot, 'package.json');
const pkg = require(packageJsonPath);

const bundleDependencies: { [name: string]: string } = {};
(pkg.bundleDependencies || pkg.bundledDependencies || []).forEach((name: string) => {
const version = pkg.dependencies && pkg.dependencies[name];
if (!version) {
throw new Error(`The "package.json" has "${name}" in "bundleDependencies", but it is not declared in "dependencies"`);
throw new Error(`The "package.json" file has "${name}" in "bundleDependencies", but it is not declared in "dependencies"`);
}

if (pkg.peerDependencies && name in pkg.peerDependencies) {
throw new Error(`The "package.json" has "${name}" in "bundleDependencies", and also in "peerDependencies"`);
throw new Error(`The "package.json" file has "${name}" in "bundleDependencies", and also in "peerDependencies"`);
}

bundleDependencies[name] = version;
});

let addedPeerDependency = false;
Object.entries(pkg.dependencies || {}).forEach(([name, version]) => {
if (name in bundleDependencies) {
return;
}
pkg.peerDependencies = pkg.peerDependencies || {};
const peerVersion = pkg.peerDependencies[name];
if (peerVersion === version) {
return;
}
if (!fixPeerDependencies) {
if (peerVersion) {
// tslint:disable-next-line: max-line-length
throw new Error(`The "package.json" file has different version requirements for "${name}" in "dependencies" (${version}) versus "peerDependencies" (${peerVersion})`);
}
throw new Error(`The "package.json" file has "${name}" in "dependencies", but not in "peerDependencies"`);
}
if (peerVersion) {
LOG.warn(`Changing "peerDependency" on "${name}" from "${peerVersion}" to ${version}`);
} else {
LOG.warn(`Recording missing "peerDependency" on "${name}" at ${version}`);
}
pkg.peerDependencies[name] = version;
addedPeerDependency = true;
});
// Re-write "package.json" if we fixed up "peerDependencies" and were told to automatically fix.
// Yes, we should never have addedPeerDependencies if not fixPeerDependency, but I still check again.
if (addedPeerDependency && fixPeerDependencies) {
await fs.writeJson(packageJsonPath, pkg, { encoding: 'utf8', spaces: 2 });
}

const transitiveAssemblies: { [name: string]: spec.Assembly } = {};
const dependencies =
await _loadDependencies(pkg.dependencies, projectRoot, transitiveAssemblies, new Set<string>(Object.keys(bundleDependencies)));
Expand Down Expand Up @@ -107,7 +139,7 @@ function _guessRepositoryType(url: string): string {
throw new Error(`The "package.json" file must specify the "repository.type" attribute (could not guess from ${url})`);
}

async function _loadDependencies(dependencies: { [name: string]: string | spec.PackageVersion } | undefined,
async function _loadDependencies(dependencies: { [name: string]: string | spec.PackageVersion } | undefined,
searchPath: string,
transitiveAssemblies: { [name: string]: spec.Assembly },
bundled = new Set<string>()): Promise<spec.Assembly[]> {
Expand Down
82 changes: 71 additions & 11 deletions packages/jsii/test/test.project-info.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,15 +18,16 @@ const BASE_PROJECT = {
jsii: {
targets: { foo: { bar: 'baz' } }
},
dependencies: { 'jsii-test-dep': '^1.2.3' }
dependencies: { 'jsii-test-dep': '^1.2.3' },
peerDependencies: { 'jsii-test-dep': '^1.2.3' }
};

export = nodeunit.testCase({
loadProjectInfo: {
async 'loads valid project'(test: nodeunit.Test) {
await _withTestProject(async projectRoot => {
try {
const info = await loadProjectInfo(projectRoot);
const info = await loadProjectInfo(projectRoot, false);
test.equal(info.name, BASE_PROJECT.name);
test.equal(info.version, BASE_PROJECT.version);
test.equal(info.description, BASE_PROJECT.description);
Expand All @@ -51,7 +52,7 @@ export = nodeunit.testCase({
async 'loads valid project (UNLICENSED)'(test: nodeunit.Test) {
await _withTestProject(async projectRoot => {
try {
const info = await loadProjectInfo(projectRoot);
const info = await loadProjectInfo(projectRoot, false);
test.equal(info && info.license, 'UNLICENSED');
} catch (e) {
test.ifError(e);
Expand All @@ -66,7 +67,7 @@ export = nodeunit.testCase({
async 'loads valid project (using bundleDependencies)'(test: nodeunit.Test) {
await _withTestProject(async projectRoot => {
try {
const info = await loadProjectInfo(projectRoot);
const info = await loadProjectInfo(projectRoot, false);
test.deepEqual(info.bundleDependencies, { bundled: '^1.2.3' });
} catch (e) {
test.ifError(e);
Expand All @@ -82,7 +83,7 @@ export = nodeunit.testCase({
async 'loads valid project (using bundledDependencies)'(test: nodeunit.Test) {
await _withTestProject(async projectRoot => {
try {
const info = await loadProjectInfo(projectRoot);
const info = await loadProjectInfo(projectRoot, false);
test.deepEqual(info.bundleDependencies, { bundled: '^1.2.3' });
} catch (e) {
test.ifError(e);
Expand All @@ -99,7 +100,7 @@ export = nodeunit.testCase({
const contributors = [{ name: 'foo', email: 'nobody@amazon.com' }];
await _withTestProject(async projectRoot => {
try {
const info = await loadProjectInfo(projectRoot);
const info = await loadProjectInfo(projectRoot, false);
test.deepEqual(info && info.contributors && info.contributors.map(_stripUndefined),
contributors.map(c => ({ ...c, roles: ['contributor'] })));
} catch (e) {
Expand All @@ -114,7 +115,7 @@ export = nodeunit.testCase({
await _withTestProject(async projectRoot => {
let error: Error | undefined;
try {
await loadProjectInfo(projectRoot);
await loadProjectInfo(projectRoot, false);
} catch (e) {
error = e;
} finally {
Expand All @@ -131,7 +132,7 @@ export = nodeunit.testCase({
await _withTestProject(async projectRoot => {
let error: Error | undefined;
try {
await loadProjectInfo(projectRoot);
await loadProjectInfo(projectRoot, false);
} catch (e) {
error = e;
} finally {
Expand All @@ -148,7 +149,7 @@ export = nodeunit.testCase({
await _withTestProject(async projectRoot => {
let error: Error | undefined;
try {
await loadProjectInfo(projectRoot);
await loadProjectInfo(projectRoot, false);
} catch (e) {
error = e;
} finally {
Expand All @@ -158,8 +159,66 @@ export = nodeunit.testCase({
}
}, info => {
info.dependencies[TEST_DEP_ASSEMBLY.name] = '^1.2.5';
info.peerDependencies[TEST_DEP_ASSEMBLY.name] = '^1.2.5';
});
}
},

async 'fails to load with missing peerDependency (refusing to auto-fix)'(test: nodeunit.Test) {
await _withTestProject(async projectRoot => {
let error: Error | undefined;
try {
await loadProjectInfo(projectRoot, false);
} catch (e) {
error = e;
} finally {
test.throws(() => { if (error) { throw error; } },
`The "package.json" file has "${TEST_DEP_ASSEMBLY.name}" in "dependencies", but not in "peerDependencies"`);
test.done();
}
}, info => {
delete info.peerDependencies[TEST_DEP_ASSEMBLY.name];
});
},

async 'loads with missing peerDependency (when auto-fixing)'(test: nodeunit.Test) {
await _withTestProject(async projectRoot => {
await loadProjectInfo(projectRoot, true);
const info = require(path.join(projectRoot, 'package.json'));
test.equal(info.peerDependencies[TEST_DEP_ASSEMBLY.name], "^1.2.3");
test.done();
}, info => {
delete info.peerDependencies[TEST_DEP_ASSEMBLY.name];
});
},

async 'fails to load with inconsistent peerDependency (refusing to auto-fix)'(test: nodeunit.Test) {
await _withTestProject(async projectRoot => {
let error: Error | undefined;
try {
await loadProjectInfo(projectRoot, false);
} catch (e) {
error = e;
} finally {
test.throws(() => { if (error) { throw error; } },
// tslint:disable-next-line: max-line-length
`The "package.json" file has different version requirements for "${TEST_DEP_ASSEMBLY.name}" in "dependencies" (^1.2.3) versus "peerDependencies" (^42.1337.0)`);
test.done();
}
}, info => {
info.peerDependencies[TEST_DEP_ASSEMBLY.name] = '^42.1337.0';
});
},

async 'loads with inconsistent peerDependency (when auto-fixing)'(test: nodeunit.Test) {
await _withTestProject(async projectRoot => {
await loadProjectInfo(projectRoot, true);
const info = require(path.join(projectRoot, 'package.json'));
test.equal(info.peerDependencies[TEST_DEP_ASSEMBLY.name], "^1.2.3");
test.done();
}, info => {
info.peerDependencies[TEST_DEP_ASSEMBLY.name] = '^42.1337.0';
});
},
}
});

Expand All @@ -175,7 +234,8 @@ const TEST_DEP_ASSEMBLY: spec.Assembly = {
fingerprint: 'F1NG3RPR1N7',
dependencies: {
'jsii-test-dep-dep': {
version: '3.2.1'
version: '3.2.1',
peer: true,
}
}
};
Expand Down

0 comments on commit f265ea6

Please sign in to comment.