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

RFC 287: CLI deprecated warnings #290

Merged
merged 10 commits into from
Mar 26, 2021
224 changes: 224 additions & 0 deletions text/287-cli-deprecation-warnings.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,224 @@
---
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
skinny85 marked this conversation as resolved.
Show resolved Hide resolved

### 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,
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we describe when this will not work? For example, what does it mean to "use" an interface in this context?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since that depends on the exact solution chosen, I've omitted it from this part.

Do we need to get into such details in the user-facing documentation?

Copy link
Contributor

Choose a reason for hiding this comment

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

This document should propose a solution and the "working backwards" section should be written as if this the proposed solution is the one we chose. If we end up changing the solution for whatever reason, we will need to evaluate the implications on the user experience and update this section (which is a good thing :-)).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK. Added. Let me know if this works.

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 are also two environment variables you can set to change the behavior of this feature:

* Setting the environment variable `CDK_DEPRECATED_IGNORE` will silence these warnings
skinny85 marked this conversation as resolved.
Show resolved Hide resolved
(they will no longer be printed to the console).

* Setting the environment variable `CDK_DEPRECATED_ERROR` will instead fail with an exception when any deprecated element is used.

### 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 `CDK_DEPRECATED_ERROR` environment variable,
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 three 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).

### 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 take effect.
Copy link
Contributor

Choose a reason for hiding this comment

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

What does that mean?

The default behavior of the application will be ignore, and the CLI will in turn flip that to warn by default unless ignored?

Motivation: unit tests etc I suppose?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It means the code adding the warnings will not be added to the emitted JavaScript by default at all.

We will also add two options to the JSII CLI that allow passing the names of the environment variables used for,
respectively, silencing the warnings, and turning the warnings into errors.

#### 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.
We will have to start compiling CDK with the new option turned on,
and also set the two additional options mentioned above to be
`CDK_DEPRECATED_IGNORE` and `CDK_DEPRECATED_ERROR`, respectively.
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure I follow this sentence. Can you re-state?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Re-worded. Let me know if this is clear now.


We should also modify our CDK test infrastructure to run with the `CDK_DEPRECATED_ERROR` option turned on by default,
unless a test is explicitly checking a deprecated API as a regression test,
in which case we should add an API for turning that option off for those tests.
Copy link
Contributor

Choose a reason for hiding this comment

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

add an API for turning that option off for those tests

Not following this. Add an API where? can you expand?

Copy link
Contributor

Choose a reason for hiding this comment

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

We should discuss this f2f.

I'm working on a similar area and have encountered unforeseen issues, and has generally been harder than I thought it will be. It's because of the different ways we deprecated elements.

Maybe you'll have the same problems, maybe not. Worth discussing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I meant add the API to our testing libraries. Similarly like you added an API for tests using feature flags.

Sure, I'd love to discuss this F2F.


### 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 turning on the `CDK_DEPRECATED_ERROR` environment variable by default in our tests,
which will be a forcing function for us to stop using deprecated APIs in the Construct Library.
3. We won't be able to register warnings for accessing deprecated static constants
Copy link
Contributor

Choose a reason for hiding this comment

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

Not actually true I don't think.

We can turn:

class Foo {
  /** @deprecated */
  public static readonly CONSTANT = 5;
}
// into
class Foo {
  public static get CONSTANT(): number {
    console.error('[WARNING] oh no');
    return 5;
  }
}

And something similar for enums.

Honestly, I feel like something like:

// Would work fine and would work for enums too
Foo.prototype.CONSTANT = deprecatedAccessor(Foo.prototype.CONSTANT, 'Use Foo.Bloop instead');

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fair enough. That's a huge intrusion to the generated code though. I've re-phrased the downside to reflect that.

(this includes enums).

skinny85 marked this conversation as resolved.
Show resolved Hide resolved
### 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).

#### 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.
skinny85 marked this conversation as resolved.
Show resolved Hide resolved

Disadvantages of this solution:

1. Just deprecated warnings in the IDE can be missed by developers
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't find this convincing, and I'm not convinced that printing to stderr is less likely to be missed.

We're probably talking about different contingents of programmers here, and printing to stderr is optimizing more for developers "like us".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I mean, we're showing the warnings in the IDE anyway - we're not taking this away. So if you don't agree they will be missed, then I guess you're in favor of a different option than the JSII one, no?

(especially as more APIs get deprecated).
skinny85 marked this conversation as resolved.
Show resolved Hide resolved
2. The experience with TypeScript properties is currently pretty bad
(the property is struck through when selecting it,
skinny85 marked this conversation as resolved.
Show resolved Hide resolved
but not after that!).
3. There is nothing out of the box for this in Python
(we would have to write our own decorator).

#### 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).

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?
skinny85 marked this conversation as resolved.
Show resolved Hide resolved

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.

2. Set the new option during building CDK,
make sure the tests use `CDK_DEPRECATED_ERROR`,
and provide an API for tests to opt-out of that on a case-by-case basis.
Copy link
Contributor

Choose a reason for hiding this comment

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

  1. Rewrite all code to stop using deprecated APIs (estimated effort: 2 months)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think it will take that long... see aws/aws-cdk#13500.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hah! :D


### Are there any open issues that need to be addressed later?

No.
skinny85 marked this conversation as resolved.
Show resolved Hide resolved