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.

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

Button: Keep deprecated props in type definitions #59913

Merged
merged 4 commits into from
Mar 18, 2024
Merged

Button: Keep deprecated props in type definitions #59913

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
df54580
Button: Keep deprecated props in type definitions
mirka Mar 15, 2024
b25ec43
Remove hard deprecation verison for `isDefault`
mirka Mar 15, 2024
fbd6d28
Add changelog
mirka Mar 15, 2024
13a5126
Add package namespace to deprecation message
mirka Mar 15, 2024
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
4 changes: 4 additions & 0 deletions packages/components/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,10 @@

- `TextControl`: Add typings for `date`, `time` and `datetime-local` ([#59666](https://github.com/WordPress/gutenberg/pull/59666)).

### Internal

- `Button`: Keep deprecated props in type definitions ([#59913](https://github.com/WordPress/gutenberg/pull/59913)).

## 27.1.0 (2024-03-06)

### Bug Fix
Expand Down
5 changes: 2 additions & 3 deletions packages/components/src/button/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,10 +65,9 @@ function useDeprecatedProps( {
}

if ( isDefault ) {
deprecated( 'Button isDefault prop', {
deprecated( 'wp.components.Button `isDefault` prop', {
Copy link
Member

Choose a reason for hiding this comment

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

Any idea why we don't have deprecated() calls for the rest of the variant old props (like isSmall, isPrimary, etc.)?

Copy link
Member Author

Choose a reason for hiding this comment

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

I can't find an explicit reason being given, but it does look intentional.

Copy link
Member

Choose a reason for hiding this comment

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

Looks like we just wanted to soft-deprecate first, and to hard-deprecate later: #31713 (comment)

It's been almost 3 years, so maybe it's about time for hard deprecation.

since: '5.4',
alternative: 'variant="secondary"',
version: '6.2',
Copy link
Member

Choose a reason for hiding this comment

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

Should we at least bump this so we still remove it in some future WP version?

Copy link
Member Author

Choose a reason for hiding this comment

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

It's the ambiguous back compat policy biting us again 😅

Up to this point the components package hasn't done many hard deprecations, mostly because a lot of these back compat layers are very low cost. Like virtually zero cost compared to the migration management cost we'd push to our third-party consumers who have to support multiple WP versions, and also the cost incurred on the end users who see incorrectly styled UI from all the consumers that never migrate.

And in that vein, I'm not particularly enthusiastic about logging deprecation warnings on "simple translation" API changes like this that really add no value to anyone. If we're not planning to hard deprecate, then there's no reason for consumers to migrate to the new API unless they want to for ergonomics or code quality reasons. I've been hearing that even managing these deprecation warnings while supporting multiple WP versions is cumbersome, so I don't consider them "free" even though they're dev-env only.

Happy to discuss and explicitly codify things a bit more, but I'm in no rush to hard deprecate things unless they have a tangible maintenance cost.

} );

computedVariant ??= 'secondary';
Expand All @@ -87,7 +86,7 @@ function useDeprecatedProps( {
}

export function UnforwardedButton(
props: ButtonProps,
props: ButtonProps & DeprecatedButtonProps,
ref: ForwardedRef< any >
) {
const {
Expand Down
5 changes: 0 additions & 5 deletions packages/components/src/button/test/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -554,33 +554,28 @@ describe( 'Button', () => {

describe( 'deprecated props', () => {
it( 'should not break when the legacy isPrimary prop is passed', () => {
// @ts-expect-error
render( <Button isPrimary /> );
expect( screen.getByRole( 'button' ) ).toHaveClass( 'is-primary' );
} );

it( 'should not break when the legacy isSecondary prop is passed', () => {
// @ts-expect-error
render( <Button isSecondary /> );
expect( screen.getByRole( 'button' ) ).toHaveClass(
'is-secondary'
);
} );

it( 'should not break when the legacy isTertiary prop is passed', () => {
// @ts-expect-error
render( <Button isTertiary /> );
expect( screen.getByRole( 'button' ) ).toHaveClass( 'is-tertiary' );
} );

it( 'should not break when the legacy isLink prop is passed', () => {
// @ts-expect-error
render( <Button isLink /> );
expect( screen.getByRole( 'button' ) ).toHaveClass( 'is-link' );
} );

it( 'should warn when the isDefault prop is passed', () => {
// @ts-expect-error
render( <Button isDefault /> );
expect( screen.getByRole( 'button' ) ).toHaveClass(
'is-secondary'
Expand Down
30 changes: 30 additions & 0 deletions packages/components/src/button/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -149,10 +149,40 @@ type AnchorProps = {
};

export type DeprecatedButtonProps = {
/**
* Gives the button a default style.
*
* @deprecated Use the `'secondary'` value on the `variant` prop instead.
* @ignore
*/
isDefault?: boolean;
/**
* Gives the button a link style.
*
* @deprecated Use the `'link'` value on the `variant` prop instead.
* @ignore
*/
isLink?: boolean;
/**
* Gives the button a primary style.
*
* @deprecated Use the `'primary'` value on the `variant` prop instead.
* @ignore
*/
isPrimary?: boolean;
/**
* Gives the button a default style.
*
* @deprecated Use the `'secondary'` value on the `variant` prop instead.
* @ignore
*/
isSecondary?: boolean;
/**
* Gives the button a text-based style.
*
* @deprecated Use the `'tertiary'` value on the `variant` prop instead.
* @ignore
*/
isTertiary?: boolean;
};

Expand Down
Loading