-
Notifications
You must be signed in to change notification settings - Fork 246
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: remove the "comments rewriter" #3368
Conversation
The comments rewriter was used to prefix documentation comments with the stability level of the associated API (e.g: `(experimental)` or `(deprecated)`). This was introduced to palliate insufficient IDE support for `@deprecated` and the mixing of `@stable` and `@experimental` APIs in CDK v1. Since then, the TypeScript language server has added support for `@deprecated` IDE features, and CDK v2 moved all `@experimental` APIs to separate packages, removing the risk for people to inadvertently take experimental dependencies. In profiling, this feature alone was attributed between 50% and 60% of the total time spent compiling `aws-cdk-lib`. The impact is likely comparable on other large construct libraries. The benefits from this feature are not sufficient to justify the cost to make it happen. Instead of trying to optimize it (which could prove impossible), decision was made to simply drop it.
The title of this Pull Request does not conform with [Conventional Commits] guidelines. It will need to be adjusted before the PR can be merged. |
@@ -761,7 +761,7 @@ | |||
{ | |||
"docs": { | |||
"stability": "stable", | |||
"summary": "(deprecated) String representation of the value." |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So many trees are going to be saved!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could be cool to calculate the negative carbon footprint of this change
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree but don't you try to nerd snipe me into sinking days or weeks into this. I'd first need to have representative telemetry for "cumulative compile times" + roughly what kind of hardware is in use (to estimate average power consumption of that build), and a ballpark idea of where that is geographically (to estimate the carbon intensity of the power supply there).
But I'd lie if your initial reaction to this did not make me think "oh jeez, it'd be sweet if the PRs had an automated comment telling you what the impact of the change is on the carbon footprint of the codebase"...
Thank you for contributing! ❤️ I will now look into making sure the PR is up-to-date, then proceed to try and merge it! |
Merging (with squash)... |
Merging (with squash)... |
The comments rewriter was used to prefix documentation comments with the
stability level of the associated API (e.g:
(experimental)
or(deprecated)
). This was introduced to palliate insufficient IDEsupport for
@deprecated
and the mixing of@stable
and@experimental
APIs in CDK v1.Since then, the TypeScript language server has added support for
@deprecated
IDE features, and CDK v2 moved all@experimental
APIs toseparate packages, removing the risk for people to inadvertently take
experimental dependencies.
In profiling, this feature alone was attributed between 50% and 60% of
the total time spent compiling
aws-cdk-lib
. The impact is likelycomparable on other large construct libraries.
The benefits from this feature are not sufficient to justify the cost to
make it happen. Instead of trying to optimize it (which could prove
impossible), decision was made to simply drop it.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.