Components: move border style utils to block-editor package

[DaniGuardiola]

There are some border-related utilities marked as experimental being exported from the components package:

gutenberg/packages/components/src/index.ts

Lines 33 to 35 in d89e5a8

	hasSplitBorders as __experimentalHasSplitBorders,
	isDefinedBorder as __experimentalIsDefinedBorder,
	isEmptyBorder as __experimentalIsEmptyBorder,

Specifically:

• hasSplitBorders
• isDefinedBorder
• isEmptyBorder

This raised some alarms when making experimental components officially public, since they seem out of place, see comment: #60837 (comment)

These utilities are used in various different packages and places across the Gutenberg repo. Upon investigation, I believe a better home for them is probably the block-editor editor package. See the README: https://github.com/WordPress/gutenberg/blob/dcbccfb1ca51c560481c018a9c6f45b0ffd6aee9/packages/block-editor/README.md

This package contains a lot of style-related utilities for blocks, including the following:

• getColorClassName
• getColorObjectByAttributeValues
• getColorObjectByColorValue
• getComputedFluidTypographyValue
• getCustomValueFromPreset
• getFontSize
• getFontSizeClass
• getFontSizeObjectByValue
• getGradientSlugByValue
• getGradientValueBySlug
• getPxFromCssUnit
• getSpacingPresetCssVar
• getTypographyClassesAndStyles

cc @mirka

[mirka]

Since some of this logic is also used in the component itself, the problem with moving it to wp-block-editor is that we'd either need to duplicate the logic, or we'd be importing functions from wp-block-editor into wp-components which is a violation of the dependency direction.

I think the order of preference should be:

1. Remove (deprecate) if possible
2. Incorporate into the component API if possible
3. Move to another package

I noticed that isEmptyBorder is not used in the app anymore, so that seems like a candidate for deprecation.

For the other two, have we considered passing them as metadata to the onChange callback? Perhaps as the second argument.

[DaniGuardiola]

@mirka you're right, I hadn't realized that. Seems like I'll need to think deeper about this. Thanks for your input.

[fullofcaffeine]

@DaniGuardiola and I discussed this today. Ideally, as Dani originally suggested, we'd move these experimental helpers to another package. We can't move them to block-editor because, as @mirka mentioned, it'd cause a circular dependency. It'd also be overkill to move only those three to a brand new package.

We concluded that the ideal scenario would be to move those and the semantically related functions (listed above by Dani) to a dedicated style-utils package (or something named along those lines). Since those utilities are spread across different modules with different semantics (hooks, helpers, etc), we'd like to get the opinion of people more acquainted with the code around that before investing time in such a task, though cc @mirka @tyxla would love to hear your insights and/or know who else to ping here about this.

As a first step to reach the goal of cleaning up the wp-components re-exports, we thought that a good compromise was to copy the helper functions that are used outside closer to the consumers in the Gutenberg app. It seems that a module in the block-editor package would make sense for now since there are other similar helpers being exported from there, and there are also some consumers there. I started an experiment/PoC here: #62155. I think we could start with this as we discuss the idea of refactoring them into a new package.

Looking forward to reading your thoughts!

[mirka]

I think the general approach in #62155 is good enough to mitigate the immediate problem 👍

I wouldn't go as far as to introduce a dedicated styles-utils package, because honestly I think the fact that we need helper utils like this in so many places is a smell in itself, and not a pattern I want to encourage. If the hasSplitBorders logic was that crucial for so many consumers to process border data, that should've been considered in the initial API design. For example, by giving the value object a shape that contains the answer in a much more straightforward way:

// When split
{ top: { color: "red" }, left: { color: "red" }, ... }

// When not split
{ all: { color: "red" } } // now `hasSplitBorders()` can be replaced with a simple inline `border.all` check

I really do hope this border data is an outlier, and we won't see more like it.

[tyxla]

Agreed with @mirka. We actively discourage utility packages and creating new packages should generally avoided, unless there's a niche need a package would occupy. Utility function packages are almost never a good scenario for that.