Skip to content

Conversation

@mrgrain
Copy link
Contributor

@mrgrain mrgrain commented Mar 19, 2025

Improves the error output for plugin loading. We are loosing the green highlight of the plugin name because I doesn't feel right to have formatting inside the error. On the upside, we are loosing stack traces that are irrelevant to the user.


Old:
image

New:
image

Old:
image

New:
image

Also changes all of cli.ts to use the IoHost


By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license

@aws-cdk-automation aws-cdk-automation requested a review from a team March 19, 2025 17:57
@github-actions github-actions bot added the p2 label Mar 19, 2025
@mrgrain mrgrain force-pushed the mrgrain/chore/plugin-load-error branch from 45897d6 to bb556b2 Compare March 19, 2025 18:53
@mrgrain mrgrain changed the title chore(cli): don't double up on plugin errors fix(cli): don't double up on plugin errors Mar 19, 2025
@mrgrain mrgrain changed the title fix(cli): don't double up on plugin errors fix(cli): don't double up on plugin errors and don't show pointless stack trace Mar 19, 2025
@mrgrain mrgrain marked this pull request as ready for review March 19, 2025 18:54
@mrgrain mrgrain temporarily deployed to integ-approval March 19, 2025 19:12 — with GitHub Actions Inactive
@codecov-commenter
Copy link

codecov-commenter commented Mar 19, 2025

Codecov Report

Attention: Patch coverage is 43.42105% with 43 lines in your changes missing coverage. Please review.

Project coverage is 85.29%. Comparing base (174c2e7) to head (6f17b41).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
packages/aws-cdk/lib/cli/pretty-print-error.ts 13.88% 31 Missing ⚠️
packages/aws-cdk/lib/cli/cli.ts 41.17% 10 Missing ⚠️
packages/aws-cdk/lib/api/plugin/plugin.ts 0.00% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #262      +/-   ##
==========================================
+ Coverage   84.92%   85.29%   +0.37%     
==========================================
  Files         219      220       +1     
  Lines       36577    36619      +42     
  Branches     4555     4458      -97     
==========================================
+ Hits        31062    31235     +173     
+ Misses       5371     5285      -86     
+ Partials      144       99      -45     
Flag Coverage Δ
suite.unit 85.29% <43.42%> (+0.37%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

},
tsconfig: {
compilerOptions: {
lib: ['es2019', 'es2022.error'],
Copy link
Contributor

Choose a reason for hiding this comment

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

What do these do?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

es2019 is what we had before (the default, but also outdated)

es2022.error adds type support for Error.cause (which is available since Node 16)

} catch (e: any) {
error(`Unable to resolve plugin ${chalk.green(plugin)}: ${e.stack}`);
throw new ToolkitError(`Unable to resolve plug-in: ${plugin}`);
throw new ToolkitError(`Unable to resolve plug-in: Cannot find module '${plugin}'`);
Copy link
Contributor

Choose a reason for hiding this comment

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

Are you sure it can only happen because the module cannot be found? Why not use withCause here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As per the docs, this is the only error case:
https://nodejs.org/api/modules.html#requireresolverequest-options

Not using withCause here because the generated Node error contains the Require Stack as part of the error message. See first screenshot. That stack is inherently useless to our users, as it just says tells them that we attempt to load a plugin from the cli code.

I can add some comments around this.

@aws-cdk-automation aws-cdk-automation added this pull request to the merge queue Mar 20, 2025
@mrgrain mrgrain removed this pull request from the merge queue due to a manual request Mar 20, 2025
@mrgrain mrgrain added this pull request to the merge queue Mar 20, 2025
@mrgrain mrgrain removed this pull request from the merge queue due to a manual request Mar 20, 2025
@mrgrain mrgrain force-pushed the mrgrain/chore/plugin-load-error branch from bc54c4d to 941315e Compare March 20, 2025 14:08
@mrgrain mrgrain added the pr/exempt-integ-test Skips the integ test steps if set. label Mar 20, 2025
@mrgrain mrgrain closed this Mar 20, 2025
@mrgrain mrgrain reopened this Mar 20, 2025
@mrgrain mrgrain enabled auto-merge March 20, 2025 14:08
@mrgrain mrgrain added this pull request to the merge queue Mar 20, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to a conflict with the base branch Mar 20, 2025
@mrgrain mrgrain force-pushed the mrgrain/chore/plugin-load-error branch from 941315e to 0e6f50c Compare March 20, 2025 14:33
@mrgrain mrgrain added this pull request to the merge queue Mar 20, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Mar 20, 2025
@mrgrain mrgrain added this pull request to the merge queue Mar 20, 2025
Merged via the queue into main with commit d8eeea8 Mar 20, 2025
11 checks passed
@mrgrain mrgrain deleted the mrgrain/chore/plugin-load-error branch March 20, 2025 16:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

p2 pr/exempt-integ-test Skips the integ test steps if set.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants