From 6aec3eb1b04a11c793c6d0ef620fc99b279ea7af Mon Sep 17 00:00:00 2001 From: Patrik Oldsberg Date: Fri, 14 Oct 2022 15:22:26 +0100 Subject: [PATCH 01/15] initial REVIEWING.md Signed-off-by: Patrik Oldsberg --- REVIEWING.md | 107 +++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 107 insertions(+) create mode 100644 REVIEWING.md diff --git a/REVIEWING.md b/REVIEWING.md new file mode 100644 index 0000000000..9b21b49d51 --- /dev/null +++ b/REVIEWING.md @@ -0,0 +1,107 @@ +# Introduction + +This file provides pointers for reviewing pull requests. While the main audience are reviewers, this can also be useful if you are contributing to the repository as well. + +## Code Style + +See [STYLE.md](./STYLE.md). + +## Secure Coding Practices + +Be sure to familiarize yourself with our [secure coding practices](./SECURITY.md#coding-practices). + +## Changesets + +We use changesets to track the changes in all published packages. Changesets both define what should go into the changelog of each package, but also what kind of version bump should be done for the next release. +An introduction to changesets can be found in our [contribution guidelines](./CONTRIBUTING.md#creating-changesets). + +When reviewing a changeset, the most important things to look for are the bump level, i.e. `major` / `minor` / `patch`, and whether the content is accurate and if it's written in a way that makes sense when reading it in the changelog for each package. + +### Reviewing Changeset Bump Levels + +### Reviewing Changeset Content + +Each changeset should be written in a way that describes the impact of the change for the end user of each package. The changesets end up in the changelog of each package, for example [@backstage/core-plugin-api](./packages/core-plugin-api/CHANGELOG.md). The changelogs are intended to provide both a summary of the new features as well as guidance in the case of breaking changes or deprecations. + +Some things that changeset should NOT contain are: + +- Internal architecture details - these are generally not interesting to end users, focus on the impact towards end users instead. +- Information related to a different package. +- A large amount of content, consider for example a separate migration guide instead, either in the package README or [./docs/](./docs/), and then link to that instead. +- Documentation - changesets can describe new features, but it should not be relied on for documenting them. Documentation should either be placed in [TSDoc](https://tsdoc.org) comments, package README, or [./docs/](./docs/). + +### When is a changeset needed? + +In general our changeset feedback bot will take care of informing whether a changeset is needed or not, but there are some edge cases. Whether a changeset is needed depends mostly on what files have been changed, but sometimes also + +Changes that do NOT need a new changeset: + +- Changes to any test, storybook, or other local development files, for example, `MyComponent.test.tsx`, `MyComponent.stories.tsx, `**mocks**/MyMock.ts`, `.eslintrc.js`, `setupTests.ts`, or `api-report.md`. Explained differently, it is only files that affect the published package that need changesets, such as source files and additional resources like `package.json`, `README.md`, `config.d.ts`, etc. +- When tweaking a change that has not yet been released, you can rely on and potentially modify the existing changeset. +- Changes that do not belong to a published packages, either because it's not a package at all, such as `docs/`, or because the package is private, such as `packages/app`. +- Changes that do not end up having an effect on the published package can be skipped, such as whitespace fixes or code formatting changes. It is also alright to include a short changeset for these kind of changes too. + +### Changeset Examples + +**Example 1** + +A new `EntityList` component has been added to `plugins/catalog-react`. + +#### GOOD + +```md +--- +'@backstage/plugin-catalog-react': minor +--- + +Added a new `EntityList` component that can be used to display detailed information about a list of entities. +``` + +The Catalog React library has reached version `1.x`, which means that feature additions that aren't breaking should be a `minor` change. We don't bother with too much documentation, keeping it short and sweet. The main purpose is to inform users that this new component exists and to give them an idea of how they can use it. + +#### BAD + +```md +--- +'@backstage/plugin-catalog-react': minor +'@backstage/plugin-catalog': minor +--- + +Added `EntityList` component. +Fixed a bug in the catalog index page. +``` + +This changeset is too short, it's best to give users an idea of how they can benefit from the new addition. + +It also includes changes affecting both the Catalog and Catalog React library. It should be split into two separate changeset for each of the two packages, otherwise we'll end up with redundant and unrelated information in both changelogs. + +```md +--- +'@backstage/plugin-catalog-react': major +--- + +Added a new `EntityList` component that can be used to display detailed information about a list of entities. The component looks like this: + +![EntityList screenshot](./screenshot.png) + +It accepts the following properties: + +- entities - The entities that should be listed. +- title - An optional formatting function for the list titles. +- dialog - An optional component that overrides the default details dialog. +``` + +This changeset is getting too detailed. It's not always bad to get this much into the weeds, but keep in mind that changesets are not easy to browse when search for information about specific APIs. It's better to document things like this separately and keep the changeset more lean. Also avoid linking to assets in changesets, keep them text-only. + +The change is also marked as a breaking `major` change. This should be changed to `minor` since adding new APIs is never a breaking change. + +## Review Checklist + +- [ ] API Reports + - [ ] Naming + - [ ] Breaking changes +- [ ] Changesets + - [ ] Content + - [ ] Bump level +- [ ] Have tests been added for new features bug fixes? +- [ ] Has documentation been added? From d383afb3722011879550febccb2595ffaad82149 Mon Sep 17 00:00:00 2001 From: Patrik Oldsberg Date: Tue, 18 Oct 2022 19:50:43 +0200 Subject: [PATCH 02/15] REVIEWING: add typescript section Signed-off-by: Patrik Oldsberg --- REVIEWING.md | 147 +++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 147 insertions(+) diff --git a/REVIEWING.md b/REVIEWING.md index 9b21b49d51..f0fdc3fbcd 100644 --- a/REVIEWING.md +++ b/REVIEWING.md @@ -105,3 +105,150 @@ The change is also marked as a breaking `major` change. This should be changed t - [ ] Bump level - [ ] Have tests been added for new features bug fixes? - [ ] Has documentation been added? + +## Breaking Changes + +Identifying breaking changes can be quite tricky. You need to look at the changes from both the point of view of consumers and producers of APIs, as well as behavioral changes. In this section we explore a couple of methods for identifying whether a change is breaking or not. + +### TypeScript + +Typescript is a huge help when it comes to identifying breaking changes, as well as the API Reports that we generate for all packages. Most of the time it is enough to only look at the API Reports to determine whether a change is breaking or not. + +In this section we will be talking about changed "types", but that refers to any kind of exported symbol from a packages, such as TypeScript types and interfaces, functions, classes, constants, etc. + +An important distinction to make when looking at changes to an API Report is the direction of a changed type, that is whether it's used as input or output from the user's point of view. In the next two sections we'll dive into the different directions of a type, and how it affects whether a change is breaking or not. + +#### Input Types + +A input type is one that users need to provide to the package by consumers. The most common form of input type are function, constructor and method parameters. + +The following is an example where `MyComponentProps` is an input type: + +```ts +type MyComponentProps = { + title: string; + size?: 'small' | 'medium' | 'large'; +}; + +function MyComponent(props: MyComponentProps): JSX.Element; +``` + +And from the consumer's point of view it would look something like this: + +```tsx + +``` + +When modifying an input type, any change that increases constraints are breaking. For example, if we made the `size` prop required, that would be a breaking change. Likewise, if we changed the type of `size` to `'small' | 'large'`, that would also be breaking. + +On the other hand, it's fine to relax constraints without it being a breaking change. For example, if we made the `title` prop optional, that would not be breaking. Likewise, if we changed the type of `size` to `'small' | 'medium' | 'large' | 'huge'`, that would not be breaking either. It is also possible to add new properties without it being a breaking change, as long as they are optional. + +There's an edge-case where completely removing a property is also considered a breaking change. That's because of TypeScript being strict and refusing unknown properties, rather than a runtime breaking change. It is typically and easy thing for consumers to fix though. + +Another way to think about the rules for evolving input types is that the old type must be assignable to the new type. In this case for example `_props: NewComponentProps = {} as OldComponentProps`. It's not a silver bullet though, because of edge-cases like the one mentioned above. + +#### Output Types + +An output type is one that the user receives from the packages. One of the most obvious examples here are the top-level exports from the package itself, but it also includes function return types. + +The following is an example where both `useSize` and `Size` are output types: + +```ts +type Box = { + title: string; + shape?: 'square' | 'rounded'; +}; + +function useBox(): Box; +``` + +And from the consumer's point of view it would look something like this: + +```ts +const { title, shape } = useBox(); +``` + +When modifying an output type, any change that reduces constraints are breaking. For example, if we made the `title` property optional, that would be a breaking change, or if we changed the type of `shape` to `'square' | 'rounded' | 'octagon'`. + +Adding new properties is not a breaking change, regardless of whether they are optional or not. Removing properties is on the other hand always breaking. + +It is generally fine to increase constraints without it being a breaking change. For example, if we made the `title` property required, that would not be breaking. + +There are some edge-cases though, for example if `shape` was changed to just `'square'`, that would be a breaking change because consumers might be checking for `box.shape === 'rounded'`, which would then be breaking. It's typically a quite easy thing for consumers to fix though. More generally, type unions and discriminated unions are quite troublesome in output types, as both adding and removing types from them are considered breaking changes. + +Another way to think about the rules for evolving output types is that the new type must be assignable to the old type. In this case for example `_box: OldBox = {} as NewBox`. It's not a silver bullet though, because of edge-cases like the one mentioned above. + +#### I/O Types + +Some types are considered both input and output types. For example, consider the following example: + +```ts +type Point = { + x: number; + y: number; +}; + +function trimCoords(point: Point): Point; +``` + +In this case `Point` is both an input and output type. This means that the only changes we can make to the type that aren't breaking are the intersection of allowed changes between input and output types. In practice this only allows for the addition of new optional properties. Because of this constraint it is generally best to avoid using I/O types, and keep the input separated from the output. + +There are some cases where I/O types favor either input or output when it comes to API stability. For example, all types used by Utility APIs are I/O types, but the stability of the output is a lot more important than the stability of the input. That is because it's a lot easier for the single producer of the input interface to adapt to changes compared to all consumers of the API that use it as an output type. + +#### Identifying the Direction + +The only way to identify the direction of a type is to look at the context in which it's being used. In particular this can be tricky when looking at individual type aliases and interfaces, as you need to look at the rest of the package exports to see how the type is being used. + +One important rule is that the context considered for any type is limited to only the package in which the type is declared. Just because a type is imported in a different package and used as an input type does not make it an input type. + +The following rules can be used to identify the direction of a type alias or interface: + +- If the type is used in an input context, for example function parameter, then it's an input type. +- If the type is used in an output context, for example function return type, then it's an output type. +- If the type is referenced by another type, then it inherits the direction of that type, except if referenced through a function callback, in which case the direction is reversed. +- If the type is used or inherits both input and output context, then it's an I/O type. +- If the type is not referenced anywhere else, then it's an I/O type. + +Below is an example of the public API of a package, with type directions assigned to each export: + +```ts +// I/O, used by getPoint as return type and referenced by BoxProps, an input type +interface Point { + x: number; + y: number; +} + +function getPoint(): Point; + +// Input, used by Box as parameter type +interface BoxProps { + point?: Point +} + +function Box(props: BoxProps): JSX.Element; + +// Output, used by createWidget as return type +interface Widget { + ... +} + +// Output, as it's referenced by WidgetOptions, which is an input +// type, but the render callback causes a direction reversal +interface WidgetProps { + ... +} + +// Input, just like WidgetProps this is due to the direction reversal +// caused by the render callback +type RenderedWidget = JSX.Element | null; + +// Input, used by createWidget parameter type +interface WidgetOptions { + render(props: WidgetProps): RenderedWidget; +} + +function createWidget(options: WidgetOptions): Widget; + +// I/O, since it's not referenced anywhere else +type LabelStyle = 'normal' | 'thin'; +``` From b3d522b0a8db57f205d7b0d6a7250f8f5b16454c Mon Sep 17 00:00:00 2001 From: Patrik Oldsberg Date: Wed, 19 Oct 2022 10:42:40 +0200 Subject: [PATCH 03/15] REVIEWING: example tweaks talk about behavioral changes Signed-off-by: Patrik Oldsberg --- REVIEWING.md | 49 ++++++++++++++++++++++++++++++------------------- 1 file changed, 30 insertions(+), 19 deletions(-) diff --git a/REVIEWING.md b/REVIEWING.md index f0fdc3fbcd..074f568638 100644 --- a/REVIEWING.md +++ b/REVIEWING.md @@ -4,7 +4,7 @@ This file provides pointers for reviewing pull requests. While the main audience ## Code Style -See [STYLE.md](./STYLE.md). +See [STYLE.md](./STYLE.md). In particular, make sure that naming follows established conventions within the project and/or package. ## Secure Coding Practices @@ -41,13 +41,15 @@ Changes that do NOT need a new changeset: - Changes that do not belong to a published packages, either because it's not a package at all, such as `docs/`, or because the package is private, such as `packages/app`. - Changes that do not end up having an effect on the published package can be skipped, such as whitespace fixes or code formatting changes. It is also alright to include a short changeset for these kind of changes too. -### Changeset Examples +### Changeset Example -**Example 1** +Consider the following scenario for a changeset: A new `EntityList` component has been added to `plugins/catalog-react`. -#### GOOD +Below are examples of a good and two bad changesets for that change. + +**GOOD** ```md --- @@ -57,14 +59,14 @@ A new `EntityList` component has been added to `plugins/catalog-react`. Added a new `EntityList` component that can be used to display detailed information about a list of entities. ``` -The Catalog React library has reached version `1.x`, which means that feature additions that aren't breaking should be a `minor` change. We don't bother with too much documentation, keeping it short and sweet. The main purpose is to inform users that this new component exists and to give them an idea of how they can use it. +The `@backstage/plugin-catalog-react` package has reached version `1.x`, which means that feature additions that aren't breaking should be a `minor` change. We don't bother with too much documentation, keeping it short and sweet. The main purpose is to inform users that this new component exists and to give them an idea of how they can use it. -#### BAD +**BAD** ```md --- '@backstage/plugin-catalog-react': minor -'@backstage/plugin-catalog': minor +'@backstage/plugin-catalog': patch --- Added `EntityList` component. @@ -75,6 +77,10 @@ This changeset is too short, it's best to give users an idea of how they can ben It also includes changes affecting both the Catalog and Catalog React library. It should be split into two separate changeset for each of the two packages, otherwise we'll end up with redundant and unrelated information in both changelogs. +Lastly, the `@backstage/plugin-catalog-react` package has reached `1.x`, which means that new features should be introduced through a `minor` bump. We'd only use `patch` bumps for minor changes or fixes that do not affect the public API. + +**BAD** + ```md --- '@backstage/plugin-catalog-react': major @@ -95,22 +101,21 @@ This changeset is getting too detailed. It's not always bad to get this much int The change is also marked as a breaking `major` change. This should be changed to `minor` since adding new APIs is never a breaking change. -## Review Checklist - -- [ ] API Reports - - [ ] Naming - - [ ] Breaking changes -- [ ] Changesets - - [ ] Content - - [ ] Bump level -- [ ] Have tests been added for new features bug fixes? -- [ ] Has documentation been added? - ## Breaking Changes Identifying breaking changes can be quite tricky. You need to look at the changes from both the point of view of consumers and producers of APIs, as well as behavioral changes. In this section we explore a couple of methods for identifying whether a change is breaking or not. -### TypeScript +### Behavioral Changes + +These are changes where the behavior of the code changes, but the public API remains the same. They can be anything from tiny tweaks, like adding a bit of padding to a visual element, to a complete redesign and refactor of an entire plugin. + +It's hard to set up exact rules for when a behavioral change is breaking or not. In some cases it's obvious, for example if you remove important functionality of a system, while in other cases it can be very hard to tell. In the end what's important is whether a significant number of users of the package will be negatively impacted by the change. One question that you can ask yourself here is "is it likely that there are users that don't want the new behavior, or will need to change their code to adapt to the new behavior?" If the answer is yes, then it's likely a breaking change. You do also want to keep [xkcd.com/1172](https://xkcd.com/1172/) in mind though. + +Note that even a bug fix can be considered a breaking change in some situations. One things to lean on in that case is what the _documented_ behavior is. If the current behavior does not match the documented behavior, then a change to match the documentation is generally not a breaking change. That is unless it is likely that there are a significant number of users that will be impacted by the change. + +For tricky behavioral changes you may simply need to let end users provide feedback. This can be done either by hiding the new behavior behind an experimental feature switch, or by releasing the change early on in the release cycle, preferably in the first or second next line release. Be ready to respond to feedback and potentially revert the change if needed. + +### Public API Changes Typescript is a huge help when it comes to identifying breaking changes, as well as the API Reports that we generate for all packages. Most of the time it is enough to only look at the API Reports to determine whether a change is breaking or not. @@ -218,6 +223,7 @@ interface Point { y: number; } +// Output, since it's an exported function function getPoint(): Point; // Input, used by Box as parameter type @@ -225,6 +231,7 @@ interface BoxProps { point?: Point } +// Output, since it's an exported function function Box(props: BoxProps): JSX.Element; // Output, used by createWidget as return type @@ -247,8 +254,12 @@ interface WidgetOptions { render(props: WidgetProps): RenderedWidget; } +// Output, since it's an exported function function createWidget(options: WidgetOptions): Widget; // I/O, since it's not referenced anywhere else type LabelStyle = 'normal' | 'thin'; + +// Output, since it's an exported constant +const LABEL_SIZE: number; ``` From 8a4f4d1f307ebdf8fa216283bd80be9bff771358 Mon Sep 17 00:00:00 2001 From: Patrik Oldsberg Date: Wed, 19 Oct 2022 13:19:00 +0200 Subject: [PATCH 04/15] REVIEWING: versioning policy and changeset bump sections Signed-off-by: Patrik Oldsberg --- REVIEWING.md | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/REVIEWING.md b/REVIEWING.md index 074f568638..d118eee367 100644 --- a/REVIEWING.md +++ b/REVIEWING.md @@ -10,6 +10,12 @@ See [STYLE.md](./STYLE.md). In particular, make sure that naming follows establi Be sure to familiarize yourself with our [secure coding practices](./SECURITY.md#coding-practices). +## Release & Versioning Policy + +When reviewing pull requests it's important to consider our [versioning policy and release cycle](https://backstage.io/docs/overview/versioning-policy). Generally the most important bit is our [package versioning policy](https://backstage.io/docs/overview/versioning-policy#package-versioning-policy), which describes when and how we can ship breaking changes. We'll dive into how to identify breaking changes in a later section section. + +One other thing to keep in mind, especially when merging pull requests, is where in the release cycle we're currently at. In particular you want to avoid merging any large or risky changes towards the end of each release cycle. If there is a change that is ready to be merged, but you want to hold off until the next main line release, then you can label it with the `merge-after-release` label. + ## Changesets We use changesets to track the changes in all published packages. Changesets both define what should go into the changelog of each package, but also what kind of version bump should be done for the next release. @@ -19,6 +25,19 @@ When reviewing a changeset, the most important things to look for are the bump l ### Reviewing Changeset Bump Levels +The following table provides a reference for what type of version bump is needed for each individual package. This applies to each individual package separately, it does not matter what the scope of a change is in any other broader scope. + +| Scope | Current Package Version | Bump Level | +| --------------- | ----------------------- | ---------- | +| Breaking Change | `1.0` and above | `major` | +| New Feature | `1.0` and above | `minor` | +| Fix | `1.0` and above | `patch` | +| Breaking Change | `0.x` | `minor` | +| New Feature | `0.x` | `patch` | +| Fix | `0.x` | `patch` | + +The only situation where a package that is currently at `0.x` can have a `major` bump is if all owners and stakeholders of the package agree that the package is ready to be released as `1.0`. + ### Reviewing Changeset Content Each changeset should be written in a way that describes the impact of the change for the end user of each package. The changesets end up in the changelog of each package, for example [@backstage/core-plugin-api](./packages/core-plugin-api/CHANGELOG.md). The changelogs are intended to provide both a summary of the new features as well as guidance in the case of breaking changes or deprecations. From 78577cb46e673a23eaf5ae87c8e0f1b6d60b55ad Mon Sep 17 00:00:00 2001 From: Patrik Oldsberg Date: Wed, 19 Oct 2022 13:22:41 +0200 Subject: [PATCH 05/15] REVIEWING: expand code style section Signed-off-by: Patrik Oldsberg --- REVIEWING.md | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/REVIEWING.md b/REVIEWING.md index d118eee367..32c2a797ae 100644 --- a/REVIEWING.md +++ b/REVIEWING.md @@ -4,7 +4,11 @@ This file provides pointers for reviewing pull requests. While the main audience ## Code Style -See [STYLE.md](./STYLE.md). In particular, make sure that naming follows established conventions within the project and/or package. +See our code style documented at [STYLE.md](./STYLE.md). + +In particular when it comes to naming, make sure that naming follows established conventions within the project and/or package. + +When adding new dependencies to packages it is always preferred to use version ranges that are already in use by other packages in the repository. This helps minimize lockfile changes and reduce package duplication, both in our repository as well as other Backstage installations. ## Secure Coding Practices From 1156ed81398abd69820d01c345d71bfbeb855828 Mon Sep 17 00:00:00 2001 From: Patrik Oldsberg Date: Wed, 19 Oct 2022 14:22:46 +0200 Subject: [PATCH 06/15] REVIEWING: couple more sections and tweaks Signed-off-by: Patrik Oldsberg --- REVIEWING.md | 83 +++++++++++++++++++++++++++++++++++----------------- 1 file changed, 56 insertions(+), 27 deletions(-) diff --git a/REVIEWING.md b/REVIEWING.md index 32c2a797ae..6a54ae875e 100644 --- a/REVIEWING.md +++ b/REVIEWING.md @@ -1,6 +1,6 @@ # Introduction -This file provides pointers for reviewing pull requests. While the main audience are reviewers, this can also be useful if you are contributing to the repository as well. +This file provides pointers for reviewing pull requests. While the main audience are reviewers, this can also be useful if you are contributing to this repository. ## Code Style @@ -16,20 +16,20 @@ Be sure to familiarize yourself with our [secure coding practices](./SECURITY.md ## Release & Versioning Policy -When reviewing pull requests it's important to consider our [versioning policy and release cycle](https://backstage.io/docs/overview/versioning-policy). Generally the most important bit is our [package versioning policy](https://backstage.io/docs/overview/versioning-policy#package-versioning-policy), which describes when and how we can ship breaking changes. We'll dive into how to identify breaking changes in a later section section. +When reviewing pull requests it's important to consider our [versioning policy and release cycle](https://backstage.io/docs/overview/versioning-policy). Generally the most important bit is our [package versioning policy](https://backstage.io/docs/overview/versioning-policy#package-versioning-policy), which describes when and how we can ship breaking changes. We'll dive into how to identify breaking changes in a different section. One other thing to keep in mind, especially when merging pull requests, is where in the release cycle we're currently at. In particular you want to avoid merging any large or risky changes towards the end of each release cycle. If there is a change that is ready to be merged, but you want to hold off until the next main line release, then you can label it with the `merge-after-release` label. ## Changesets -We use changesets to track the changes in all published packages. Changesets both define what should go into the changelog of each package, but also what kind of version bump should be done for the next release. +We use changesets to track changes in all published packages. Changesets both define what should go into the changelog of each package, but also what kind of version bump should be done for the next release. An introduction to changesets can be found in our [contribution guidelines](./CONTRIBUTING.md#creating-changesets). -When reviewing a changeset, the most important things to look for are the bump level, i.e. `major` / `minor` / `patch`, and whether the content is accurate and if it's written in a way that makes sense when reading it in the changelog for each package. +When reviewing a changeset, the most important things to look for are the bump levels, i.e. `major` / `minor` / `patch`, as well as whether the content is accurate and if it's written in a way that makes sense when reading it in the changelog for each package. ### Reviewing Changeset Bump Levels -The following table provides a reference for what type of version bump is needed for each individual package. This applies to each individual package separately, it does not matter what the scope of a change is in any other broader scope. +The following table provides a reference for what type of version bump is needed for each package. This is applied separately to each individual package, it does not matter what the scope of a change is in any other broader context. | Scope | Current Package Version | Bump Level | | --------------- | ----------------------- | ---------- | @@ -44,25 +44,25 @@ The only situation where a package that is currently at `0.x` can have a `major` ### Reviewing Changeset Content -Each changeset should be written in a way that describes the impact of the change for the end user of each package. The changesets end up in the changelog of each package, for example [@backstage/core-plugin-api](./packages/core-plugin-api/CHANGELOG.md). The changelogs are intended to provide both a summary of the new features as well as guidance in the case of breaking changes or deprecations. +Each changeset should be written in a way that describes the impact of the change for the users of each package. The contents of the changesets will end up in the changelog of each package, for example [@backstage/core-plugin-api](./packages/core-plugin-api/CHANGELOG.md). The changelogs are intended to provide both a summary of the new features as well as guidance in the case of breaking changes or deprecations. Some things that changeset should NOT contain are: -- Internal architecture details - these are generally not interesting to end users, focus on the impact towards end users instead. +- Internal architecture details - these are generally not interesting to users, focus on the impact towards users of the package instead. - Information related to a different package. - A large amount of content, consider for example a separate migration guide instead, either in the package README or [./docs/](./docs/), and then link to that instead. - Documentation - changesets can describe new features, but it should not be relied on for documenting them. Documentation should either be placed in [TSDoc](https://tsdoc.org) comments, package README, or [./docs/](./docs/). ### When is a changeset needed? -In general our changeset feedback bot will take care of informing whether a changeset is needed or not, but there are some edge cases. Whether a changeset is needed depends mostly on what files have been changed, but sometimes also +In general our changeset feedback bot will take care of informing whether a changeset is needed or not, but there are some edge cases. Whether a changeset is needed depends mostly on what files have been changed, but sometimes also on the kind of change that has been made. Changes that do NOT need a new changeset: - Changes to any test, storybook, or other local development files, for example, `MyComponent.test.tsx`, `MyComponent.stories.tsx, `**mocks**/MyMock.ts`, `.eslintrc.js`, `setupTests.ts`, or `api-report.md`. Explained differently, it is only files that affect the published package that need changesets, such as source files and additional resources like `package.json`, `README.md`, `config.d.ts`, etc. -- When tweaking a change that has not yet been released, you can rely on and potentially modify the existing changeset. +- When tweaking a change that has not yet been released, you can rely on and potentially modify the existing changeset instead. - Changes that do not belong to a published packages, either because it's not a package at all, such as `docs/`, or because the package is private, such as `packages/app`. -- Changes that do not end up having an effect on the published package can be skipped, such as whitespace fixes or code formatting changes. It is also alright to include a short changeset for these kind of changes too. +- Changes that do not end up having an effect on the published package, such as whitespace fixes or code formatting changes. Although it's also fine to have a short changeset for these kind of changes too. ### Changeset Example @@ -70,7 +70,7 @@ Consider the following scenario for a changeset: A new `EntityList` component has been added to `plugins/catalog-react`. -Below are examples of a good and two bad changesets for that change. +Below are examples of a good and three bad changesets for that change. **GOOD** @@ -89,18 +89,17 @@ The `@backstage/plugin-catalog-react` package has reached version `1.x`, which m ```md --- '@backstage/plugin-catalog-react': minor -'@backstage/plugin-catalog': patch +'@backstage/plugin-catalog': minor --- Added `EntityList` component. + Fixed a bug in the catalog index page. ``` This changeset is too short, it's best to give users an idea of how they can benefit from the new addition. -It also includes changes affecting both the Catalog and Catalog React library. It should be split into two separate changeset for each of the two packages, otherwise we'll end up with redundant and unrelated information in both changelogs. - -Lastly, the `@backstage/plugin-catalog-react` package has reached `1.x`, which means that new features should be introduced through a `minor` bump. We'd only use `patch` bumps for minor changes or fixes that do not affect the public API. +It also includes changes affecting both the Catalog and Catalog React library. It should be split into two separate changesets for each of the two packages, otherwise we'll end up with redundant and unrelated information in both changelogs. **BAD** @@ -120,17 +119,31 @@ It accepts the following properties: - dialog - An optional component that overrides the default details dialog. ``` -This changeset is getting too detailed. It's not always bad to get this much into the weeds, but keep in mind that changesets are not easy to browse when search for information about specific APIs. It's better to document things like this separately and keep the changeset more lean. Also avoid linking to assets in changesets, keep them text-only. +This changeset is getting too detailed. It's not always bad to get this much into the weeds, but keep in mind that changesets are not easy to browse when searching for information about specific APIs. It's better to document things like this separately and keep the changeset more lean. Also avoid linking to assets in changesets, keep them text-only. The change is also marked as a breaking `major` change. This should be changed to `minor` since adding new APIs is never a breaking change. +**BAD** + +```md +--- +'@backstage/plugin-catalog-react': patch +--- + +Added a new `EntityList` component that can be used to display detailed information about a list of entities. The `ListView` component was also refactored in order to make it possible to reuse it between the new `EntityList` and `KindList` components. +``` + +Assuming that the `ListView` component is not public API, this changeset goes into details that are not interesting to the user of the package. Internal changes do not need to be highlighted in changesets. If an internal refactor is the only change then it's alright to say something short like "Internal refactor to improve code reuse", but otherwise those details should be left out. + +The `@backstage/plugin-catalog-react` package has also reached `1.x`, which means that new features should be introduced through a `minor` bump. We'd only use `patch` bumps for minor changes or fixes that do not affect the public API. + ## Breaking Changes Identifying breaking changes can be quite tricky. You need to look at the changes from both the point of view of consumers and producers of APIs, as well as behavioral changes. In this section we explore a couple of methods for identifying whether a change is breaking or not. ### Behavioral Changes -These are changes where the behavior of the code changes, but the public API remains the same. They can be anything from tiny tweaks, like adding a bit of padding to a visual element, to a complete redesign and refactor of an entire plugin. +These are changes where the behavior of the code changes, but the public API is unchanged or doesn't have any breaking changes. They can be anything from tiny tweaks, like adding a bit of padding to a visual element, to a complete redesign and refactor of an entire plugin. It's hard to set up exact rules for when a behavioral change is breaking or not. In some cases it's obvious, for example if you remove important functionality of a system, while in other cases it can be very hard to tell. In the end what's important is whether a significant number of users of the package will be negatively impacted by the change. One question that you can ask yourself here is "is it likely that there are users that don't want the new behavior, or will need to change their code to adapt to the new behavior?" If the answer is yes, then it's likely a breaking change. You do also want to keep [xkcd.com/1172](https://xkcd.com/1172/) in mind though. @@ -140,15 +153,31 @@ For tricky behavioral changes you may simply need to let end users provide feedb ### Public API Changes -Typescript is a huge help when it comes to identifying breaking changes, as well as the API Reports that we generate for all packages. Most of the time it is enough to only look at the API Reports to determine whether a change is breaking or not. +Typescript is a huge help when it comes to identifying breaking changes, as well as the API Reports that we generate for all packages. Most of the time it is enough to only look at the API Reports to determine whether a change is breaking or not. If you determine that a change is breaking at the TypeScript level, then it is a breaking change. -In this section we will be talking about changed "types", but that refers to any kind of exported symbol from a packages, such as TypeScript types and interfaces, functions, classes, constants, etc. +In this section we will be talking about changed "types", but by that we mean any kind of exported symbol from a packages, such as TypeScript types aliases or interfaces, functions, classes, constants, etc. -An important distinction to make when looking at changes to an API Report is the direction of a changed type, that is whether it's used as input or output from the user's point of view. In the next two sections we'll dive into the different directions of a type, and how it affects whether a change is breaking or not. +#### API Reports + +We generate API Reports using the [API Extractor](https://api-extractor.com/) tool. These reports are generated for most packages in the Backstage repository, and are stored in the `api-report.md` file of each package. For CLI package we use custom tooling, and instead store the result in `cli-report.md`. Whenever the public API of a package changes, the API Report needs to be updated to reflect the new state of the API. Our CI checks will fail if the API reports are not up to date in a pull request. + +Each API report contains a list of all the exported types of each package. As long as the API report does not have any warnings it will contain the full publicly facing API of the package, meaning you do not need to consider any other changes to the package from the point of view of TypeScript API stability. + +Exported types can be marked with either `@public`, `@alpha` or `@beta` release tags. It is only the `@public` exports that we consider to be part of the stable API. The `@alpha` and `@beta` exports are considered unstable and can be changed at any time without needing a breaking package versions bump. However, this **ONLY** applies if the package has been configured to use experimental type builds, which looks like this in `package.json`: + +```json + "build": "backstage-cli package build --experimental-type-build" +``` + +If a package does not have this configuration, then all exported types are considered stable, even if they are marked as `@alpha` or `@beta`. + +#### Type Contract Direction + +An important distinction to make when looking at changes to an API Report is the direction of the contract of a changed type, that is, whether it's used as input or output from the user's point of view. In the next two sections we'll dive into the different directions of a type contract, and how it affects whether a change is breaking or not. #### Input Types -A input type is one that users need to provide to the package by consumers. The most common form of input type are function, constructor and method parameters. +A input type is one where a value needs to be provided by users of the package. The most common form of input type are function, constructor, and method parameters. The following is an example where `MyComponentProps` is an input type: @@ -161,7 +190,7 @@ type MyComponentProps = { function MyComponent(props: MyComponentProps): JSX.Element; ``` -And from the consumer's point of view it would look something like this: +And from the package user's point of view it would look something like this: ```tsx @@ -177,9 +206,9 @@ Another way to think about the rules for evolving input types is that the old ty #### Output Types -An output type is one that the user receives from the packages. One of the most obvious examples here are the top-level exports from the package itself, but it also includes function return types. +An output type is one that the user receives from the packages. One of the most obvious examples here are the top-level exports from the package itself, but it also includes for example function return types. -The following is an example where both `useSize` and `Size` are output types: +The following is an example where both `useBox` and `Box` are output types: ```ts type Box = { @@ -223,9 +252,9 @@ In this case `Point` is both an input and output type. This means that the only There are some cases where I/O types favor either input or output when it comes to API stability. For example, all types used by Utility APIs are I/O types, but the stability of the output is a lot more important than the stability of the input. That is because it's a lot easier for the single producer of the input interface to adapt to changes compared to all consumers of the API that use it as an output type. -#### Identifying the Direction +#### Identifying the Contract Direction -The only way to identify the direction of a type is to look at the context in which it's being used. In particular this can be tricky when looking at individual type aliases and interfaces, as you need to look at the rest of the package exports to see how the type is being used. +The only way to identify the contract direction of a type is to look at the context in which it's being used. In particular this can be tricky when looking at individual type aliases and interfaces, as you need to look at the rest of the package exports to see how the type is being used. One important rule is that the context considered for any type is limited to only the package in which the type is declared. Just because a type is imported in a different package and used as an input type does not make it an input type. @@ -234,7 +263,7 @@ The following rules can be used to identify the direction of a type alias or int - If the type is used in an input context, for example function parameter, then it's an input type. - If the type is used in an output context, for example function return type, then it's an output type. - If the type is referenced by another type, then it inherits the direction of that type, except if referenced through a function callback, in which case the direction is reversed. -- If the type is used or inherits both input and output context, then it's an I/O type. +- If the type is used or inherits both input and output contexts, then it's an I/O type. - If the type is not referenced anywhere else, then it's an I/O type. Below is an example of the public API of a package, with type directions assigned to each export: From c0098f1bdd968f518006603cf42d32325fd74441 Mon Sep 17 00:00:00 2001 From: Patrik Oldsberg Date: Wed, 19 Oct 2022 15:20:26 +0200 Subject: [PATCH 07/15] add TSDoc to vocab Signed-off-by: Patrik Oldsberg --- .github/vale/Vocab/Backstage/accept.txt | 1 + 1 file changed, 1 insertion(+) diff --git a/.github/vale/Vocab/Backstage/accept.txt b/.github/vale/Vocab/Backstage/accept.txt index e40dfffda1..19205d0c8d 100644 --- a/.github/vale/Vocab/Backstage/accept.txt +++ b/.github/vale/Vocab/Backstage/accept.txt @@ -341,6 +341,7 @@ transpiled transpiler transpilers truthy +TSDoc typeahead ui unbreak From e0aa20ab120fcfbcd6cff50e4ef798586f639efa Mon Sep 17 00:00:00 2001 From: Patrik Oldsberg Date: Wed, 19 Oct 2022 16:21:12 +0200 Subject: [PATCH 08/15] REVIEWING: use valid png link Signed-off-by: Patrik Oldsberg --- REVIEWING.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/REVIEWING.md b/REVIEWING.md index 6a54ae875e..29bdcc3877 100644 --- a/REVIEWING.md +++ b/REVIEWING.md @@ -110,7 +110,7 @@ It also includes changes affecting both the Catalog and Catalog React library. I Added a new `EntityList` component that can be used to display detailed information about a list of entities. The component looks like this: -![EntityList screenshot](./screenshot.png) +![EntityList screenshot](./docs/assets/headline.png) It accepts the following properties: From c20e2b2f33f6571e92bca42d5e72e303c0afbf0b Mon Sep 17 00:00:00 2001 From: Patrik Oldsberg Date: Wed, 19 Oct 2022 16:26:07 +0200 Subject: [PATCH 09/15] Update REVIEWING.md Co-authored-by: Phil Kuang Signed-off-by: Patrik Oldsberg --- REVIEWING.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/REVIEWING.md b/REVIEWING.md index 29bdcc3877..1e34938a0f 100644 --- a/REVIEWING.md +++ b/REVIEWING.md @@ -200,7 +200,7 @@ When modifying an input type, any change that increases constraints are breaking On the other hand, it's fine to relax constraints without it being a breaking change. For example, if we made the `title` prop optional, that would not be breaking. Likewise, if we changed the type of `size` to `'small' | 'medium' | 'large' | 'huge'`, that would not be breaking either. It is also possible to add new properties without it being a breaking change, as long as they are optional. -There's an edge-case where completely removing a property is also considered a breaking change. That's because of TypeScript being strict and refusing unknown properties, rather than a runtime breaking change. It is typically and easy thing for consumers to fix though. +There's an edge-case where completely removing a property is also considered a breaking change. That's because of TypeScript being strict and refusing unknown properties, rather than a runtime breaking change. It is typically an easy thing for consumers to fix though. Another way to think about the rules for evolving input types is that the old type must be assignable to the new type. In this case for example `_props: NewComponentProps = {} as OldComponentProps`. It's not a silver bullet though, because of edge-cases like the one mentioned above. From b573005f62f5f10220b3e5a841443906b7d0306c Mon Sep 17 00:00:00 2001 From: Patrik Oldsberg Date: Wed, 19 Oct 2022 16:26:35 +0200 Subject: [PATCH 10/15] Update REVIEWING.md Co-authored-by: Phil Kuang Signed-off-by: Patrik Oldsberg --- REVIEWING.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/REVIEWING.md b/REVIEWING.md index 1e34938a0f..38bae08423 100644 --- a/REVIEWING.md +++ b/REVIEWING.md @@ -155,7 +155,7 @@ For tricky behavioral changes you may simply need to let end users provide feedb Typescript is a huge help when it comes to identifying breaking changes, as well as the API Reports that we generate for all packages. Most of the time it is enough to only look at the API Reports to determine whether a change is breaking or not. If you determine that a change is breaking at the TypeScript level, then it is a breaking change. -In this section we will be talking about changed "types", but by that we mean any kind of exported symbol from a packages, such as TypeScript types aliases or interfaces, functions, classes, constants, etc. +In this section we will be talking about changed "types", but by that we mean any kind of exported symbol from packages, such as TypeScript types aliases or interfaces, functions, classes, constants, etc. #### API Reports From 1fc2b3efe2136bab76a10d04c46c621cd97daff9 Mon Sep 17 00:00:00 2001 From: Patrik Oldsberg Date: Wed, 19 Oct 2022 16:30:03 +0200 Subject: [PATCH 11/15] Update REVIEWING.md Co-authored-by: Phil Kuang Signed-off-by: Patrik Oldsberg --- REVIEWING.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/REVIEWING.md b/REVIEWING.md index 38bae08423..9d88c36b36 100644 --- a/REVIEWING.md +++ b/REVIEWING.md @@ -59,7 +59,7 @@ In general our changeset feedback bot will take care of informing whether a chan Changes that do NOT need a new changeset: -- Changes to any test, storybook, or other local development files, for example, `MyComponent.test.tsx`, `MyComponent.stories.tsx, `**mocks**/MyMock.ts`, `.eslintrc.js`, `setupTests.ts`, or `api-report.md`. Explained differently, it is only files that affect the published package that need changesets, such as source files and additional resources like `package.json`, `README.md`, `config.d.ts`, etc. +- Changes to any test, storybook, or other local development files, for example, `MyComponent.test.tsx`, `MyComponent.stories.tsx`, `**mocks**/MyMock.ts`, `.eslintrc.js`, `setupTests.ts`, or `api-report.md`. Explained differently, it is only files that affect the published package that need changesets, such as source files and additional resources like `package.json`, `README.md`, `config.d.ts`, etc. - When tweaking a change that has not yet been released, you can rely on and potentially modify the existing changeset instead. - Changes that do not belong to a published packages, either because it's not a package at all, such as `docs/`, or because the package is private, such as `packages/app`. - Changes that do not end up having an effect on the published package, such as whitespace fixes or code formatting changes. Although it's also fine to have a short changeset for these kind of changes too. From 599b9d00698c2b7b5549ad12f8ccf80cc006690b Mon Sep 17 00:00:00 2001 From: Patrik Oldsberg Date: Thu, 20 Oct 2022 09:41:45 +0200 Subject: [PATCH 12/15] Update REVIEWING.md MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Fredrik Adelöw Signed-off-by: Patrik Oldsberg --- REVIEWING.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/REVIEWING.md b/REVIEWING.md index 9d88c36b36..cccbe90b3d 100644 --- a/REVIEWING.md +++ b/REVIEWING.md @@ -147,7 +147,7 @@ These are changes where the behavior of the code changes, but the public API is It's hard to set up exact rules for when a behavioral change is breaking or not. In some cases it's obvious, for example if you remove important functionality of a system, while in other cases it can be very hard to tell. In the end what's important is whether a significant number of users of the package will be negatively impacted by the change. One question that you can ask yourself here is "is it likely that there are users that don't want the new behavior, or will need to change their code to adapt to the new behavior?" If the answer is yes, then it's likely a breaking change. You do also want to keep [xkcd.com/1172](https://xkcd.com/1172/) in mind though. -Note that even a bug fix can be considered a breaking change in some situations. One things to lean on in that case is what the _documented_ behavior is. If the current behavior does not match the documented behavior, then a change to match the documentation is generally not a breaking change. That is unless it is likely that there are a significant number of users that will be impacted by the change. +Note that even a bug fix can be considered a breaking change in some situations. One thing to lean on in that case is what the _documented_ behavior is. If the current behavior does not match the documented behavior, then a change to match the documentation is generally not a breaking change. That is unless it is likely that there are a significant number of users that will be impacted by the change. For tricky behavioral changes you may simply need to let end users provide feedback. This can be done either by hiding the new behavior behind an experimental feature switch, or by releasing the change early on in the release cycle, preferably in the first or second next line release. Be ready to respond to feedback and potentially revert the change if needed. From dacd03daf84c53481b6df969da752a9bf875623e Mon Sep 17 00:00:00 2001 From: Patrik Oldsberg Date: Thu, 20 Oct 2022 09:45:38 +0200 Subject: [PATCH 13/15] Update REVIEWING.md MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Fredrik Adelöw Signed-off-by: Patrik Oldsberg --- REVIEWING.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/REVIEWING.md b/REVIEWING.md index cccbe90b3d..686844b88e 100644 --- a/REVIEWING.md +++ b/REVIEWING.md @@ -177,7 +177,7 @@ An important distinction to make when looking at changes to an API Report is the #### Input Types -A input type is one where a value needs to be provided by users of the package. The most common form of input type are function, constructor, and method parameters. +An input type is one where a value needs to be provided by users of the package. The most common form of input type are function, constructor, and method parameters. The following is an example where `MyComponentProps` is an input type: From ff621dbb5b507baad96510625032f2b65b922717 Mon Sep 17 00:00:00 2001 From: Patrik Oldsberg Date: Thu, 20 Oct 2022 09:49:38 +0200 Subject: [PATCH 14/15] Update REVIEWING.md MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Fredrik Adelöw Signed-off-by: Patrik Oldsberg --- REVIEWING.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/REVIEWING.md b/REVIEWING.md index 686844b88e..89d9212274 100644 --- a/REVIEWING.md +++ b/REVIEWING.md @@ -229,7 +229,7 @@ When modifying an output type, any change that reduces constraints are breaking. Adding new properties is not a breaking change, regardless of whether they are optional or not. Removing properties is on the other hand always breaking. -It is generally fine to increase constraints without it being a breaking change. For example, if we made the `title` property required, that would not be breaking. +It is generally fine to increase constraints without it being a breaking change. For example, if we made the `shape` property required, that would not be breaking. There are some edge-cases though, for example if `shape` was changed to just `'square'`, that would be a breaking change because consumers might be checking for `box.shape === 'rounded'`, which would then be breaking. It's typically a quite easy thing for consumers to fix though. More generally, type unions and discriminated unions are quite troublesome in output types, as both adding and removing types from them are considered breaking changes. From d6e5523326e94f91369d2699fa8a6fd2cc10e9d1 Mon Sep 17 00:00:00 2001 From: Patrik Oldsberg Date: Thu, 20 Oct 2022 09:50:28 +0200 Subject: [PATCH 15/15] Update REVIEWING.md MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Fredrik Adelöw Signed-off-by: Patrik Oldsberg --- REVIEWING.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/REVIEWING.md b/REVIEWING.md index 89d9212274..469ac249f7 100644 --- a/REVIEWING.md +++ b/REVIEWING.md @@ -149,7 +149,7 @@ It's hard to set up exact rules for when a behavioral change is breaking or not. Note that even a bug fix can be considered a breaking change in some situations. One thing to lean on in that case is what the _documented_ behavior is. If the current behavior does not match the documented behavior, then a change to match the documentation is generally not a breaking change. That is unless it is likely that there are a significant number of users that will be impacted by the change. -For tricky behavioral changes you may simply need to let end users provide feedback. This can be done either by hiding the new behavior behind an experimental feature switch, or by releasing the change early on in the release cycle, preferably in the first or second next line release. Be ready to respond to feedback and potentially revert the change if needed. +For tricky behavioral changes you may simply need to let end users provide feedback. This can be done either by hiding the new behavior behind an experimental feature switch, or by releasing the change early on in the release cycle, preferably in the first or second `next`-line release. Be ready to respond to feedback and potentially revert the change if needed. ### Public API Changes