Skip to content

docs: add versioning guide #2753

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.

Sign up for GitHub

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

Closed
joshblack wants to merge 10 commits into from
Closed

docs: add versioning guide #2753

Changes from 4 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
c2eb7ae
docs: add versioning guide
joshblack Jan 9, 2023
4d71ac8
docs: run doctoc
joshblack Jan 9, 2023
0a052f8
Update versioning.md
joshblack Jan 9, 2023
0af1868
docs(versioning): update about section and add todo notes for bumps
joshblack Jan 10, 2023
ca3559b
Update versioning.md
joshblack Jan 12, 2023
621640b
Update versioning.md
joshblack Jan 24, 2023
2b7c947
Merge branch 'main' into docs/add-versioning-docs
joshblack Jan 25, 2023
fa353fd
Add token reference
joshblack Jan 30, 2023
0800394
Update versioning.md
joshblack Jan 30, 2023
37cfd07
Update versioning.md
joshblack Jan 30, 2023
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
132 changes: 132 additions & 0 deletions contributor-docs/versioning.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,132 @@
# Versioning

<!-- prettier-ignore-start -->
<!-- START doctoc generated TOC please keep comment here to allow auto update -->
<!-- DON'T EDIT THIS SECTION, INSTEAD RE-RUN doctoc TO UPDATE -->
## Table of Contents

