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

chore(contrib-guide): add guidance around breaking changes #13347

Merged
merged 7 commits into from
Mar 3, 2021
Merged
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
138 changes: 138 additions & 0 deletions CONTRIBUTING.md
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ and let us know if it's not up-to-date (even better, submit a PR with your corr
- [Step 4: Commit](#step-4-commit)
- [Step 5: Pull Request](#step-5-pull-request)
- [Step 6: Merge](#step-6-merge)
- [Breaking Changes](#breaking-changes)
- [Tools](#tools)
- [Main build scripts](#main-build-scripts)
- [Partial build tools](#partial-build-tools)
Expand Down Expand Up @@ -266,6 +267,143 @@ BREAKING CHANGE: Description of what broke and how to achieve this behavior now
* Once approved and tested, a maintainer will squash-merge to master and will use your PR title/description as the
commit message.

## Breaking Changes

Whenever you are making changes, there is a chance for those changes to be
*breaking* existing users of the library. A change is breaking if there are
programs that customers could have been writing against the current version
of the CDK, that will no longer "work correctly" with the proposed new
version of the CDK.

Breaking changes are not allowed in *stable* libraries¹. They are permissible
but still *highly discouraged* in experimental libraries, and require explicit
callouts in the bodies of Pull Requests that introduce them.

rix0rrr marked this conversation as resolved.
Show resolved Hide resolved
> ¹) Note that starting in version 2 of the CDK, the majority of library code will be
> bundled into a single main CDK library which will be considered stable, and so
> no code in there can undergo breaking changes.

Breaking changes come in two flavors:

* API surface changes
* Behavior changes

### API surface changes
rix0rrr marked this conversation as resolved.
Show resolved Hide resolved

This encompasses any changes that affect the shape of the API. Changes that
will make existing programs fail to compile are not allowed. Typical examples
of that are:

* Renaming classes or methods
* Adding required properties to a struct that is used as an input to a constructor
or method. This also includes changing a type from nullable to non-nullable.
* Removing properties from a struct that is returned from a method, or removing
properties from a class. This also includes changing a type from non-nullable
to nullable.

To see why the latter is a problem, consider the following class:

```ts
class SomeClass {
public readonly count: number;
// ❓ let's say I want to change this to 'count?: number',
// i.e. make it optional.
}

// Someone could have written the following code:
const obj = new SomeClass();
console.log(obj.count + 1);

// After the proposed change, this code that used to compile fine will now throw:
console.log(obj.count + 1);
// ~~~~~~~~~ Error: Object is possibly 'undefined'.
```

rix0rrr marked this conversation as resolved.
Show resolved Hide resolved
CDK comes with build tooling to check whether changes you made introduce breaking
changes to the API surface. In a package directory, run:

```shell
$ yarn build
$ yarn compat
```

To figure out if the changes you made were breaking. See the section [API Compatibility
Checks](#api-compatibility-checks) for more information.

#### Dealing with breaking API surface changes

If you need to change the type of some API element, introduce a new API
element and mark the old API element as `@deprecated`.

If you need to pretend to have a value for the purposes of implementing an API
and you don't actually have a useful value to return, it is acceptable to make
the property a `getter` and throw an exception (keeping in mind to write error
messages that will be useful to a user of your construct):

```ts
class SomeClass implements ICountable {
constructor(private readonly _count?: number) {
}

public get count(): number {
if (this._count === undefined) {
// ✅ DO: throw a descriptive error that tells the user what to do
throw new Error('This operation requires that a \'count\' is specified when SomeClass is created.');
// ❌ DO NOT: just throw an error like 'count is missing'
}
return this._count;
}
}
```

### Behavior changes

These are changes that do not directly affect the compilation of programs
written against the previous API, but may change their meaning. In practice,
even though the user didn't change their code, the CloudFormation template
that gets synthesized is now different.

**Not all template changes are breaking changes!** Consider a user that has
created a Stack using the previous version of the library, has updated their
version of the CDK library and is now deploying an update. A behavior change
is breaking if:

* The update cannot be applied at all
* The update can be applied but causes service interruption or data loss.

Data loss happens when the [Logical
ID](https://docs.aws.amazon.com/cdk/latest/guide/identifiers.html#identifiers_logical_ids)
of a stateful resource changes, or one of the [resource properties that requires
replacement](https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/using-cfn-updating-stacks-update-behaviors.html)
is modified. In both of these cases, CloudFormation will delete the
resource, and if it was a stateful resource like a database the data in it is now gone.

If a change applies cleanly and does not cause any service interruption, it
is not breaking. Nevertheless, it might still be wise to avoid those kinds of
changes as users are understandably wary of unexpected template changes, will
scrutinize them heavily, and we don't want to cause unnecessary panic and churn
in our use base.

Determining whether or not behavioral changes are breaking requires expertise
and judgement on the part of the library owner, and testing.

#### Dealing with breaking behavior changes

Most of the time, behavioral changes will arise because we want to change the
default value or default behavior of some property (i.e., we want to change the
interpretation of what it means if the value is missing).

If the new behavior is going to be breaking, the user must opt in to it, either by:

* Adding a new API element (class, property, method, ...) to have users
explicitly opt in to the new behavior at the source code level (potentially
`@deprecate`ing the old API element); or
* Use the [feature flag](#feature-flags) mechanism to have the user opt in to the new
behavior without changing the source code.

rix0rrr marked this conversation as resolved.
Show resolved Hide resolved
Of these two, the first one is preferred if possible (as feature flags have
non-local effects which can cause unintended effects).

## Tools

The CDK is a big project, and at the moment, all of the CDK modules are mastered in a single monolithic repository
Expand Down