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

Button: Keep deprecated props in type definitions #59913

Merged
merged 4 commits into from
Mar 18, 2024
Merged

Conversation

mirka
Copy link
Member

@mirka mirka commented Mar 15, 2024

In preparation for #59733
Informed by feedback in #55401

What?

Tweaks how the deprecated props on Button are managed so that they are more permissive:

  • Keep deprecated props in the TypeScript type definitions, so that they will be flagged as deprecated but not block TS compilation.
  • Remove the hard deprecation version for the isDefault prop, since this is a very low maintenance back compat layer and we can pretty much keep it around indefinitely.

Why?

It's already been a while since these props have been deprecated, but I still think it's worthwhile to set some good patterns here before adding new deprecations like in #59734.

The main back compat considerations I think we should uphold are:

  • Keep deprecated props in the type definitions so compilation does not fail, and to prevent future prop name collisions.
  • Hard deprecations don't necessarily need to happen if the maintenance cost of the back compat layer is negligible.

Testing Instructions

  • Type checks still pass
  • Using one of the deprecated Button props should not throw a TS error
  • The deprecated props should still be hidden from the Storybook props table (The JSDoc @ignore tag does this)

@mirka mirka added Backwards Compatibility Issues or PRs that impact backwards compatability [Type] Code Quality Issues or PRs that relate to code quality [Package] Components /packages/components labels Mar 15, 2024
@mirka mirka self-assigned this Mar 15, 2024
@mirka mirka requested a review from ajitbohra as a code owner March 15, 2024 14:43
Copy link

github-actions bot commented Mar 15, 2024

The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the props-bot label.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: mirka <[email protected]>
Co-authored-by: tyxla <[email protected]>
Co-authored-by: andrewhayward <[email protected]>
Co-authored-by: t-hamano <[email protected]>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

@mirka mirka requested review from t-hamano and a team March 15, 2024 14:55
Copy link
Member

@tyxla tyxla left a comment

Choose a reason for hiding this comment

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

Looking good 👍

I wonder why we don't have deprecated() calls for the rest of these props though.

Could be a good idea for a follow-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.

Copy link
Contributor

@andrewhayward andrewhayward left a comment

Choose a reason for hiding this comment

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

Tweaks how the deprecated props on Button are managed so that they are more permissive:

  • Keep deprecated props in the TypeScript type definitions, so that they will be flagged as deprecated but not block TS compilation.
  • Remove the hard deprecation version for the isDefault prop, since this is a very low maintenance back compat layer and we can pretty much keep it around indefinitely.

This makes a lot of sense. I didn't realise we were removing deprecated props from the type definition, which is a bit weird if they're still technically supported 🚀

Copy link
Contributor

@t-hamano t-hamano left a comment

Choose a reason for hiding this comment

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

LGTM!

The code editor also appears to properly mark deprecated props.

after

The deprecation rule may need to be reaffirmed for the entire Gutenberg project, not just for components. The discussion in #46105, which attempted to remove deprecated APIs from component packages, might be helpful.

@mirka mirka merged commit 2bcd06b into trunk Mar 18, 2024
63 of 65 checks passed
@mirka mirka deleted the button-deprecated-props branch March 18, 2024 12:15
@github-actions github-actions bot added this to the Gutenberg 18.0 milestone Mar 18, 2024
carstingaxion pushed a commit to carstingaxion/gutenberg that referenced this pull request Mar 27, 2024
* Button: Keep deprecated props in type definitions

* Remove hard deprecation verison for `isDefault`

* Add changelog

* Add package namespace to deprecation message

Co-authored-by: mirka <[email protected]>
Co-authored-by: tyxla <[email protected]>
Co-authored-by: andrewhayward <[email protected]>
Co-authored-by: t-hamano <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Backwards Compatibility Issues or PRs that impact backwards compatability [Package] Components /packages/components [Type] Code Quality Issues or PRs that relate to code quality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants