Create deprecating-components.md #2733
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| @@ -0,0 +1,77 @@ | ||||||
| Primer components may occasionally be deprecated when they are no longer recommended for use. In some cases, we may recommend replacing a deprecated component with a new one. New replacement components are typically built in a parallel bundle to avoid breaking existing components, and are promoted to the main bundle when it's time for migration. | ||||||
|
|
||||||
| ## Developing a replacement component | ||||||
|
|
||||||
| Start building the new component outside of the main bundle. This includes the source code of the component being under the `src/drafts` folder and the component being exported in the draft bundle (`src/drafts/index.ts`). That way, folks who | ||||||
| who would like to try the draft version of the component can export it from the draft bundle. | ||||||
|
|
||||||
| ``` | ||||||
| import { ActionList } from "@primer/react/drafts" | ||||||
| ``` | ||||||
|
|
||||||
| If it is a 1:1 replacement, it's useful to keep the component name the same for consumers. Internally, we would want to, for example, call the folder name `ActionList2` and call the docs `ActionMenu v2`. | ||||||
|
|
||||||
| ## Deprecating the component | ||||||
|
|
||||||
| When Primer maintainers are ready to deprecate a component, they will plan for an upcoming major release that will include the change. This includes preparing code changes and communications about the change. | ||||||
|
|
||||||
| ### Code side of the deprecation | ||||||
broccolinisoup marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||||||
|
|
||||||
| Here is a checklist for developers to smoothen the deprecation process. | ||||||
|
|
||||||
| #### Source Code | ||||||
|
|
||||||
| - [ ] Move the old component's source code under the `src/deprecated` folder. | ||||||
| - [ ] Remove the old component export in the main bundle to be added to the deprecated one. | ||||||
| - [ ] Export the old component in the deprecated bundle `src/deprecated/index.ts`. | ||||||
| - [ ] Rename the new component to its neutral version if it includes `2` or `v2`. I.e. `ActionList2` -> `ActionList`. | ||||||
| - [ ] Move the new component's source code to the `src` folder if it is under `src/drafts`. | ||||||
| - [ ] Export the new component in the main bundle `src/index.ts`. | ||||||
|
|
||||||
| #### Storybook | ||||||
|
|
||||||
| - [ ] Update the new component's storybook title from `Drafts/Components/<Component-Name>` to `Components/<Component-Name>`. | ||||||
| - [ ] Move the old component's stories under `src/stories/deprecated`. | ||||||
|
|
||||||
| #### Tests | ||||||
|
|
||||||
| - [ ] Move the old component's tests under `src/__tests__/deprecated`. | ||||||
| - [ ] Update `script/generate-e2e-tests.js` to reflect the new component's component ID is generated by the storybook title. | ||||||
| - [ ] Update the snapshots using the command `npm run test -- -u`. | ||||||
|
|
||||||
| #### Docs | ||||||
|
|
||||||
| - [ ] Move the deprecated component's docs to the `src/docs/deprecated` folder and update the title by adding `(legacy)`, status as well as the storybook and the source code link. | ||||||
| - [ ] Make sure to update `jsx live` -> `jsx live deprecated` | ||||||
| - [ ] Make sure to update the import code block. I.e. `import { ActionList } from "@primer/react/deprecated"`. | ||||||
| - [ ] Add a `Deprecation` section with the link of the newly developed component and provide a diff. See [deprecated ActionList docs](https://primer.style/react/deprecated/ActionList#deprecation) as an example. | ||||||
| - [ ] Move the new components docs from draft folder to the main docs folder `docs/content/` and update the title by removing `v2`, status as well as the storybook and the source code link. | ||||||
| - [ ] Make sure to update `jsx live drafts` -> `jsx live` | ||||||
| - [ ] Make sure to update the import code block. I.e. `import { ActionList } from "@primer/react"`. | ||||||
| - [ ] Update the navigation on `docs/src/@primer/gatsby-theme-doctocat/nav.yml` accordingly. | ||||||
|
|
||||||
| ### Comms side of the deprecation | ||||||
|
|
||||||
| When we know if the newly developed component is ready to go in the main bundle and the old component is good to be deprecates in the next major release, we announce the deprecation and the promotion in the [Primer changelog](https://github.com/github/primer/discussions/categories/primer-changelog) (GitHub staff only) under the `Coming soon` section. | ||||||
|
|
||||||
| To ensure the impending deprecation is included in the Primer changelog, Primer maintainers should highlight any upcoming deprecations in their [weekly status updates](https://github.com/github/design-infrastructure/blob/main/how-we-work/planning-and-tracking-work/updates.md#weekly-status-updates-required) (GitHub staff only). | ||||||
|
|
||||||
| Once the major release is out, we provide support on how to switch using the newly develop component as well as how to continue to use the deprecated component for folks who prefer to migrate later on. | ||||||
|
|
||||||
| We also write codemods to deprecate the old component and use the new component under [our migration repo](https://github.com/primer/react-migrate#readme). | ||||||
|
||||||
|
|
||||||
| ### Primer maintainers to alert Accessibility team about deprecations | ||||||
|
|
||||||
| As GitHub teams onboard to the [accessibility scorecard](https://github.com/github/engineering/discussions/2443) (GitHub staff only), we have a new opportunity to encourage teams to remove deprecated components and adopt new, more accessible ones. | ||||||
|
|
||||||
| Primer maintainers should reach out to #accessibility any time a component is planned for deprecation, or if there is a new component that we want to enforce usage around. | ||||||
|
|
||||||
| ## Removing the deprecated component | ||||||
|
|
||||||
| After about 6 months of living in the deprecated bundle, a component is retired/deleted from the codebase. This is also a breaking change and we release it in the upcoming major release. | ||||||
| At this point, consumers are expected to plan migration work and we annouce the deletion in Primer Changelog as we do for the deprecation. | ||||||
|
|
||||||
| ## FAQ | ||||||
|
|
||||||
| - Does the deprecated component accept new feature request? | ||||||
broccolinisoup marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||||||
| - No, because we have a single bundle for all components, you can not pick the components you want to upgrade. This can result in additional unrelated work. | ||||||
|
||||||
| - No, because we have a single bundle for all components, you can not pick the components you want to upgrade. This can result in additional unrelated work. | |
| - No, once a component is deprecated the only changes it will receive are bug fixes. |
Member
There was a problem hiding this comment.
What do y'all think: should this be will receive or may receive? Not sure how dedicated we've been to handling all bugfixes in deprecated components, or just major functional bugs 🤔
Member
Author
There was a problem hiding this comment.
"may receive" sounds good to me. I like that it removes the certainty.
I updated to be "may receive" but please let me know if anyone has a concern 🙌🏻
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see some of the drafts component under the
src/draftsand some not. Would that be true that we eventually want to have the practise to create the drafts components under thesrc/drafts?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@siddharthkp instead of recommending these components go into
src/drafts, do you think we should be encouraging folks to build directly in the new experimental components repo instead? https://github.com/github/experimental-react-componentsThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh super keen to hear about this 👀
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes!
But given the change in direction for experimental-components, i'm not sure if it should be
github/experimental-react-componentsor our ownprimer/react-experimental, another ADR? 😅There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, probably. @justinbyo is going to be helping us figure out ownership models for shared React components looking forward.
In the meantime, are we comfortable shipping these docs with the direction to build direct in
experimental-react-componentsfor now, and we can always update if/when we make a change? We don't seem to deprecate components super often so I'm thinking that changing this guidance later wouldn't be a huge problem.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point @broccolinisoup. In most cases we should have similar standards for components (including unit tests and Storybook) in the
experimental-react-componentsrepo as we do inprimer/react. I'm admittedly not as familiar with all of the items in the checklist and whether they would apply exactly the same inexperimental-react-components; what do you think @siddharthkp?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am a fan of this!
I wasn't sure how much we can "influence"
experimental-react-componentsto follow our Primer component standards.From my observation,
experimental-react-componentsdiffer from PRC in the way of documenting components and they don't include some util unit test functions as PRC do.Super keen to hear @siddharthkp's thoughts.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👋 thanks for DMing me! i've lost the fight against notifications.
Oooh, we definitely should influence it a lot. We can even assist (and enforce) practices by baking them into the react toolchain.
However, we also keep the barrier for contribution pretty low. Experimental components are meant to be pre-alpha in the lifecycle, more context in ADR 008
Some of this, like the docs, is intentional, we chose storybook docs over doctocat to make it easier (and less elaborate) to document than primer. The lack of test functions isn't part of the strategy, that's just because we've only had one iteration of this till now :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome to hear!
This is a pretty good point!
Gotcha!
It feels to me that moving experimental components from
react-experimental-componentsis a slightly different process than deprecating components, especially around the practises.Another question - if an existing component was to be re-written by a Primer engineer (following all Primer standards), would we still start in the
experimental-react-componentsrepo or in our own repos?cc @lesliecdubs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hello! @siddharthkp and @lesliecdubs
Seems like we still have questions regarding to
experimental-react-componentsrepo's practises and how they will make their way to Primer. Would it be okay to go ahead with this section as is (creating the components in thedraftsfolder) until we have a clear practise forexperimental-react-components? At least we will treat this document as the current practise and we can update as we go?