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

BorderControl: Promote to stable #65475

Open
wants to merge 4 commits into
base: trunk
Choose a base branch
from

Conversation

DaniGuardiola
Copy link
Contributor

@DaniGuardiola DaniGuardiola commented Sep 19, 2024

What?

Promote BorderControl to a non-experimental component.

How?

  • Export it from components without the __experimental prefix.
  • Switch subcomponents to dot notation if applicable.
  • Keep the old __experimental export for backwards compatibility.
  • Remove experimental callout from readme.
  • Change all imports of the old __experimental in GB and components to the one without the prefix (including in Storybook stories).
  • Update the docs (readme, JSDocs, Storybook) to refer to the new unprefixed component.
  • Add the component storybook id (get it from the storybook URL) to the PREVIOUSLY_EXPERIMENTAL_COMPONENTS const array in manager-head.html so that old experimental story paths are redirected to the new one.
  • Add a changelog for both the promotion (enhancement) and the deprecation.

For discussion

There are two unstable/experimental props that need to be discussed @WordPress/gutenberg-components @WordPress/gutenberg-core

  • __experimentalIsRenderedInSidebar: comes from the ColorPalette component that's used internally. Description: "Whether this is rendered in the sidebar". I guess this needs to be stabilized in the ColorPalette component itself, which is already stable. This prop seems to be used in many places with different values, so my hunch is that we'll want to make it stable.
  • __unstablePopoverProps: exclusively used in BorderBoxControl, described as "An internal prop used to control the visibility of the dropdown". It forwards props to Popover through Dropdown. I'm not sure what we'll want to do here. Popover is stable but some of its props are unstable.

Are any of these safe to stabilize? Separately, should the stabilization of these props block stabilization of the component? Let me know what you think.

Copy link

github-actions bot commented Sep 19, 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: DaniGuardiola <daniguardiola@git.wordpress.org>
Co-authored-by: youknowriad <youknowriad@git.wordpress.org>
Co-authored-by: mirka <0mirka00@git.wordpress.org>

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

export { BorderControl as __experimentalBorderControl } from './border-control';
export {
/** @deprecated Import `BorderControl` instead. */
BorderControl as __experimentalBorderControl,
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we add a deprecated call when using this one?

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

What's the reasoning for this specific components? This and the AlignmentMatrix one?

Copy link
Member

Choose a reason for hiding this comment

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

There is no strong reason for a consumer to switch over, and no immediate need for the library to remove the back-compat code (which is just an alias).

Copy link
Contributor

Choose a reason for hiding this comment

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

no immediate need for the library to remove the back-compat code (which is just an alias).

This is true but in my perspective, if we don't deprecate, this will never change. But if we do deprecate now, we'll be in a better position one/two years from now to actually remove the component if we wanted to.

This is why I think if we consider something deprecated, we should always add the deprecated call. It opens more doors for us in the future without any downside for the present.

The only cases where I won't do that is if it's something that is too impactful and that is likely to trigger warnings for all users... I would add a step there: soft deprecate and communicate and add a deprecation warning a bit later. It doesn't seem to be the case for me for these components.

Copy link
Member

Choose a reason for hiding this comment

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

This type of pros/cons weighing is already considered in the guidelines I proposed in #61099. There is a very tangible cost we shift to the consumers when we log a deprecated call. They need to hunt it down and address it, even if they get zero functional benefit in doing so. Whereas, indefinitely keeping a couple lines of back-compat code in the library is virtually free.

And when it does start to become not free for the library in aggregate, it's not too late to log a warning then. Maybe that is what you're proposing though?

Copy link
Contributor

Choose a reason for hiding this comment

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

And when it does start to become not free for the library in aggregate, it's not too late to log a warning then. Maybe that is what you're proposing though?

I'm saying that it's too late at that point to add a warning because it will cause the "removal" to be delayed by a couple years at least (people to notice the new warnings and move away from it).

On the opposite site, if you don't show the warnings, there's no incentive for people to move out. so why it's deprecated in the first place?

There is a very tangible cost we shift to the consumers when we log a deprecated call.

The cost is very small in most cases, this is what I'm arguing about. Very few people outside Core are using these two components, so the cost is small compare to the pros of incentivizing these users to move away from these deprecated APIs. At what point the cost becomes too big that it requires an initial period of communication (make/core post) before deprecation is the right question to ask IMO. I think this would be rare and only for very largely used APIs.

Copy link
Member

Choose a reason for hiding this comment

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

I'm saying that it's too late at that point to add a warning because it will cause the "removal" to be delayed by a couple years at least (people to notice the new warnings and move away from it).

The thing is that we've been maintaining components for three years or so now, and we've never experienced this level of urgency to hard remove deprecated code. And for things that we do think would be better to hard remove quickly, we do in fact log deprecation warnings as soon as possible.

On the opposite site, if you don't show the warnings, there's no incentive for people to move out. so why it's deprecated in the first place?

This is also why we wanted to define different levels of deprecation. Passive deprecations are exactly for cases where we have no reason for people to move out. It's merely an administrative change for the library. It adds clarity/simplicity for new consumers, but existing usages don't gain anything by switching over.

The cost is very small in most cases, this is what I'm arguing about. Very few people outside Core are using these two components, so the cost is small compare to the pros of incentivizing these users to move away from these deprecated APIs.

This is about a subjective perception of cost, so it's hard to weigh objectively. At least from the usage checks we've been doing for components, AlignmentMatrixControl and BorderControl are not low-usage, relatively speaking. And we've been getting feedback from extenders that it's definitely a hassle to deal with deprecation warnings, especially in a way that works across WP versions. To me, keeping two lines of back-compat code is cheaper than the cost on the ecosystem to switch over for no tangible benefit. And whenever we start to feel the cost analysis flipping, it likely won't be too late.

I actually think it's more likely that we'll reach a point where these (stable) components need to be deprecated entirely and/or superseded by a v2, sooner than we'll reach a point where the back-compat code is too costly to maintain.

Copy link
Contributor

Choose a reason for hiding this comment

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

This is also why we wanted to define different levels of deprecation. Passive deprecations are exactly for cases where we have no reason for people to move out. It's merely an administrative change for the library. It adds clarity/simplicity for new consumers, but existing usages don't gain anything by switching over.

I'm not sure I agree that keeping deprecated APIs forever is a good idea personally. Also, there's no urgency today but there might be tomorrow. So having the possibility to remove a deprecated API tomorrow is for me a good thing to have.

Say, two years have passed and for some random reason, I don't want to keep these around? I would be very happy to know that we had a message warning developers that these are deprecated for two years now. That message is costless IMO, neither for us, nor for the very few people that actually use that component.

Also, I don't like that we treat the "components" package as separate from all the other APIs in Gutenberg. If we have a strategy, we should have a consistent one.

Copy link
Contributor

Choose a reason for hiding this comment

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

While I stand by my reasoning. I don't mind disagreeing and committing to any approach here.

I think the important piece of feedback for me here is the latter

Also, I don't like that we treat the "components" package as separate from all the other APIs in Gutenberg. If we have a strategy, we should have a consistent one.

I think we should stop treating components as a special package and whatever we do for that package, we should do for the rest.

@draganescu draganescu added [Feature] UI Components Impacts or related to the UI component system [Type] Code Quality Issues or PRs that relate to code quality [Feature] Component System WordPress component system labels Sep 19, 2024
@DaniGuardiola DaniGuardiola added [Package] Components /packages/components and removed [Feature] UI Components Impacts or related to the UI component system [Feature] Component System WordPress component system labels Sep 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[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