Skip to content

Commit

Permalink
fix(jsii): improve checks around peerDependencies (#2997)
Browse files Browse the repository at this point in the history 
There are two changes here:

- The check that forces package authors to list all `dependencies` in
  `peerDependencies` is superfluous, and has been removed. This
  effectively removes the `--fix-peer-dependencies` flag as well.
- Add a warning to tell people that when they add a `peerDependency`,
  they should have a dependency in `devDependencies` as well.

Fixes #2994.



---

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
  • Loading branch information
@rix0rrr
rix0rrr authored Sep 22, 2021
1 parent e465b6f commit d1ff3e6
Show file tree
Hide file tree
Showing 5 changed files with 130 additions and 133 deletions.
38 changes: 18 additions & 20 deletions packages/jsii/bin/jsii.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@ import '@jsii/check-node/run';

import * as log4js from 'log4js';
import * as path from 'path';
import * as process from 'process';
import * as yargs from 'yargs';

import { Compiler } from '../lib/compiler';
Expand Down Expand Up @@ -41,7 +40,8 @@ const warningTypes = Object.keys(enabledWarnings);
.option('fix-peer-dependencies', {
type: 'boolean',
default: true,
desc: 'Automatically add missing entries in the peerDependencies section of package.json',
desc: 'This option no longer has any effect.',
hidden: true,
})
.options('fail-on-warnings', {
alias: 'Werr',
Expand Down Expand Up @@ -79,9 +79,8 @@ const warningTypes = Object.keys(enabledWarnings);
path.resolve(process.cwd(), argv.PROJECT_ROOT),
);

const projectInfo = await loadProjectInfo(projectRoot, {
fixPeerDependencies: argv['fix-peer-dependencies'],
});
const { projectInfo, diagnostics: projectInfoDiagnostics } =
await loadProjectInfo(projectRoot);

// disable all silenced warnings
for (const key of argv['silence-warnings']) {
Expand All @@ -105,21 +104,20 @@ const warningTypes = Object.keys(enabledWarnings);
stripDeprecated: argv['strip-deprecated'],
});

const result = argv.watch ? compiler.watch() : compiler.emit();
return { projectRoot, emitResult: await result };
})()
.then(({ projectRoot, emitResult }) => {
for (const diagnostic of emitResult.diagnostics) {
utils.logDiagnostic(diagnostic, projectRoot);
}
if (emitResult.emitSkipped) {
process.exit(1);
}
})
.catch((e) => {
console.error(`Error: ${e.stack}`);
process.exit(-1);
});
const emitResult = await (argv.watch ? compiler.watch() : compiler.emit());

const allDiagnostics = [...projectInfoDiagnostics, ...emitResult.diagnostics];

for (const diagnostic of allDiagnostics) {
utils.logDiagnostic(diagnostic, projectRoot);
}
if (emitResult.emitSkipped) {
process.exitCode = 1;
}
})().catch((e) => {
console.error(`Error: ${e.stack}`);
process.exitCode = -1;
});

