Components: use colord instead of tinycolor2 in ColorPicker

[ciampo]

Part of #34284

Why

The colord library may be a better replacement for tinycolor2, which is currently used by the ColorPicker component:

• it has a smaller size
• it comes with first-party TS types

What

The maintainer of colord reached out to us in a previous PR and suggested we take a look at the colord library.

We should first assess the feasibility of this refactor (partially started in #34014 (comment))

A/C

• Assess if colord provides all needed functionality
• Assess if any breaking changes are introduced
• Swap the libraries, update imports

[jorgefilipecosta]

It seems like colord combined with the names and a11y plugins provides the functionality we need. The readability from tinyColor that is widely used is the same number as contrast in colors. There is a function in tinycolor2 to return the most readable color from an array of colors that we also use in some cases including for providing a public API abstraction on top of (getMostReadableColor), that has nothing equivalent in colord but it is just a simple loop checking the contrast of all the colors in an array and returning the highest so we can easily implement it.

I think as long as we add the two plugins we don't need to introduce any breaking change.

[gziolo]

I see this call in #34605:

import { colord, extend } from 'colord';
import namesPlugin from 'colord/plugins/names';
import a11yPlugin from 'colord/plugins/a11y';

extend( [ namesPlugin, a11yPlugin ] );

How is it going to scale? Can you extend with the same plugin multiple times?

I would also argue if the bundle size is going to be the deciding factor with the plugin architecture, the core library is smaller than tinycolor2 (5 kB):

but there is additional overhead when using plugins:

Anyway, I don't care what you end up using eventually. I just wanted to raise some concerns I had. The call is on your side if it's worth an effort.

[ciampo]

I think as long as we add the two plugins we don't need to introduce any breaking change.

From our initial investigation, I remember finding a potential (small) breaking change: #34014 (comment)

@jorgefilipecosta, would you mind looking into it as well and confirming if our initial findings were correct?

Can you extend with the same plugin multiple times?

It looks like the extend function supports an array of active plugins, so that shouldn't be a problem.

Edit: I misread your question. That's actually a great question and something that we should look into — we may need to initialise the plugin in a separate function and maybe implement something similar to the singleton pattern to use the library

Anyway, I don't care what you end up using eventually. I just wanted to raise some concerns I had. The call is on your side if it's worth an effort.

Your concerns are definitely relevant! Size is only one potential improvement that we'd enjoy when switching to colord — this library should in fact be also faster at runtime, and comes with first-party types (see this benchmark)

[jorgefilipecosta]

@jorgefilipecosta, would you mind looking into it as well and confirming if our initial findings were correct?

Hi @ciampo,
I did not notice the color picker component was including a tinycolor object on the callback. I shared some thoughts on #34014 (comment). But in summary, I think we should not return that object, we never documented anywhere that we do it and even if there was no component library change I think we should do the change sooner rather than later. We should also remove oldHue and source from there, this should be internal information.

[jorgefilipecosta]

Hi @gziolo,

How is it going to scale? Can you extend with the same plugin multiple times?

Yes one can extend with the same plugin multiple times
https://github.com/omgovich/colord/blob/9fe0e2b8c11ec2a6ed371137d52966f0a38dc890/src/extend.ts#L9-L16

If the plugin is already registered as an extension the call to extend does nothing.

So I guess the current approach scales and may work, but it is not ideal from the developer experience point of view it would be better to not have to call extend all the time. But I'm not sure how to do that I would appreciate any insights here.

[gziolo]

So I guess the current approach scales and may work, but it is not ideal from the developer experience point of view it would be better to not have to call extend all the time. But I'm not sure how to do that I would appreciate any insights here.

Let's add those extend calls where necessary for now. We can abstract it later if necessary. In practice, this API prevents tree-shaking because the imported extensions are used in the code in extend call so tools will assume that they should be never removed even when they aren't used. It isn't an issue in Gutenberg, but it's a minor thing for 3rd party projects that want to use a subset of components from @wordpress/components.

[ciampo]

We should also remove oldHue and source from there, this should be internal information.

I quickly looked into oldHue, and found this comment, which may be worth investigating to make sure that this issue wouldn't be there in the new version of the component.

On the other hand, I couldn't find any usage of the source prop 👌

The current legacy adapter already takes care of marking those two properties as deprecated. Probably, to avoid introducing more breaking changes than necessary, the better option is to leave the code for the oldHue and source props as-is. We should leave these props marked as deprecated, while not mentioning them in official docs.

I think we should not return that object, we never documented anywhere that we do it and even if there was no component library change I think we should do the change sooner rather than later.

Regarding the removal of the color property (which is a tinycolor2 object), we need to first make sure that there are no usages in Gutenberg — from a quick search, I could find a couple of usages in packages/components/src/custom-gradient-bar/control-points.js

After that's done, we can remove the color property from the values returned in the legacy adapter, and proceed to migrate from tinycolor2 to colord

[jorgefilipecosta]

I quickly looked into oldHue, and found this comment, which may be worth investigating to make sure that this issue wouldn't be there in the new version of the component.

Good point, I guess we may need to have an oldHue in the state to avoid the issue but we should not pass it on to the onChange it may be relevant internally for the component but is not relevant for users of the component.

[jorgefilipecosta]

Regarding the removal of the color property (which is a tinycolor2 object), we need to first make sure that there are no usages in Gutenberg — from a quick search, I could find a couple of usages in packages/components/src/custom-gradient-bar/control-points.js

These should be taken care of when we remove tinycolor for the gradient-related components.

[ciampo]

These should be taken care of when we remove tinycolor for the gradient-related components.

Absolutely — sorry if I didn't explain myself clearly!

Nonetheless, something to keep in mind — I assume that the refactor from tinycolor2 to colord should apply to all components in the package (although of course it should happen in smaller steps across different PRs)