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 1 commit
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
118 changes: 117 additions & 1 deletion 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,121 @@ 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
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
#### Dealing with 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;
rix0rrr marked this conversation as resolved.
Show resolved Hide resolved
}
}
```

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

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

The user must opt in to the new behavior, 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.
* 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 Expand Up @@ -806,7 +922,7 @@ step. If you are working on getting rid of example compilation errors of a
single package, you can run `yarn rosetta:extract --strict` in the package's
directory (see the [**jsii-rosetta**](#jsii-rosetta) section).

### Feature Flags
### feature flags
rix0rrr marked this conversation as resolved.
Show resolved Hide resolved

Sometimes we want to introduce new breaking behavior because we believe this is
the correct default behavior for the CDK. The problem of course is that breaking
Expand Down