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

Add documentation for deprecating styles. #38540

Merged
merged 4 commits into from
Feb 10, 2022
Merged
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
69 changes: 68 additions & 1 deletion packages/components/CONTRIBUTING.md
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,11 @@ In these situations, one possible approach is to "soft-deprecate" a given legacy

When adding new components or new props to existing components, it's recommended to prefix them with `__unstable` or `__experimental` until they're stable enough to be exposed as part of the public API.

Learn more on [How to preserve backward compatibility for a React Component](/docs/contributors/code/backward-compatibility.md#how-to-preserve-backward-compatibility-for-a-react-component) and [Experimental and Unstable APIs](/docs/contributors/code/coding-guidelines.md#experimental-and-unstable-apis).
### Learn more

- [How to preserve backward compatibility for a React Component](/docs/contributors/code/backward-compatibility.md#how-to-preserve-backward-compatibility-for-a-react-component)
- [Experimental and Unstable APIs](/docs/contributors/code/coding-guidelines.md#experimental-and-unstable-apis)
- [Deprecating styles](#deprecating-styles)

<!-- ## Polymorphic Components (i.e. the `as` prop)

Expand Down Expand Up @@ -180,6 +184,69 @@ All new component should be styled using [Emotion](https://emotion.sh/docs/intro

Note: Instead of using Emotion's standard `cx` function, the custom [`useCx` hook](/packages/components/src/utils/hooks/use-cx.ts) should be used instead.


### Deprecating styles
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this deprecation strategy be recommended also beyond styles? It seems like a reasonable strategy to use also for behavioural changes, API changes, etc...

Copy link
Member Author

@mirka mirka Feb 8, 2022

Choose a reason for hiding this comment

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

I do see this as a special case of deprecating APIs, where there is no actual prop to deprecate and it's just a change in default behavior. Looking at the instances in components/* where deprecated() is called, I'm not sure this kind of "change in default" deprecation has ever happened. Are you aware of any?

I did choose the __next* prefix so it would be appropriate for behavioral changes as well though, so it probably would be generally applicable when it happens. I think we could reuse or refactor this section when we write a more detailed deprecation guide for components.

By the way I just learned about this section that asks us to add a "Needs Dev Note" label to the PR. I'll add this into the doc.

Copy link
Contributor

Choose a reason for hiding this comment

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

Are you aware of any?

On top of my mind, no. Hey @sarayourfriend (sorry for the ping!) — do you ever remember an instance of a deprecation where the meaning of a style/prop was changed (causing a potentially breaking change)?

I did choose the __next* prefix so it would be appropriate for behavioral changes as well though, so it probably would be generally applicable when it happens. I think we could reuse or refactor this section when we write a more detailed deprecation guide for components.

Sounds good to me. Having a defined way to "safely" introduce a breaking change is great and we should absolutely iterate on this procedure in order to be able to apply it to more deprecation scenarios

By the way I just learned about this section that asks us to add a "Needs Dev Note" label to the PR. I'll add this into the doc.

TIL !

Copy link
Contributor

Choose a reason for hiding this comment

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

do you ever remember an instance of a deprecation where the meaning of a style/prop was changed (causing a potentially breaking change)

I can't think of any from my time working on wp/components but that's mostly because I'm fairly certain we only "soft" deprecated things.

We did change the variant prop on the Text component, but it was experimental. We originally wrote an adapter for it IIRC but it isn't there anymore, or maybe it was never merged because the component was experimental anyway.

Although, does this count? #33490

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, that's what I thought as well.

Although, does this count? #33490

Not sure if it does, I'd probably see it as more of a bug fix — isScrollable was implicitly introduced to CardBody in a previous PR, and the PR that you highlighted changes its default value to fix that regression.

Copy link
Contributor

Choose a reason for hiding this comment

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

Right right... I feel like I remember seeing a bug raised where we'd accidentally left a deprecation or prop-meaning change out of the changelogs somewhere and it'd caused problems for someone consuming the package. Just can't remember how to find it now 😞

Copy link
Contributor

Choose a reason for hiding this comment

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

I believe the original "breaking change" was caused by my refactor of Card in #32566

Copy link
Contributor

Choose a reason for hiding this comment

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

Ahhh yes! That is what I was remembering. But it was just an accidental prop value rename 😬 Doesn't really match what we're talking about here.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good to know, thanks!


Changing the styles of a non-experimental component must be done with care. To prevent serious breakage in third-party usage, in some cases we may want a grace period before fully removing the old styles. This can be done by temporarily placing the new styles behind a feature flag prop prefixed by `__next`, accompanied by a `deprecate()` warning in the console. The feature flag should be opt-in (false by default), and have a reasonably descriptive name (**not** `__nextHasNewStyles`). A descriptive name allows for multiple deprecations to proceed in parallel, separated by concerns or by deprecation version.

```jsx
// component.tsx
import deprecated from '@wordpress/deprecated';
import { Wrapper } from './styles.ts';

function MyComponent({ __nextHasNoOuterMargins = false }) {
if ( ! __nextHasNoOuterMargins ) {
deprecated( 'Outer margin styles for wp.components.MyComponent', {
since: '6.0',
version: '6.2', // Set a reasonable grace period depending on impact
hint:
'Set the `__nextHasNoOuterMargins` prop to true to start opting into the new styles, which will become the default in a future version.',
} );
}
return <Wrapper __nextHasNoOuterMargins={__nextHasNoOuterMargins} />
}
```

Styles should be structured so the deprecated styles are cleanly encapsulated, and can be easily removed when the deprecation version arrives.

```js
// styles.ts
const deprecatedMargins = ({ __nextHasNoOuterMargins }) => {
if ( ! __nextHasNoOuterMargins ) {
return css`
margin: 8px;
`;
}
};

export const Wrapper = styled.div`
margin: 0;

${deprecatedMargins}
`;
```

Once deprecated, code examples in docs/stories should include the opt-in prop set to `true` so that new consumers are encouraged to adopt it from the start.
mirka marked this conversation as resolved.
Show resolved Hide resolved

Remember to [add a **Needs Dev Note** label](/docs/contributors/code/backward-compatibility.md##dev-notes) to the pull request so third-party developers can be informed of the deprecation.

When the grace period is over and the deprecation version arrives, the `__next*` prop, deprecation notice, and deprecated styles should all be completely removed from the codebase.

#### Criteria for putting styles changes behind a feature flag

Not all style changes justify a formal deprecation process. The main thing to look for is whether the changes could cause layouts to break in an obvious or harmful way, given that the component is being used in a standard fashion.

##### DOES need formal deprecation

- Removing an outer margin.
- Substantial changes to width/height, such as adding or removing a size restriction.

##### DOES NOT need formal deprecation

- Breakage only occurs in non-standard usage, such as when the consumer is overriding component internals.
- Minor layout shifts of a few pixels.
- Internal layout changes of a higher-level component.
mirka marked this conversation as resolved.
Show resolved Hide resolved

## Context system

The `@wordpress/components` context system is based on [React's `Context` API](https://reactjs.org/docs/context.html), and is a way for components to adapt to the "context" they're being rendered in.
Expand Down