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(jsii): improve checks around peerDependencies #2997

Merged
merged 10 commits into from
Sep 22, 2021

Conversation

rix0rrr
Copy link
Contributor

@rix0rrr rix0rrr commented Sep 22, 2021

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.

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.
@rix0rrr rix0rrr requested a review from a team September 22, 2021 10:37
@rix0rrr rix0rrr self-assigned this Sep 22, 2021
@mergify mergify bot added the contribution/core This is a PR that came from AWS. label Sep 22, 2021
Comment on lines 106 to 120
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 result;

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;
});
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
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 result;
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;
});
const result = await (argv.watch ? compiler.watch() : compiler.emit());
const allDiagnostics = [...projectInfoDiagnostics, ... result.diagnostics];
for (const diagnostic of allDiagnostics) {
utils.logDiagnostic(diagnostic, projectRoot);
}
if (result.emitSkipped) {
process.exitCode = 1;
}
})().catch((e) => {
console.error(`Error: ${e.stack}`);
process.exitCode = -1;
});

packages/jsii/lib/helpers.ts Outdated Show resolved Hide resolved
@@ -238,6 +238,20 @@ export class JsiiDiagnostic implements ts.Diagnostic {
name: 'metadata/missing-peer-dependency',
});

public static readonly JSII_0006_MISSING_DEV_DEPENDENCY = Code.warning({
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe drop a comment here stating that for now, it is not possible to increase the severity of this because it's generated before the severity settings are applied?

packages/jsii/lib/project-info.ts Outdated Show resolved Hide resolved
packages/jsii/bin/jsii.ts Outdated Show resolved Hide resolved
rix0rrr and others added 5 commits September 22, 2021 12:56
Co-authored-by: Romain Marcadier <rmuller@amazon.com>
Co-authored-by: Romain Marcadier <rmuller@amazon.com>
Co-authored-by: Romain Marcadier <rmuller@amazon.com>
@mergify
Copy link
Contributor

mergify bot commented Sep 22, 2021

Thank you for contributing! ❤️ I will now look into making sure the PR is up-to-date, then proceed to try and merge it!

@mergify mergify bot added the pr/ready-to-merge This PR is ready to be merged. label Sep 22, 2021
@mergify
Copy link
Contributor

mergify bot commented Sep 22, 2021

Merging (with squash)...

@mergify mergify bot merged commit d1ff3e6 into main Sep 22, 2021
@mergify mergify bot deleted the huijbers/peer-deps-checks branch September 22, 2021 15:53
@mergify
Copy link
Contributor

mergify bot commented Sep 22, 2021

Merging (with squash)...

@mergify mergify bot removed the pr/ready-to-merge This PR is ready to be merged. label Sep 22, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
contribution/core This is a PR that came from AWS.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

jsii should help users get devDependencies/peerDependencies right
2 participants