- [Overview](#overview)
- [Changes](#changes)
- [Examples](#examples)
- [A prop is added to a component](#a-prop-is-added-to-a-component)
- [An existing prop is deprecated](#an-existing-prop-is-deprecated)
- [The DOM node that an `id` corresponds to is changed](#the-dom-node-that-an-id-corresponds-to-is-changed)
- [The DOM node that an `aria-label` corresponds to is changed](#the-dom-node-that-an-aria-label-corresponds-to-is-changed)

<!-- END doctoc generated TOC please keep comment here to allow auto update -->
<!-- prettier-ignore-end -->

## Overview

The Primer team aims to follow
[Semantic Versioning](https://semver.org/) (semver) for each of the packages
that we ship. From semver.org, this means that:

> Given a version number MAJOR.MINOR.PATCH, increment the:
>
> 1. **MAJOR** version when you make incompatible API changes,
> 2. **MINOR** version when you add functionality in a backwards compatible
> manner, and
> 3. **PATCH** version when you make backwards compatible bug fixes.
>
> _Additional labels for pre-release and build metadata are available as
> extensions to the MAJOR.MINOR.PATCH format._

As a result, whenever you see a `minor` or `patch` update for a package from the
Copy link
Contributor

@rezrah rezrah Jan 10, 2023

Choose a reason for hiding this comment

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

- ... a `minor` or `patch` update for a package from the Primer you should feel confident ...
+ ... a `minor` or `patch` update to a package from Primer you should feel confident ...

Primer you should feel confident that you can update without
anything breaking in your project. For a full list of changes and their
corresponding semver bumps, check out the [changes](#changes) table below.

For a full list of releases, visit our [releases](https://github.com/primer/react/releases) page.

## Changes

| Category | Type of change | semver bump |
| :--------- | :------------------------------------------------------------ | :---------- |
| Component | A component is added | `minor` |
| | A component is deprecated | `major` |
Copy link
Contributor

@rezrah rezrah Jan 10, 2023

Choose a reason for hiding this comment

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

This one's interesting:

  1. In Primer Primitives deprecating something doesn't constitute a material breaking change in the output, but serves as a flag that something will soon be removed and that we have provided an alternative to ease migration
  2. During v35 major release, I recall the team suggested in a retro that we don't make deprecations major. We had the same feedback from library users.

Copy link
Member Author

@joshblack joshblack Jan 10, 2023

Choose a reason for hiding this comment

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

@rezrah I definitely agree, deprecations in minor releases feel more appropriate/align with expectations.

Copy link
Member

@broccolinisoup broccolinisoup Jan 11, 2023

Choose a reason for hiding this comment

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

During v35 major release, I recall the team suggested in a retro that we don't make deprecations major. We had the same feedback from library users.

I am very curious to learn more about this. It is very interesting.

We will be deprecating UnderlineNav in v36 and it is planned to be a major change. We re-wrote the component entirely and the public API has changed. I don't have much experience with versioning but I am super keen to improve! I am curious to know if it is still possible to refactor UnderlineNav and introduce as a minor change with warning the deprecation and providing the new API as an alternative 🤔 Would it worth giving a try or not really at this stage?

Current UnderlineNav
Newly written UnderlineNav

Copy link
Contributor

@rezrah rezrah Jan 12, 2023

Choose a reason for hiding this comment

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

@joshblack @broccolinisoup I've somehow found the FigJam file I was referencing in my earlier comment. You should receive a link to it in your emails.

@broccolinisoup - my reading of your situation based on the diagram would be to issue the deprecation notice through comms (docs, changelogs, etc) and a patch (or minor) update of the libraries (for JSDOC @deprecate annotations). Main thing is to leave it in-place. We're just issuing notice AoT.

Then wait for the next major release (~couple of weeks) to move it into the deprecated bundle and move UnderlineNavV2 from drafts into main bundle. Eventually few months after, we can remove deprecated component altogether.

Could this still work for you?

Copy link
Member

@broccolinisoup broccolinisoup Jan 13, 2023

Choose a reason for hiding this comment

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

I've somehow found the FigJam file I was referencing in my earlier comment. You should receive a link to it in your emails.

Thanks, this is brilliant 🙌🏼

Could this still work for you?

@rezrah Thank you so much for clarifying the process! According to your explanation, we are on track. The only thing is different than the plan is updating the old UnderlineNav with the @deprecate status as a patch (or minor) update before the major release. Would that still be a good practise without promoting the new UnderlineNav as an alternative component (As this plan to come as a major later)? I think this is what I am trying to wrap my head around.

Thank you so much 🙏🏼

Copy link
Member

@broccolinisoup broccolinisoup Jan 23, 2023

Choose a reason for hiding this comment

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

Hi! @rezrah just wanted to resurface this, would love to hear your thoughts 😊

Copy link
Contributor

@rezrah rezrah Jan 24, 2023

Choose a reason for hiding this comment

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

Hey @broccolinisoup, sorry I missed this earlier 😞.

Would that still be a good practise without promoting the new UnderlineNav as an alternative component

I think so, because the idea behind issuing a deprecation notice before creating a breaking change (as a result of moving from main bundle to deprecated) is that we're giving notice of a deprecation ahead of time so there are no surprises. Some of the recurring feedback received after the v35 release from teams around GitHub was that replacing certain deprecated components was not on their priority list, but upgrading to a major is still important. Had they known ahead of time that we were deprecating a component, they could have been better prepared for it.

I think this problem is exacerbated by us having a separate bundle for deprecated components.

TL;DR the proposal in my suggestion is:

  1. Issue @deprecated notice for UnderlineNav in next patch/minor release. At same time, provide the new UnderlineNav in the drafts bundle and update any relevant docs about this upcoming change.
  2. Release a technical preview. See here for v35 example.
  3. Wait for next major release.
  4. Cut the major release and Move UnderlineNav(v1) into deprecated and UnderlineNav(v2) into main bundle.

What do you think?

Copy link
Member

@broccolinisoup broccolinisoup Jan 27, 2023

Choose a reason for hiding this comment

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

That sounds great! I appreciate the very detailed explanation as well 🙇🏻‍♀️ I'll capture this info in the deprecation process documentation PR.

| | A component is removed | `major` |
| Props | A prop is added to a component | `minor` |
| | The type of a prop is made more general | `minor` |
| | The type of a prop is made more specific | `major` |
| | A prop is deprecated | `minor` |
| | A prop is removed from a component | `major` |
| | The element to which additional props are added to is changed | `<todo>` |
| Markup | The DOM node that an `id` corresponds to is changed | `<todo>` |
| | The DOM node that an `aria-label` corresponds to is changed | `<todo>` |
| | The `role` of a component is changed | `<todo>` |
| Styles | The `position` of the outermost element is updated | `<todo>` |
| | The `display` property of the outermost element is updated | `<todo>` |
| | A flex or grid property is updated | `<todo>` |
| | A selector is added | `<todo>` |
| | A selector is removed | `<todo>` |
| | The specificity of a selector is raised | `<todo>` |
| | The specificity of a selector is lowered | `<todo>` |
| Behavior | Interactions are added to a component | `<todo>` |
| | Interactions are removed from a component | `<todo>` |
| TypeScript | A type is added | `minor` |
Copy link
Contributor

@rezrah rezrah Jan 10, 2023

Choose a reason for hiding this comment

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

Do you have any examples of this one in mind? I think this could also be a patch in certain scenarios?

Copy link
Member Author

@joshblack joshblack Jan 10, 2023

Choose a reason for hiding this comment

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

Really just was thinking of a narrow case where a type has been added to the Public API of the package.

In this scenario, I framed it as a minor since it would be additive and would require someone to explicitly import it in order to use it.

Let me know if there is a better way to talk about types in this kind of context, I am honestly not sure how to frame type changes in terms of semver so was just applying a propTypes mindset to this kind of section 👀

Copy link
Contributor

@rezrah rezrah Jan 12, 2023

Choose a reason for hiding this comment

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

ah gotcha, thanks for clarifying. I think a type addition that doesn't make a material API change, or something that doesn't require the user to react to can go into a patch. A prominent new feature however, like a new component entirely - which includes new types - would constitute a minor in my mind. I agree that it's nuanced however. Perhaps we can split this up into different type edge cases.

| | A type is made more general | `minor` |
| | A type is made more specific | `major` |
| | A type is removed | `major` |
| Package | An entrypoint is added to the package | `minor` |
| | An entrypoint is removed from a package | `major` |

Copy link
Member

@broccolinisoup broccolinisoup Jan 10, 2023

Choose a reason for hiding this comment

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

Should we add a section for Accessibility or things related to accessibility can go under other corresponding sections?

Copy link
Member Author

@joshblack joshblack Jan 10, 2023

Choose a reason for hiding this comment

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

@broccolinisoup makes sense to me! What kind of situations would be good to place under an Accessibility category?

Copy link
Member

@broccolinisoup broccolinisoup Jan 13, 2023

Choose a reason for hiding this comment

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

I guess things in my mind can certainly go under other categories but maybe explicitly having a section for Accessibility feels more inclusive? 🙂

I was thinking;

  • Exposing a mandatory aria attribute in the API
  • Introducing a11y remediations that result in breaking changes in a current user behaviour
  • Updating keyboard interactions

Does any of it sound applicable at all?

### Examples

#### A prop is added to a component

semver bump: **minor**

```diff
type Props = {
propA: string;
+ propB: string;
};

function ExampleComponent({
propA,
+ propB,
}: Props) {
return (
<>
<span>{propA}</span>
+ <span>{propB}</span>
</>
);
}
```

#### An existing prop is deprecated

semver bump: **minor**

```diff
type Props = {
propA: string;
+ /**
+ * @deprecated This prop will be removed in the next major version of
+ * `@primer/react`. Please use <replacement> instead.
+ */
propB: string;
};

function ExampleComponent({
propA,
+ propB,
}: Props) {
return (
<>
<span>{propA}</span>
+ <span>{propB}</span>
</>
);
}
```

#### The DOM node that an `id` corresponds to is changed

semver bump: **major**

#### The DOM node that an `aria-label` corresponds to is changed

semver bump: **minor**