diff --git a/text/287-cli-deprecation-warnings.md b/text/287-cli-deprecation-warnings.md new file mode 100644 index 000000000..57c868ed0 --- /dev/null +++ b/text/287-cli-deprecation-warnings.md @@ -0,0 +1,280 @@ +--- +rfc pr: [#290](https://github.com/aws/aws-cdk-rfcs/pull/290) +tracking issue: https://github.com/aws/aws-cdk-rfcs/issues/287 +--- + +# CDK CLI deprecation warnings + +If an element in the CDK Construct Library (class, interface, property, function) is deprecated, +using it inside a CDK application should produce a warning in the CDK CLI for all commands that perform synthesis +(`cdk synth`, `cdk diff`, `cdk deploy`, etc.). + +This is done in order to aid the migration from `V1` of the CDK to `V2` +(where all deprecated elements from `V1` will be removed). + +## Working Backwards + +### CHANGELOG + +feat(cli): warn about usages of deprecated Construct Library elements + +### README + +When using any element (class, interface, property, function) that is deprecated in the Construct Library, +you will receive a warning when performing any CLI operation that performs synthesis +(`cdk synth`, `cdk diff`, `cdk deploy`, etc.) that looks similar to: + +``` +[WARNING] The API @aws-cdk/core.CfnInclude is deprecated: + please use the CfnInclude class from the cloudformation-include module instead. + This API will be removed in the next major release +``` + +There is also an environment variable `DEPRECATED` that you can use to change the behavior of this feature: + +* Setting that environment variable to the value `quiet` will silence these warnings + (they will no longer be printed to the stderr stream). +* Setting that environment variable to the value `error` will instead fail with an exception when any deprecated element is used. +* Setting that environment variable to the value `warn` is the same as the default behavior + (the warnings will be printed to the stderr stream, + but will not cause errors). + +#### When exactly will this warning be shown? + +In general, this warning will be shown when executing any member +(by which I mean function (static or standalone), constructor, method, +or field (static or instance)) +from the AWS CDK Construct Library that is either: + +* Deprecated itself; +* Belongs to a class or interface that is deprecated; +* Has an argument that is of an interface type with a deprecated property, + and that property has been passed when calling the member. + +In particular, this means the warnings will not be printed for: + +* Any elements in your own code which are either explicitly deprecated, + or override any deprecated members from the Construct Library. +* Any code that invokes deprecated members, + but that is itself not actually invoked when running CDK synthesis. +* Using any deprecated members without actually invoking them + (for example, using a deprecated type only in a type declaration). + +### Contributing guide + +To deprecate an element (class, interface, property, function, etc.) in the CDK, +you need to add the `@deprecated` JSDoc tag to the documentation block of that element. +This will change the generated JavaScript code to make it emit a warning to the standard error stream whenever that element is invoked. + +## FAQ + +### What are we launching today? + +In the newest release of the AWS CDK Construct Library, +any usage of a deprecated element (class, interface, property, function) +in your CDK code will result in a warning being printed in the console output. + +### Does this mean my code will stop working after this change? + +No! This feature only adds warnings to the CLI, +it does not result in any code that was previously working to now fail. + +Note that if you want to migrate to `V2` of the CDK, +which we strongly recommend, +you will have to handle all the warnings that this feature emits, +as all deprecated elements in `V1` will be removed from `V2`, +and thus no longer available to be used by your code. + +If you want to make sure your code does not use any deprecated APIs, +and thus is ready for migrating to CDK `V2`, +you can set the `DEPRECATED` environment variable to the value `error`, +which will make any CDK command that invokes synthesis +(`cdk synth`, `cdk deploy`, `cdk diff`, etc.) +fail with an exception if any deprecated element is used. + +## Internal FAQ + +### Why are we doing this? + +We are doing this to help customers have a smoother migration from `V1` to `V2` of the CDK. + +### Why should we _not_ do this? + +I see four reasons why we might not want to implement this feature: + +1. Deprecated elements are already highlighted by virtually any editor/IDE. + In my experience, customers are diligent in moving away from deprecated APIs, + so this feature might not provide much value. +2. We risk overloading customers with warnings, + training them to ignore our output if there's too much of it. + A warning must be read in order to be effective, after all. +3. Depending on the exact solution chosen + (see discussion below), + we might not be able to distinguish from the user's code using a deprecated API, + and from our own libraries using deprecated APIs. + Which means a user might get warnings that they will be unable to get rid of + (because they're coming from code that they don't control). +4. Depending on the exact solution chosen + (see discussion below), + there might be edge cases where something is deprecated, + but we won't be able to report a particular pattern of its usage. + Some examples include: + * Using deprecated types only in type declarations. + * Static constants (which includes enums). + * Deprecated elements declared in the code, but not actually executed during synthesis. + +### What changes are required to enable this change? + +There are two high-level components of this change: + +#### 1. Changes to JSII + +We will add a feature to JSII that adds the warning code to the emitted JavaScript for deprecated elements. +It will be off by default, and will have to be activated explicitly to affect the emitted JavaScript code. + +#### 2. Using the changed JSII + +Once we have modified JSII and released a new version of it, +we will need to use it in the CDK, +and start compiling CDK with the new option turned on. + +We should also modify our CDK test infrastructure to run with the `DEPRECATED` +environment variable set to `error` by default, +to prevent our own code from using deprecated APIs which would cause warnings to be shown to users. +We should come up with a nice API that allows individual tests that are explicitly checking deprecated API(s) +as regression tests to make the value of that environment variable `quiet` easily. + +### Is this a breaking change? + +No. + +### What are the drawbacks of this solution? + +1. Requires changes to JSII. +2. Does not allow distinguishing between the customer using deprecated APIs, + and the library code using deprecated APIs. + We can alleviate this problem by making the value of the `DEPRECATED` environment variable `error` by default in our tests, + which will be a forcing function for us to stop using deprecated APIs in the Construct Library. +3. We only report APIs actually hit during a particular execution of the code + (while missing statically declared but not actually invoked deprecated elements - + note though that IDEs/editors still warn about them). + +### What alternative solutions did you consider? + +There are many alternatives that were considered, +but were, for various reasons, discarded: + +#### 1. Manual code for emitting the warnings + +Instead of modifying JSII, +we could simply write the code inside the TypeScript APIs "manually", +emitting warnings for any deprecated elements being used. + +This has the advantage of being the simplest solution, +but has been discarded because of the large effort, +and the fact that there's no way for us to verify that code has been added +(and added correctly). + +#### 2. TypeScript decorators + +We can use [TypeScript decorators](https://www.typescriptlang.org/docs/handbook/decorators.html) +to enhance the runtime JavaScript code with our custom logic. +We can add an `awslint` rule that enforces that every element with the `@deprecated` +JSDoc tag also needs to have the `@deprecated` decorator present. +While decorators are still considered an experimental feature, +our init templates and JSII enable them in `tsconfig.json`, +which means the vast majority of our customers will have them enabled as well. + +The main advantage of this solution is that changes are needed only in the CDK project. + +Disadvantages of this solution: + +1. It won't be possible to reliably get the module the deprecated element belongs to (only the name of the element) -- + the decorator is simply a JavaScript function defined in `@aws-cdk/core`, + and doesn't have (easy) access to the information on what module a given object it's called on belongs to. +2. TypeScript does not allow decorators on interfaces (neither on the interface directly, nor on any of its properties). + This means we will not be able to register warnings for structs, or struct properties. +3. We won't be able to register warnings for accessing deprecated static constants + (this includes enums). +4. We only report APIs actually hit during a particular execution of the code + (while missing statically declared but not actually invoked deprecated elements). + +#### 3. Rely on language-specific deprecation warning mechanisms + +JSII emits deprecated elements with the correct language-specific mechanisms in place +(for example, the `@Deprecated` annotation for Java). +We could simply piggyback on those, +instead of writing our own. + +Advantages of this solution: + +1. Minimal development effort on our side. +2. Compared to runtime analysis, allows listing of all deprecated API usages in the code base, + not the just ones that happen to be executed during a particular run. + +Disadvantages of this solution: + +1. TypeScript in VSCode (our most popular language/IDE combination) + does not by default show a complete list of deprecated API usages: + it will only show strikethroughs on identifiers you happen to be looking at. + Getting a complete list of deprecated APIs requires installing and configuring `eslint`. +2. TypeScript currently does not properly detect deprecation of properties in object literals, + and deprecation of properties (props) is very important for our project. + Even though it will properly detect them while autocompleting, + and there is an issue about it on the TypeScript bug tracker, + we cannot rely on them fixing this in a reasonable time frame. +3. There is nothing out of the box for this in Python + (we would have to change JSII and write our own decorator, + which would have the same disadvantages as the TypeScript decorators solution). + +#### 4. Parse the customer's code + +We could create a parser and type-checker for each language the CDK supports, +and use that to discover any cases of using deprecated elements. + +Advantages of this solution: + +1. Allows us to discover some tricky deprecated elements, + like enum values, or deprecated types used only in type declarations, + that will be difficult to do with only runtime reporting. +2. Would prevent reporting warnings for deprecated usages outside the customer's code + (for example, in our own libraries). +3. Compared to runtime analysis, allows listing of all deprecated API usages in the code base, + not the just ones that happen to be executed during a particular run. + +Disadvantages of this solution: + +1. We would have to write a separate parser and type-checker for each language supported by the CDK. +2. This additional parsing could have an adverse impact on the performance of CDK commands. +3. It's not obvious this analysis can even be performed at all in the case of dynamically-typed languages like JavaScript or Python. + +### What is the high level implementation plan? + +The high-level implementation plan is: + +1. Make the changes in JSII + (will probably be a single PR), + and release a new version once those are merged in. + Effort estimate (development + code review): 2 weeks. + +2. Set the new option during building CDK, + make sure the tests use the `DEPRECATED` environment variable set to `error`, + and provide an API for tests to opt-out of that on a case-by-case basis. + Effort estimate (development + code review): 1 week. + +### Are there any open issues that need to be addressed later? + +No. + +### Are there any things that need to be paid attention to during implementation? + +1. We need to check all arguments of all functions that have struct types, + and add code checking whether any of the deprecated struct properties have been passed. + This must include doing this recursively for nested structs as well. +2. It's very likely internal (starting with `_`) + API elements should be excluded from emitting these warnings. +3. It would be ideal if each API element only emitted the warning once , + when it was first invoked - as opposed to doing it every time it was invoked. +4. The message displayed in the console should include the deprecation message specified in the code. +5. If a class is deprecated, the warnings should most likely be added to every API element of that class, + whether that element is explicitly deprecated or not.