function _configureLog4js(verbosity: number) {
log4js.configure({
Expand Down
7 changes: 4 additions & 3 deletions packages/jsii/lib/helpers.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -145,9 +145,10 @@ async function makeProjectInfo(
spaces: 2,
});

return loadProjectInfo(path.resolve(process.cwd(), '.'), {
fixPeerDependencies: true,
});
const { projectInfo } = await loadProjectInfo(
path.resolve(process.cwd(), '.'),
);
return projectInfo;
}

export type PackageInfo = {
Expand Down
16 changes: 16 additions & 0 deletions packages/jsii/lib/jsii-diagnostic.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -238,6 +238,22 @@ export class JsiiDiagnostic implements ts.Diagnostic {
name: 'metadata/missing-peer-dependency',
});

// NOTE: currently not possible to change the severity of this code,
// as it's being emitted before the overrides have been loaded
public static readonly JSII_0006_MISSING_DEV_DEPENDENCY = Code.warning({
code: 6,
formatter: (
dependencyName: string,
peerRange: string,
minVersion: string,
actual: string,
) =>
`A "peerDependency" on "${dependencyName}" at "${peerRange}" means you ` +
`should take a "devDependency" on "${dependencyName}" at "${minVersion}" ` +
`(found "${actual}")"`,
name: 'metadata/missing-dev-dependency',
});

//////////////////////////////////////////////////////////////////////////////
// 1000 => 1999 -- TYPESCRIPT LANGUAGE RESTRICTIONS

Expand Down
96 changes: 46 additions & 50 deletions packages/jsii/lib/project-info.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import * as semver from 'semver';
import { intersect } from 'semver-intersect';
import * as ts from 'typescript';

import { JsiiDiagnostic } from './jsii-diagnostic';
import { parsePerson, parseRepository } from './utils';

// eslint-disable-next-line @typescript-eslint/no-var-requires, @typescript-eslint/no-require-imports
Expand Down Expand Up @@ -55,14 +56,20 @@ export interface ProjectInfo {
readonly bin?: { readonly [name: string]: string };
}

export interface ProjectInfoResult {
readonly projectInfo: ProjectInfo;
readonly diagnostics: readonly ts.Diagnostic[];
}

export async function loadProjectInfo(
projectRoot: string,
{ fixPeerDependencies }: { fixPeerDependencies: boolean },
): Promise<ProjectInfo> {
): Promise<ProjectInfoResult> {
const packageJsonPath = path.join(projectRoot, 'package.json');
// eslint-disable-next-line @typescript-eslint/no-var-requires,@typescript-eslint/no-require-imports
const pkg = require(packageJsonPath);

const diagnostics: ts.Diagnostic[] = [];

let bundleDependencies: { [name: string]: string } | undefined;
for (const name of pkg.bundleDependencies ?? pkg.bundledDependencies ?? []) {
const version = pkg.dependencies?.[name];
Expand All @@ -79,53 +86,34 @@ export async function loadProjectInfo(
}

bundleDependencies = bundleDependencies ?? {};
bundleDependencies[name] = _resolveVersion(version, projectRoot).version!;
bundleDependencies[name] = _resolveVersion(version, projectRoot).version;
}

let addedPeerDependency = false;
Object.entries(pkg.dependencies ?? {}).forEach(([name, version]) => {
if (name in (bundleDependencies ?? {})) {
return;
}
version = _resolveVersion(version as any, projectRoot).version;
pkg.peerDependencies = pkg.peerDependencies ?? {};
const peerVersion = _resolveVersion(
pkg.peerDependencies[name],
projectRoot,
).version;
if (peerVersion === version) {
return;
}
if (!fixPeerDependencies) {
if (peerVersion) {
throw new Error(
`The "package.json" file has different version requirements for "${name}" in "dependencies" (${
version as any
}) 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 as any
}`,
);
} else {
LOG.warn(
`Recording missing "peerDependency" on "${name}" at ${version as any}`,
// Check peerDependencies are also in devDependencies
// You need this to write tests properly. There are probably cases where
// it makes sense to have this different, so most of what this checking
// produces is warnings, not errors.
const devDependencies = pkg.devDependencies ?? {};
for (const [name, rng] of Object.entries(pkg.peerDependencies ?? {})) {
const range = new semver.Range(
_resolveVersion(rng as string, projectRoot).version,
);
const minVersion = semver.minVersion(range)?.raw;

if (
!(name in devDependencies) ||
devDependencies[name] !== `${minVersion}`
) {
diagnostics.push(
JsiiDiagnostic.JSII_0006_MISSING_DEV_DEPENDENCY.createDetached(
name,
`${rng as any}`,
`${minVersion}`,
`${devDependencies[name]}`,
),
);
continue;
}
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 } = {};
Expand Down Expand Up @@ -158,7 +146,7 @@ export async function loadProjectInfo(
pkg.jsii?.metadata,
);

return {
const projectInfo = {
projectRoot,
packageJson: pkg,

Expand Down Expand Up @@ -226,6 +214,7 @@ export async function loadProjectInfo(
bin: pkg.bin,
diagnostics: _loadDiagnostics(pkg.jsii?.diagnostics),
};
return { projectInfo, diagnostics };
}

function _guessRepositoryType(url: string): string {
Expand Down Expand Up @@ -260,7 +249,7 @@ async function _loadDependencies(
dependencies[name],
searchPath,
);
const version = new semver.Range(versionString!);
const version = new semver.Range(versionString);
if (!version) {
throw new Error(
`Invalid semver expression for ${name}: ${versionString}`,
Expand All @@ -277,8 +266,8 @@ async function _loadDependencies(
}
packageVersions[assm.name] =
packageVersions[assm.name] != null
? intersect(versionString!, packageVersions[assm.name])
: versionString!;
? intersect(versionString, packageVersions[assm.name])
: versionString;
transitiveAssemblies[assm.name] = assm;
const pkgDir = path.dirname(pkg);
if (assm.dependencies) {
Expand Down Expand Up @@ -423,10 +412,17 @@ function _validateStability(
return stability as spec.Stability;
}

/**
* Resolves an NPM package specifier to a version range
*
* If it was already a version range, return it. If it the
* package references a local file, return the version that
* package is at.
*/
function _resolveVersion(
dep: string,
searchPath: string,
): { version: string | undefined; localPackage?: string } {
): { version: string; localPackage?: string } {
const matches = /^file:(.+)$/.exec(dep);
if (!matches) {
return { version: dep };
Expand Down
Loading

0 comments on commit d1ff3e6

Please sign in to comment.